Fix broken table when no branches available (detached HEAD on checkout)

[julieg18]

main

PR

https://github.com/iterative/vscode-dvc/assets/43496356/331ef8cf-737b-49fb-97e4-7def706e5afd

Fixes bug mentioned in slack thread

[mattseddon]

[C] We now have two sets of logic. One here and one at:

vscode-dvc/extension/src/experiments/model/collect.ts

Lines 497 to 501 in a2920aa

	const cleanBranch = branch
	.replace('* ', '')
	.replace(/\(HEAD\s\w+\s\w+\s/, '')
	.replace(')', '')

Maybe that is ok but it would be good to pull out constants like HEAD & (no branch) as they may serve as breadcrumbs later. We could also move isCurrentBranch into the git folder.

A good practice would be to (generally) encapsulate all of the logic in the same place. I've tried to leave the reader as dumb as possible previously and not put any transformation logic in here at all (an exception being trim and split). The reason for encapsulation is that in future when things inevitably break we won't be searching through the code to work out all of the possible places something has been changed.

With all of that said maybe the correct place for this logic is in:

vscode-dvc/extension/src/experiments/data/index.ts

Lines 125 to 132 in a2920aa

	private async updateBranches() {
	const allBranches = await this.internalCommands.executeCommand<string[]>(
	AvailableCommands.GIT_GET_BRANCHES,
	this.dvcRoot
	)
	this.experiments.setBranches(allBranches)
	}

after calling setBranches check the current branch and overwrite with revParseHead if it matches (no branch) this is a special case so we should still avoid calling revParseHead if necessary.

Does all of that make sense?

[mattseddon]

Another (more involved) way of fixing the issue is assuming that these commands:

vscode-dvc/extension/src/experiments/data/index.ts

Lines 85 to 97 in a2920aa

	const [branchLog, totalCommits] = await Promise.all([
	this.internalCommands.executeCommand(
	AvailableCommands.GIT_GET_COMMIT_MESSAGES,
	this.dvcRoot,
	branch,
	String(nbOfCommitsToShow)
	),
	this.internalCommands.executeCommand<number>(
	AvailableCommands.GIT_GET_NUM_COMMITS,
	this.dvcRoot,
	branch
	)
	])

will work if we don't pass a branch to them (needs tested). We could then leave the detached HEAD/no branch strings as is (I think) and not pass a branch for the current branch. If this all worked as expected it should lead to a slightly better UX of showing the original branch name in the experiments table (e.g (no branch) so that they user knows that they currently don't have a branch to work with.

[julieg18]

Does all of that make sense?

Yes, makes sense! I'll move this branch parsing logic to updateBranches.

will work if we don't pass a branch to them (needs tested). We could then leave the detached HEAD/no branch strings as is (I think) and not pass a branch for the current branch. If

Did a quick test and GIT_GET_NUM_COMMITS needs a branch to work with.

[mattseddon]

GIT_GET_NUM_COMMITS <= could this use HEAD?

[mattseddon]

Making these updates seems to work (rough):

  private async updateExpShow() {
    await this.updateBranches()
    const [currentBranch, ...branches] = this.experiments.getBranchesToShow()
    const availableNbCommits: { [branch: string]: number } = {}

    const promises = []

    promises.push(
      this.collectGitLogByBranch(currentBranch, availableNbCommits, true)
    )

    for (const branch of branches) {
      promises.push(this.collectGitLogByBranch(branch, availableNbCommits))
    }

    const branchLogs = await Promise.all(promises)
    const { args, gitLog, rowOrder } = this.collectGitLogAndOrder(branchLogs)

    const expShow = await this.internalCommands.executeCommand<ExpShowOutput>(
      AvailableCommands.EXP_SHOW,
      this.dvcRoot,
      ...args
    )

    this.collectFiles({ expShow })
    return { availableNbCommits, expShow, gitLog, rowOrder }
  }

  private async collectGitLogByBranch(
    branch: string,
    availableNbCommits: { [branch: string]: number },
    isCurrent?: boolean
  ) {
    const nbOfCommitsToShow = this.experiments.getNbOfCommitsToShow(branch)

    const [branchLog, totalCommits] = await Promise.all([
      this.internalCommands.executeCommand(
        AvailableCommands.GIT_GET_COMMIT_MESSAGES,
        this.dvcRoot,
        isCurrent ? 'HEAD' : branch,
        String(nbOfCommitsToShow)
      ),
      this.internalCommands.executeCommand<number>(
        AvailableCommands.GIT_GET_NUM_COMMITS,
        this.dvcRoot,
        isCurrent ? 'HEAD' : branch
      )
    ])

    availableNbCommits[branch] = totalCommits

    return {
      branch,
      branchLog: isCurrent ? branchLog.replace('HEAD', branch) : branchLog
    }
  }

...

  private async updateBranches() {
    const allBranches = await this.internalCommands.executeCommand<string[]>(
      AvailableCommands.GIT_GET_BRANCHES,
      this.dvcRoot
    )

    const { currentBranch, branches } = collectBranches(allBranches)

    this.experiments.setBranches(branches, currentBranch)
  }

This would also mean we could remove env: { LANG: 'en_US.UTF-8' } from the GitReader.

Happy to pick up this second change if I won't be stepping on your toes.

[mattseddon]

note: also need to update collectBranches:

    const cleanBranch = branch.replace('* ', '')
    // .replace(new RegExp(`\\(${gitPath.DOT_GIT_HEAD}\\s\\w+\\s\\w+\\s`), '')
    // .replace(')', '')

[julieg18]

Ohhhhhh, ok, I understand your solution idea a lot better, thanks! I'll apply your changes + add tests.

[julieg18]

Resolved the review comments and going to rerequest reviews since I've changed the PR quite a bit

[codeclimate]

Code Climate has analyzed commit dbdd0c9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 96.6% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.3% (0.0% change).

View more on Code Climate.

[mattseddon]

[F] An explanatory variable would have been good here. The reason being when we come back to this we would have some idea as to why we made this decision. Example would be

• detachedSafeBranchName
• safeBranchName

The information won't be lost as we can always come back to this PR (I find the PR via Gitlens file/line history) but you are saving your future self some work by fighting to make the code as expressive as possible 👍🏻.