Skip to content

Commit e7857d8

Browse files
julieg18julieg18
authored
Fix broken table when no branches available (detached HEAD on checkout) (#4426)
1 parent 6151b0f commit e7857d8

File tree

13 files changed

+253
-140
lines changed

13 files changed

+253
-140
lines changed

‎extension/src/cli/git/reader.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ describe('GitReader', () => {
5454
expect(mockedCreateProcess).toHaveBeenCalledWith({
5555
args: ['branch'],
5656
cwd,
57-
env: { GIT_OPTIONAL_LOCKS: '0', LANG: 'en_US.UTF-8' },
5857
executable: 'git'
5958
})
6059
})

‎extension/src/cli/git/reader.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,7 @@ export class GitReader extends GitCli {
103103
public async getBranches(cwd: string): Promise<string[]> {
104104
const options = getOptions({
105105
args: [Command.BRANCH],
106-
cwd,
107-
env: { LANG: 'en_US.UTF-8' }
106+
cwd
108107
})
109108
try {
110109
const branches = await this.executeProcess(options)

‎extension/src/experiments/data/collect.test.ts‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { join } from 'path'
2-
import { collectFiles } from './collect'
2+
import { collectBranches, collectFiles } from './collect'
33
import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
44
import expShowFixture from '../../test/fixtures/expShow/base/output'
55
import { generateTestExpShowOutput } from '../../test/util/experiments'
@@ -86,3 +86,54 @@ describe('collectFiles', () => {
8686
])
8787
})
8888
})
89+
90+
describe('collectBranches', () => {
91+
it('should correctly parse the git branch output', () => {
92+
const { branches, branchesToSelect, currentBranch } = collectBranches([
93+
'* main',
94+
'exp-12',
95+
'fix-bug-11',
96+
'other'
97+
])
98+
99+
expect(branches).toStrictEqual(['main', 'exp-12', 'fix-bug-11', 'other'])
100+
expect(branchesToSelect).toStrictEqual(['exp-12', 'fix-bug-11', 'other'])
101+
expect(currentBranch).toStrictEqual('main')
102+
})
103+
104+
it('should correct parse a detached head branch', () => {
105+
const { branches, branchesToSelect, currentBranch } = collectBranches([
106+
'* (HEAD detached at 201a9a5)',
107+
'exp-12',
108+
'fix-bug-11',
109+
'other'
110+
])
111+
112+
expect(branchesToSelect).toStrictEqual(['exp-12', 'fix-bug-11', 'other'])
113+
expect(branches).toStrictEqual([
114+
'(HEAD detached at 201a9a5)',
115+
'exp-12',
116+
'fix-bug-11',
117+
'other'
118+
])
119+
expect(currentBranch).toStrictEqual('(HEAD detached at 201a9a5)')
120+
})
121+
122+
it('should correct parse a "no-branch" output', () => {
123+
const { branches, currentBranch, branchesToSelect } = collectBranches([
124+
'exp-12',
125+
'* (no-branch)',
126+
'fix-bug-11',
127+
'other'
128+
])
129+
130+
expect(branches).toStrictEqual([
131+
'exp-12',
132+
'(no-branch)',
133+
'fix-bug-11',
134+
'other'
135+
])
136+
expect(currentBranch).toStrictEqual('(no-branch)')
137+
expect(branchesToSelect).toStrictEqual(['exp-12', 'fix-bug-11', 'other'])
138+
})
139+
})

‎extension/src/experiments/data/collect.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,36 @@ export const collectFiles = (
2222
...existingFiles
2323
])
2424
}
25+
26+
const isCurrentBranch = (branch: string) => branch.indexOf('*') === 0
27+
28+
export const collectBranches = (
29+
allBranches: string[]
30+
): {
31+
currentBranch: string
32+
branches: string[]
33+
branchesToSelect: string[]
34+
} => {
35+
let currentBranch = ''
36+
const branches: string[] = []
37+
const branchesToSelect: string[] = []
38+
39+
for (const branch of allBranches) {
40+
const isCurrent = isCurrentBranch(branch)
41+
42+
const cleanBranch = branch.replace('* ', '')
43+
44+
branches.push(cleanBranch)
45+
46+
if (isCurrent) {
47+
if (!currentBranch) {
48+
currentBranch = cleanBranch
49+
}
50+
continue
51+
}
52+
53+
branchesToSelect.push(cleanBranch)
54+
}
55+
56+
return { branches, branchesToSelect, currentBranch }
57+
}

‎extension/src/experiments/data/index.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { collectFiles } from './collect'
1+
import { collectBranches, collectFiles } from './collect'
22
import {
33
EXPERIMENTS_GIT_LOGS_REFS,
44
EXPERIMENTS_GIT_REFS,
@@ -55,10 +55,15 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
5555

5656
private async updateExpShow() {
5757
await this.updateBranches()
58-
const branches = this.experiments.getBranchesToShow()
58+
const [currentBranch, ...branches] = this.experiments.getBranchesToShow()
5959
const availableNbCommits: { [branch: string]: number } = {}
6060

6161
const promises = []
62+
63+
promises.push(
64+
this.collectGitLogByBranch(currentBranch, availableNbCommits, true)
65+
)
66+
6267
for (const branch of branches) {
6368
promises.push(this.collectGitLogByBranch(branch, availableNbCommits))
6469
}
@@ -78,21 +83,23 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
7883

7984
private async collectGitLogByBranch(
8085
branch: string,
81-
availableNbCommits: { [branch: string]: number }
86+
availableNbCommits: { [branch: string]: number },
87+
isCurrent?: boolean
8288
) {
8389
const nbOfCommitsToShow = this.experiments.getNbOfCommitsToShow(branch)
90+
const branchName = isCurrent ? gitPath.DOT_GIT_HEAD : branch
8491

8592
const [branchLog, totalCommits] = await Promise.all([
8693
this.internalCommands.executeCommand(
8794
AvailableCommands.GIT_GET_COMMIT_MESSAGES,
8895
this.dvcRoot,
89-
branch,
96+
branchName,
9097
String(nbOfCommitsToShow)
9198
),
9299
this.internalCommands.executeCommand<number>(
93100
AvailableCommands.GIT_GET_NUM_COMMITS,
94101
this.dvcRoot,
95-
branch
102+
branchName
96103
)
97104
])
98105

@@ -128,7 +135,10 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
128135
this.dvcRoot
129136
)
130137

131-
this.experiments.setBranches(allBranches)
138+
const { currentBranch, branches, branchesToSelect } =
139+
collectBranches(allBranches)
140+
141+
this.experiments.setBranches(branches, branchesToSelect, currentBranch)
132142
}
133143

134144
private collectFiles({ expShow }: { expShow: ExpShowOutput }) {

‎extension/src/experiments/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,10 @@ export class Experiments extends BaseRepository<TableData> {
581581
return this.pipeline.getCwd()
582582
}
583583

584+
public getAvailableBranchesToSelect() {
585+
return this.experiments.getAvailableBranchesToSelect()
586+
}
587+
584588
protected sendInitialWebviewData() {
585589
return this.webviewMessages.sendWebviewMessage()
586590
}

‎extension/src/experiments/model/collect.ts‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -483,32 +483,6 @@ export const collectRunningInWorkspace = (
483483
}
484484
}
485485

486-
export const isCurrentBranch = (branch: string) => branch.indexOf('*') === 0
487-
488-
export const collectBranches = (
489-
allBranches: string[]
490-
): { currentBranch: string; branches: string[] } => {
491-
let currentBranch = ''
492-
const branches: string[] = []
493-
494-
for (const branch of allBranches) {
495-
const isCurrent = isCurrentBranch(branch)
496-
497-
const cleanBranch = branch
498-
.replace('* ', '')
499-
.replace(/\(HEAD\s\w+\s\w+\s/, '')
500-
.replace(')', '')
501-
502-
if (!currentBranch && isCurrent) {
503-
currentBranch = cleanBranch
504-
}
505-
506-
branches.push(cleanBranch)
507-
}
508-
509-
return { branches, currentBranch }
510-
}
511-
512486
export const collectRemoteExpShas = (remoteExpRefs: string): Set<string> => {
513487
const remoteExpShas = new Set<string>()
514488
for (const ref of trimAndSplit(remoteExpRefs)) {

‎extension/src/experiments/model/index.test.ts‎

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -372,16 +372,42 @@ describe('ExperimentsModel', () => {
372372
const model = new ExperimentsModel('', buildMockMemento())
373373

374374
model.setSelectedBranches(['A', 'old', 'B', 'C', 'older'])
375-
model.setBranches(['A', 'B', 'C', 'four', 'five', '* six'])
375+
model.setBranches(
376+
['A', 'B', 'C', 'four', 'five', 'six'],
377+
['A', 'B', 'C', 'four', 'five'],
378+
'six'
379+
)
376380

377381
expect(model.getBranchesToShow()).toStrictEqual(['six', 'A', 'B', 'C'])
378382
})
379383

384+
it('should return all selectable branches', () => {
385+
const model = new ExperimentsModel('', buildMockMemento())
386+
387+
model.setBranches(
388+
['main', 'A', 'B', 'C', 'four', 'five'],
389+
['A', 'B', 'C', 'four', 'five'],
390+
'main'
391+
)
392+
393+
expect(model.getAvailableBranchesToSelect()).toStrictEqual([
394+
'A',
395+
'B',
396+
'C',
397+
'four',
398+
'five'
399+
])
400+
})
401+
380402
it('should remove outdated selected branches when calling setBranches', () => {
381403
const model = new ExperimentsModel('', buildMockMemento())
382404

383405
model.setSelectedBranches(['A', 'old', 'B', 'C', 'older'])
384-
model.setBranches(['* main', 'A', 'B', 'C', 'four', 'five', 'six'])
406+
model.setBranches(
407+
['main', 'A', 'B', 'C', 'four', 'five', 'six'],
408+
['A', 'B', 'C', 'four', 'five', 'six'],
409+
'main'
410+
)
385411

386412
expect(model.getBranchesToShow()).toStrictEqual(['main', 'A', 'B', 'C'])
387413
})
@@ -391,7 +417,11 @@ describe('ExperimentsModel', () => {
391417
const model = new ExperimentsModel('', memento)
392418

393419
model.setSelectedBranches(['A', 'old', 'B', 'C', 'older'])
394-
model.setBranches(['A', 'B', 'C', '* four', 'five', 'six'])
420+
model.setBranches(
421+
['A', 'B', 'C', 'five', 'four', 'six'],
422+
['A', 'B', 'C', 'five', 'six'],
423+
'four'
424+
)
395425

396426
expect(memento.get(PersistenceKey.EXPERIMENTS_BRANCHES)).toStrictEqual([
397427
'A',
@@ -420,7 +450,11 @@ describe('ExperimentsModel', () => {
420450
const model = new ExperimentsModel('', memento)
421451

422452
model.setSelectedBranches(['A', 'old', 'B', 'C', 'older'])
423-
model.setBranches(['* A', 'B', 'C', 'four', 'five', 'six'])
453+
model.setBranches(
454+
['B', 'A', 'C', 'four', 'five', 'six'],
455+
['B', 'C', 'four', 'five', 'six'],
456+
'A'
457+
)
424458

425459
expect(model.getBranchesToShow()).toStrictEqual(['A', 'B', 'C'])
426460
expect(memento.get(PersistenceKey.EXPERIMENTS_BRANCHES)).toStrictEqual([
@@ -568,27 +602,4 @@ describe('ExperimentsModel', () => {
568602

569603
expect(model.getCliError()).toBe(undefined)
570604
})
571-
572-
it('should correctly parse the git branch output', () => {
573-
const model = new ExperimentsModel('', buildMockMemento())
574-
const allBranches = ['* main', 'exp-12', 'fix-bug-11', 'other']
575-
const selectedBranches = allBranches.slice(1, 2)
576-
model.setBranches(allBranches)
577-
model.setSelectedBranches(selectedBranches)
578-
expect(model.getBranchesToShow()).toStrictEqual([
579-
'main',
580-
...selectedBranches
581-
])
582-
583-
model.setBranches(
584-
allBranches.map(branch =>
585-
branch.replace('main', '(HEAD detached at 029da11)')
586-
)
587-
)
588-
589-
expect(model.getBranchesToShow()).toStrictEqual([
590-
'029da11',
591-
...selectedBranches
592-
])
593-
})
594605
})

‎extension/src/experiments/model/index.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { FilterDefinition, getFilterId } from './filterBy'
44
import { collectFiltered, collectUnfiltered } from './filterBy/collect'
55
import {
66
collectAddRemoveCommitsDetails,
7-
collectBranches,
87
collectExperiments,
98
collectOrderedCommitsAndExperiments,
109
collectRemoteExpShas,
@@ -69,6 +68,7 @@ export class ExperimentsModel extends ModelWithPersistence {
6968
private currentBranch: string | undefined
7069
private selectedBranches: string[] = []
7170
private availableBranchesToShow: string[] = []
71+
private availableBranchesToSelect: string[] = []
7272
private hasMoreCommits: { [branch: string]: boolean } = {}
7373
private isShowingMoreCommits: { [branch: string]: boolean } = {}
7474

@@ -490,10 +490,13 @@ export class ExperimentsModel extends ModelWithPersistence {
490490
return this.numberOfCommitsToShow
491491
}
492492

493-
public setBranches(allBranches: string[]) {
494-
const { currentBranch, branches } = collectBranches(allBranches)
495-
493+
public setBranches(
494+
branches: string[],
495+
branchesToSelect: string[],
496+
currentBranch: string
497+
) {
496498
this.availableBranchesToShow = branches
499+
this.availableBranchesToSelect = branchesToSelect
497500
this.currentBranch = currentBranch
498501

499502
this.selectedBranches = this.selectedBranches.filter(
@@ -521,6 +524,10 @@ export class ExperimentsModel extends ModelWithPersistence {
521524
return this.availableBranchesToShow
522525
}
523526

527+
public getAvailableBranchesToSelect() {
528+
return this.availableBranchesToSelect
529+
}
530+
524531
private findIndexByPath(pathToRemove: string) {
525532
return this.currentSorts.findIndex(({ path }) => path === pathToRemove)
526533
}

0 commit comments

Comments
 (0)