Merge pull request #12357 from desktop/git-reorder-commits

Initial support for the git-based reorder implementation

app/src/lib/git/reorder.ts

@@ -0,0 +1,155 @@
+import * as FSE from 'fs-extra'
+import { getCommits, revRange } from '.'
+import { Commit } from '../../models/commit'
+import { MultiCommitOperationKind } from '../../models/multi-commit-operation'
+import { IMultiCommitOperationProgress } from '../../models/progress'
+import { Repository } from '../../models/repository'
+import { getTempFilePath } from '../file-system'
+import { rebaseInteractive, RebaseResult } from './rebase'
+
+/**
+ * Reorders provided commits by calling interactive rebase.
+ *
+ * Goal is to replay the commits in order from oldest to newest to reduce
+ * conflicts with toMove commits placed in the log at the location of the
+ * prior to the base commit.
+ *
+ * Example: A user's history from oldest to newest is A, B, C, D, E and they
+ * want to move A and E (toMove) before C. Our goal:  B, A, E, C, D. Thus,
+ * maintaining that A came before E, placed in history before the the base
+ * commit C.
+ *
+ * @param toMove - commits to move
+ * @param beforeCommit  - commits will be moved right before this commit. If it's
+ * null, the commits will be moved to the end of the history.
+ * @param lastRetainedCommitRef - sha of commit before commits to reorder or null
+ * if base commit for reordering is the root (first in history) of the branch
+ */
+export async function reorder(
+  repository: Repository,
+  toMove: ReadonlyArray<Commit>,
+  beforeCommit: Commit | null,
+  lastRetainedCommitRef: string | null,
+  progressCallback?: (progress: IMultiCommitOperationProgress) => void
+): Promise<RebaseResult> {
+  let todoPath
+  let result: RebaseResult
+
+  try {
+    if (toMove.length === 0) {
+      throw new Error('[reorder] No commits provided to reorder.')
+    }
+
+    const toMoveShas = new Set(toMove.map(c => c.sha))
+
+    const commits = await getCommits(
+      repository,
+      lastRetainedCommitRef === null
+        ? undefined
+        : revRange(lastRetainedCommitRef, 'HEAD')
+    )
+
+    if (commits.length === 0) {
+      throw new Error(
+        '[reorder] Could not find commits in log for last retained commit ref.'
+      )
+    }
+
+    todoPath = await getTempFilePath('reorderTodo')
+    let foundBaseCommitInLog = false
+    const toReplayBeforeBaseCommit = []
+    const toReplayAfterReorder = []
+
+    // Traversed in reverse so we do oldest to newest (replay commits)
+    for (let i = commits.length - 1; i >= 0; i--) {
+      const commit = commits[i]
+      if (toMoveShas.has(commit.sha)) {
+        // If it is toMove commit and we have found the base commit, we
+        // can go ahead and insert them (as we will hold any picks till after)
+        if (foundBaseCommitInLog) {
+          await FSE.appendFile(
+            todoPath,
+            `pick ${commit.sha} ${commit.summary}\n`
+          )
+        } else {
+          // However, if we have not found the base commit yet we want to
+          // keep track of them in the order of the log. Thus, we use a new
+          // `toReplayBeforeBaseCommit` array and not trust that what was sent is in the
+          // order of the log.
+          toReplayBeforeBaseCommit.push(commit)
+        }
+
+        continue
+      }
+
+      // If it's the base commit, replay to the toMove in the order they
+      // appeared on the log to reduce potential conflicts.
+      if (beforeCommit !== null && commit.sha === beforeCommit.sha) {
+        foundBaseCommitInLog = true
+        toReplayBeforeBaseCommit.unshift(commit)
+
+        for (let j = 0; j < toReplayBeforeBaseCommit.length; j++) {
+          await FSE.appendFile(
+            todoPath,
+            `pick ${toReplayBeforeBaseCommit[j].sha} ${toReplayBeforeBaseCommit[j].summary}\n`
+          )
+        }
+
+        continue
+      }
+
+      // We can't just replay a pick in case there is a commit from the toMove
+      // commits further up in history that need to be moved. Thus, we will keep
+      // track of these and replay after traversing the remainder of the log.
+      if (foundBaseCommitInLog) {
+        toReplayAfterReorder.push(commit)
+        continue
+      }
+
+      // If it is not one toMove nor the base commit and have not found the base
+      // commit, we simply record it is an unchanged pick (before the base commit)
+      await FSE.appendFile(todoPath, `pick ${commit.sha} ${commit.summary}\n`)
+    }
+
+    if (toReplayAfterReorder.length > 0) {
+      for (let i = 0; i < toReplayAfterReorder.length; i++) {
+        await FSE.appendFile(
+          todoPath,
+          `pick ${toReplayAfterReorder[i].sha} ${toReplayAfterReorder[i].summary}\n`
+        )
+      }
+    }
+
+    if (beforeCommit === null) {
+      for (let i = 0; i < toReplayBeforeBaseCommit.length; i++) {
+        await FSE.appendFile(
+          todoPath,
+          `pick ${toReplayBeforeBaseCommit[i].sha} ${toReplayBeforeBaseCommit[i].summary}\n`
+        )
+      }
+    } else if (!foundBaseCommitInLog) {
+      throw new Error(
+        '[reorder] The base commit onto was not in the log. Continuing would result in dropping the commits in the toMove array.'
+      )
+    }
+
+    result = await rebaseInteractive(
+      repository,
+      todoPath,
+      lastRetainedCommitRef,
+      MultiCommitOperationKind.Reorder,
+      undefined,
+      progressCallback,
+      commits
+    )
+  } catch (e) {
+    log.error(e)
+    return RebaseResult.Error
+  } finally {
+    if (todoPath !== undefined) {
+      FSE.remove(todoPath)
+    }
+  }
+
+  return result
+}

app/src/models/multi-commit-operation.ts

@@ -14,6 +14,7 @@ export const enum MultiCommitOperationKind {
   Rebase = 'Rebase',
   CherryPick = 'Cherry-pick',
   Squash = 'Squash',
+  Reorder = 'Reorder',
 }
 
 /**
@@ -133,13 +134,6 @@ export type CreateBranchStep = {
 }
 
 interface IInteractiveRebaseDetails {
-  /**
-   * A commit that the interactive rebase takes place around.
-   *
-   * Example: Squashing all the 'commits' array onto the 'targetCommit'.
-   */
-  readonly targetCommit: Commit
-
   /**
    * The reference to the last retained commit on the branch during an
    * interactive rebase or null if rebasing to the root.
@@ -157,12 +151,27 @@ interface ISourceBranchDetails {
 }
 interface ISquashDetails extends IInteractiveRebaseDetails {
   readonly kind: MultiCommitOperationKind.Squash
+
+  /**
+   * A commit that the interactive rebase takes place around.
+   *
+   * Example: Squashing all the 'commits' array onto the 'targetCommit'.
+   */
+  readonly targetCommit: Commit
+
   /**
    * The commit context of the commit squashed.
    */
   readonly commitContext: ICommitContext
 }
 
+interface IReorderDetails extends IInteractiveRebaseDetails {
+  readonly kind: MultiCommitOperationKind.Reorder
+
+  /** The commit before which the commits to reorder will be placed. */
+  readonly beforeCommit: Commit | null
+}
+
 interface ICherryPickDetails extends ISourceBranchDetails {
   readonly kind: MultiCommitOperationKind.CherryPick
   /**
@@ -179,5 +188,6 @@ interface IRebaseDetails extends ISourceBranchDetails {
 
 export type MultiCommitOperationDetail =
   | ISquashDetails
+  | IReorderDetails
   | ICherryPickDetails
   | IRebaseDetails

app/src/ui/multi-commit-operation/multi-commit-operation.tsx

@@ -32,6 +32,8 @@ export class MultiCommitOperation extends React.Component<
             openRepositoryInShell={this.props.openRepositoryInShell}
           />
         )
+      case MultiCommitOperationKind.Reorder:
+        return null
       default:
         return assertNever(
           kind,

app/test/unit/git/reorder-test.ts

@@ -0,0 +1,272 @@
+import * as FSE from 'fs-extra'
+import * as Path from 'path'
+import {
+  continueRebase,
+  getCommit,
+  getCommits,
+  getRebaseInternalState,
+  RebaseResult,
+} from '../../../src/lib/git'
+import { Commit } from '../../../src/models/commit'
+import { Repository } from '../../../src/models/repository'
+import { setupEmptyRepositoryDefaultMain } from '../../helpers/repositories'
+import { makeCommit } from '../../helpers/repository-scaffolding'
+import { GitProcess } from 'dugite'
+import { getStatusOrThrow } from '../../helpers/status'
+import { getTempFilePath } from '../../../src/lib/file-system'
+import { reorder } from '../../../src/lib/git/reorder'
+
+describe('git/reorder', () => {
+  let repository: Repository
+  let initialCommit: Commit
+
+  beforeEach(async () => {
+    repository = await setupEmptyRepositoryDefaultMain()
+    initialCommit = await makeSampleCommit(repository, 'initialize')
+  })
+
+  it('moves first commit after the second one', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+    const secondCommit = await makeSampleCommit(repository, 'second')
+
+    const result = await reorder(
+      repository,
+      [firstCommit],
+      secondCommit,
+      initialCommit.sha
+    )
+
+    expect(result).toBe(RebaseResult.CompletedWithoutError)
+
+    const log = await getCommits(repository, 'HEAD', 5)
+    expect(log.length).toBe(3)
+    expect(log[2].summary).toBe('initialize')
+    expect(log[1].summary).toBe('second')
+    expect(log[0].summary).toBe('first')
+  })
+
+  it('moves first and fourth commits after the second one respecting their order in the log', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+    const secondCommit = await makeSampleCommit(repository, 'second')
+    await makeSampleCommit(repository, 'third')
+    const fourthCommit = await makeSampleCommit(repository, 'fourth')
+
+    const result = await reorder(
+      repository,
+      [fourthCommit, firstCommit], // provided in opposite log order
+      secondCommit,
+      initialCommit.sha
+    )
+
+    expect(result).toBe(RebaseResult.CompletedWithoutError)
+
+    const log = await getCommits(repository, 'HEAD', 5)
+    expect(log.length).toBe(5)
+
+    const summaries = log.map(c => c.summary)
+    expect(summaries).toStrictEqual([
+      'third',
+      'fourth',
+      'first',
+      'second',
+      'initialize',
+    ])
+  })
+
+  it('moves first commit after the last one', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+    await makeSampleCommit(repository, 'second')
+    await makeSampleCommit(repository, 'third')
+    await makeSampleCommit(repository, 'last')
+
+    const result = await reorder(
+      repository,
+      [firstCommit],
+      null,
+      initialCommit.sha
+    )
+
+    expect(result).toBe(RebaseResult.CompletedWithoutError)
+
+    const log = await getCommits(repository, 'HEAD', 5)
+    const summaries = log.map(c => c.summary)
+    expect(summaries).toStrictEqual([
+      'first',
+      'last',
+      'third',
+      'second',
+      'initialize',
+    ])
+  })
+
+  it('reorders using the root of the branch if last retained commit is null', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+    await makeSampleCommit(repository, 'second')
+
+    const result = await reorder(repository, [firstCommit], initialCommit, null)
+
+    expect(result).toBe(RebaseResult.CompletedWithoutError)
+
+    const log = await getCommits(repository, 'HEAD', 5)
+    expect(log.length).toBe(3)
+
+    const summaries = log.map(c => c.summary)
+    expect(summaries).toStrictEqual(['second', 'first', 'initialize'])
+  })
+
+  it('handles reordering a conflicting commit', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+
+    // make a commit with a commit message 'second' and adding file 'second.md'
+    await makeSampleCommit(repository, 'second')
+
+    // make a third commit modifying 'second.md' from secondCommit
+    const thirdCommit = await makeSampleCommit(repository, 'third', 'second')
+
+    // move third commit after first commit
+    // Will cause a conflict due to modifications to 'second.md'  - a file that
+    // does not exist in the first commit.
+    const result = await reorder(
+      repository,
+      [thirdCommit],
+      firstCommit,
+      initialCommit.sha
+    )
+
+    expect(result).toBe(RebaseResult.ConflictsEncountered)
+
+    let status = await getStatusOrThrow(repository)
+    let { files } = status.workingDirectory
+
+    // resolve conflicts by adding the conflicting file
+    await GitProcess.exec(
+      ['add', Path.join(repository.path, 'second.md')],
+      repository.path
+    )
+
+    // If there are conflicts, we need to resend in git editor for changing the
+    // git message on continue
+    const thirdMessagePath = await getTempFilePath('reorderCommitMessage-third')
+    await FSE.writeFile(thirdMessagePath, 'third - fixed')
+
+    // continue rebase
+    let continueResult = await continueRebase(
+      repository,
+      files,
+      undefined,
+      undefined,
+      `cat "${thirdMessagePath}" >`
+    )
+
+    // This will now conflict with the 'third' commit since it is going to now
+    // apply the 'second' commit which now modifies the same lines in the
+    // 'second.md' that the previous commit does.
+    expect(continueResult).toBe(RebaseResult.ConflictsEncountered)
+
+    status = await getStatusOrThrow(repository)
+    files = status.workingDirectory.files
+
+    await FSE.writeFile(
+      Path.join(repository.path, 'second.md'),
+      '# resolve conflict from putting "third" before "second"'
+    )
+
+    const secondMessagePath = await getTempFilePath(
+      'reorderCommitMessage-second'
+    )
+    await FSE.writeFile(secondMessagePath, 'second - fixed')
+
+    continueResult = await continueRebase(
+      repository,
+      files,
+      undefined,
+      undefined,
+      `cat "${secondMessagePath}" >`
+    )
+    expect(continueResult).toBe(RebaseResult.CompletedWithoutError)
+
+    const log = await getCommits(repository, 'HEAD', 5)
+    const summaries = log.map(c => c.summary)
+    expect(summaries).toStrictEqual([
+      'second - fixed',
+      'third - fixed',
+      'first',
+      'initialize',
+    ])
+  })
+
+  it('returns error on invalid lastRetainedCommitRef', async () => {
+    const firstCommit = await makeSampleCommit(repository, 'first')
+    const secondCommit = await makeSampleCommit(repository, 'second')
+
+    const result = await reorder(
+      repository,
+      [secondCommit],
+      firstCommit,
+      'INVALID INVALID'
+    )
+
+    expect(result).toBe(RebaseResult.Error)
+
+    // Rebase will not start - As it won't be able retrieve a commits to build a
+    // todo and then interactive rebase would fail for bad revision. Added logic
+    // to short circuit to prevent unnecessary attempt at an interactive rebase.
+    const isRebaseStillOngoing = await getRebaseInternalState(repository)
+    expect(isRebaseStillOngoing).toBeNull()
+  })
+
+  it('returns error on invalid base commit', async () => {
+    await makeSampleCommit(repository, 'first')
+    const secondCommit = await makeSampleCommit(repository, 'second')
+
+    const badCommit = { ...secondCommit, sha: 'INVALID', summary: 'INVALID' }
+    const result = await reorder(
+      repository,
+      [secondCommit],
+      badCommit,
+      initialCommit.sha
+    )
+
+    expect(result).toBe(RebaseResult.Error)
+
+    // Rebase should not start - if we did attempt this, it could result in
+    // dropping commits.
+    const isRebaseStillOngoing = await getRebaseInternalState(repository)
+    expect(isRebaseStillOngoing).toBeNull()
+  })
+
+  it('returns error when no commits are reordered', async () => {
+    const first = await makeSampleCommit(repository, 'first')
+    await makeSampleCommit(repository, 'second')
+
+    const result = await reorder(repository, [], first, initialCommit.sha)
+
+    expect(result).toBe(RebaseResult.Error)
+
+    // Rebase should not start - technically there would be no harm in this
+    // rebase as it would just replay history, but we should not use reorder to
+    // replay history.
+    const isRebaseStillOngoing = await getRebaseInternalState(repository)
+    expect(isRebaseStillOngoing).toBeNull()
+  })
+})
+
+async function makeSampleCommit(
+  repository: Repository,
+  desc: string,
+  file?: string
+): Promise<Commit> {
+  file = file || desc
+  const commitTree = {
+    commitMessage: desc,
+    entries: [
+      {
+        path: file + '.md',
+        contents: '# ' + desc + ' \n',
+      },
+    ],
+  }
+  await makeCommit(repository, commitTree)
+
+  return (await getCommit(repository, 'HEAD'))!
+}