Fix missing commit messages in experiment branches (#3053)

Author: julieg18

extension/src/cli/git/constants.ts

@@ -48,4 +48,6 @@ export enum Commit {
 
 export const DEFAULT_REMOTE = 'origin'
 
+export const COMMITS_SEPARATOR = '\u0000'
+
 export type Args = (Command | Flag | Commit | typeof DEFAULT_REMOTE | string)[]

extension/src/cli/git/reader.ts

@@ -7,7 +7,7 @@ import { trimAndSplit } from '../../util/stdout'
 import { isDirectory } from '../../fileSystem'
 
 export const autoRegisteredCommands = {
-  GIT_GET_LAST_THREE_COMMIT_MESSAGES: 'getLastThreeCommitMessages',
+  GIT_GET_COMMIT_MESSAGES: 'getCommitMessages',
   GIT_GET_REPOSITORY_ROOT: 'getGitRepositoryRoot',
   GIT_HAS_CHANGES: 'hasChanges',
   GIT_LIST_UNTRACKED: 'listUntracked'
@@ -31,26 +31,18 @@ export class GitReader extends GitCli {
     return !!output
   }
 
-  public async getLastThreeCommitMessages(cwd: string): Promise<{
-    [sha: string]: string
-  }> {
+  public async getCommitMessages(cwd: string, sha: string): Promise<string> {
     const options = getOptions(
       cwd,
       Command.LOG,
+      `${sha}^..HEAD`,
       Flag.PRETTY_FORMAT_COMMIT_MESSAGE,
-      Flag.NUMBER,
-      '6',
       Flag.SEPARATE_WITH_NULL
     )
     try {
-      const output = await this.executeProcess(options)
-      const commits = output.split('\u0000').map(commit => {
-        const [sha, ...splitMessage] = commit.split(' ')
-        return [sha, splitMessage.join(' ')]
-      })
-      return Object.fromEntries(commits)
+      return await this.executeProcess(options)
     } catch {
-      return {}
+      return ''
     }
   }
 

extension/src/experiments/index.ts

@@ -173,15 +173,15 @@ export class Experiments extends BaseRepository<TableData> {
 
   public async setState(data: ExperimentsOutput) {
     const dvcLiveOnly = await this.checkSignalFile()
-
-    const commitMessages: { [sha: string]: string } =
-      await this.internalCommands.executeCommand(
-        AvailableCommands.GIT_GET_LAST_THREE_COMMIT_MESSAGES,
-        this.dvcRoot
-      )
+    const dataKeys = Object.keys(data)
+    const commitsOutput = await this.internalCommands.executeCommand(
+      AvailableCommands.GIT_GET_COMMIT_MESSAGES,
+      this.dvcRoot,
+      dataKeys[dataKeys.length - 1]
+    )
     await Promise.all([
       this.columns.transformAndSet(data),
-      this.experiments.transformAndSet(data, dvcLiveOnly, commitMessages)
+      this.experiments.transformAndSet(data, dvcLiveOnly, commitsOutput)
     ])
 
     return this.notifyChanged(data)

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

@@ -5,17 +5,18 @@ import {
   ExperimentStatus,
   EXPERIMENT_WORKSPACE_ID
 } from '../../cli/dvc/contract'
+import { COMMITS_SEPARATOR } from '../../cli/git/constants'
 
 describe('collectExperiments', () => {
-  it('should return an empty array if no branches are present', async () => {
-    const { branches } = await collectExperiments(
+  it('should return an empty array if no branches are present', () => {
+    const { branches } = collectExperiments(
       {
         [EXPERIMENT_WORKSPACE_ID]: {
           baseline: {}
         }
       },
       false,
-      {}
+      ''
     )
     expect(branches).toStrictEqual([])
   })
@@ -39,56 +40,67 @@ describe('collectExperiments', () => {
     }
   }
 
-  it('should define a workspace', async () => {
-    const { workspace } = await collectExperiments(
-      repoWithTwoBranches,
-      false,
-      {}
-    )
+  it('should define a workspace', () => {
+    const { workspace } = collectExperiments(repoWithTwoBranches, false, '')
 
     expect(workspace).toBeDefined()
   })
 
-  it('should find two branches from a repo with two branches', async () => {
-    const { branches } = await collectExperiments(
-      repoWithTwoBranches,
-      false,
-      {}
-    )
+  it('should find two branches from a repo with two branches', () => {
+    const { branches } = collectExperiments(repoWithTwoBranches, false, '')
 
     expect(branches.length).toStrictEqual(2)
   })
 
-  it('should list branches in the same order as they are collected', async () => {
-    const { branches } = await collectExperiments(
-      repoWithTwoBranches,
-      false,
-      {}
-    )
+  it('should list branches in the same order as they are collected', () => {
+    const { branches } = collectExperiments(repoWithTwoBranches, false, '')
     const [branchA, branchB] = branches
 
     expect(branchA.id).toStrictEqual('branchA')
     expect(branchB.id).toStrictEqual('branchB')
   })
 
-  it('should find two experiments on branchA', async () => {
-    const { experimentsByBranch } = await collectExperiments(
+  it('should find two experiments on branchA', () => {
+    const { experimentsByBranch } = collectExperiments(
       repoWithTwoBranches,
       false,
-      {}
+      ''
     )
     expect(experimentsByBranch.get('branchA')?.length).toStrictEqual(2)
   })
 
-  it('should find no experiments on branchB', async () => {
-    const { experimentsByBranch } = await collectExperiments(
+  it('should find no experiments on branchB', () => {
+    const { experimentsByBranch } = collectExperiments(
       repoWithTwoBranches,
       false,
-      {}
+      ''
     )
     expect(experimentsByBranch.get('branchB')).toBeUndefined()
   })
 
+  it('should add git commit messages to branches if git log output is provided', () => {
+    const { branches } = collectExperiments(
+      {
+        [EXPERIMENT_WORKSPACE_ID]: {
+          baseline: {}
+        },
+        a123: {
+          baseline: { data: {} }
+        },
+        b123: {
+          baseline: { data: {} }
+        }
+      },
+      false,
+      `a123 add new feature${COMMITS_SEPARATOR}b123 update various dependencies\n* update dvc\n* update jest`
+    )
+    const [branch1, branch2] = branches
+    expect(branch1.displayNameOrParent).toStrictEqual('add new feature')
+    expect(branch2.displayNameOrParent).toStrictEqual(
+      'update various dependencies ...'
+    )
+  })
+
   const repoWithNestedCheckpoints = {
     [EXPERIMENT_WORKSPACE_ID]: { baseline: {} },
     branchA: {
@@ -108,31 +120,31 @@ describe('collectExperiments', () => {
     }
   }
 
-  it('should only list the tip as a top-level experiment', async () => {
-    const { experimentsByBranch } = await collectExperiments(
+  it('should only list the tip as a top-level experiment', () => {
+    const { experimentsByBranch } = collectExperiments(
       repoWithNestedCheckpoints,
       false,
-      {}
+      ''
     )
     expect(experimentsByBranch.size).toStrictEqual(1)
   })
 
-  it('should find three checkpoints on the tip', async () => {
-    const { checkpointsByTip } = await collectExperiments(
+  it('should find three checkpoints on the tip', () => {
+    const { checkpointsByTip } = collectExperiments(
       repoWithNestedCheckpoints,
       false,
-      {}
+      ''
     )
     const checkpoints = checkpointsByTip.get('tip1') as Experiment[]
 
     expect(checkpoints?.length).toStrictEqual(3)
   })
 
-  it('should find checkpoints in the correct order', async () => {
-    const { checkpointsByTip } = await collectExperiments(
+  it('should find checkpoints in the correct order', () => {
+    const { checkpointsByTip } = collectExperiments(
       repoWithNestedCheckpoints,
       false,
-      {}
+      ''
     )
     const checkpoints = checkpointsByTip.get('tip1') as Experiment[]
     const [tip1cp1, tip1cp2, tip1cp3] = checkpoints
@@ -141,12 +153,8 @@ describe('collectExperiments', () => {
     expect(tip1cp3.id).toStrictEqual('tip1cp3')
   })
 
-  it('should handle the continuation of a modified checkpoint', async () => {
-    const { checkpointsByTip } = await collectExperiments(
-      modifiedFixture,
-      false,
-      {}
-    )
+  it('should handle the continuation of a modified checkpoint', () => {
+    const { checkpointsByTip } = collectExperiments(modifiedFixture, false, '')
 
     const modifiedCheckpointTip = checkpointsByTip
       .get('exp-01b3a')
@@ -181,7 +189,7 @@ describe('collectExperiments', () => {
     }
   })
 
-  it('should handle a checkpoint tip not having a name', async () => {
+  it('should handle a checkpoint tip not having a name', () => {
     const checkpointTipWithoutAName = '3fceabdcef3c7b97c7779f8ae0c69a5542eefaf5'
 
     const repoWithNestedCheckpoints = {
@@ -202,7 +210,7 @@ describe('collectExperiments', () => {
         }
       }
     }
-    const acc = await collectExperiments(repoWithNestedCheckpoints, false, {})
+    const acc = collectExperiments(repoWithNestedCheckpoints, false, '')
 
     const { experimentsByBranch, checkpointsByTip } = acc
     const [experiment] = experimentsByBranch.get('branchA') || []

extension/src/experiments/model/collect.ts

@@ -22,6 +22,7 @@ import { uniqueValues } from '../../util/array'
 import { RegisteredCommands } from '../../commands/external'
 import { Resource } from '../../resourceLocator'
 import { shortenForLabel } from '../../util/string'
+import { COMMITS_SEPARATOR } from '../../cli/git/constants'
 
 export type ExperimentItem = {
   command?: {
@@ -316,22 +317,32 @@ const formatCommitMessage = (commit: string) => {
   return `${lines[0]}${lines.length > 1 ? ' ...' : ''}`
 }
 
+const getCommitMessages = (commitsOutput: string) => {
+  const commits = commitsOutput.split(COMMITS_SEPARATOR).map(commit => {
+    const [sha, ...splitMessage] = commit.split(' ')
+    return [sha, splitMessage.join(' ')]
+  })
+  return Object.fromEntries(commits)
+}
+
 const addCommitDataToBranches = (
   branches: Experiment[],
-  commitMessages: { [sha: string]: string } = {}
-): Experiment[] =>
-  branches.map(branch => {
+  commitsOutput: string
+): Experiment[] => {
+  const commitMessages = getCommitMessages(commitsOutput)
+  return branches.map(branch => {
     const { sha } = branch
     if (sha && commitMessages[sha]) {
       branch.displayNameOrParent = formatCommitMessage(commitMessages[sha])
     }
     return branch
   })
+}
 
 export const collectExperiments = (
   data: ExperimentsOutput,
   dvcLiveOnly: boolean,
-  commitMessages: { [sha: string]: string }
+  commitsOutput: string
 ): ExperimentsAccumulator => {
   const { workspace, ...branchesObject } = data
 
@@ -350,7 +361,7 @@ export const collectExperiments = (
 
   collectFromBranchesObject(acc, branchesObject)
 
-  acc.branches = addCommitDataToBranches(acc.branches, commitMessages)
+  acc.branches = addCommitDataToBranches(acc.branches, commitsOutput)
 
   return acc
 }