Repository: isomorphic-git/isomorphic-git
Pull request #843 (2019-09-07 20:28:52)

feat: Added 'dryRun' to commit and 'noUpdateBranch' to merge
## I'm adding a parameter to an existing command:

- [x] add parameter to the function in `src/commands/X.js`
- [x] document the parameter in the JSDoc comment above the function
- [x] add parameter to the TypeScript library definition for X in `src/index.d.ts`
- [x] add a test case in `__tests__/test-X.js` if possible
- [ ] squash merge the PR with commit message "feat: Added 'bar' parameter to X command"


isomorphic-git-bot[bot]: Thank you wmhilton! I auto-formatted the code using [prettier-standard](https://github.com/sheerun/prettier-standard). :robot:
karma-pr-reporter: ## Test Results for dc7df723d1f30a57a9f3348ae817153190b35803:
| Browser | Passed | Skipped | Failed | Time | Disconnected | Error |
|---------|--------|---------|--------|------|--------------|-------|
| HeadlessChrome 0.0.0 (Linux 0.0.0) | 295 | 1 | 0 | 1 min 44.83 secs | false | undefined |
| Firefox 68.0.0 (Ubuntu 0.0.0) | 295 | 1 | 0 | 2 mins 27.047 secs | false | undefined |
| Chrome Mobile 67.0.3396 (Android 7.1.1) | 295 | 1 | 0 | 4 mins 32.864 secs | false | undefined |
| Safari 11.1.2 (Mac OS X 10.13.6) | 295 | 1 | 0 | 4 mins 37.137 secs | false | undefined |
| Mobile Safari 11.0.0 (iOS 11.2.0) | 295 | 1 | 0 | 5 mins 10.699 secs | false | undefined |
| Edge 17.17134.0 (Windows 10 0.0.0) | 295 | 1 | 0 | 7 mins 12.681 secs | false | undefined |
karma-pr-reporter: ## Test Results for be1c7ba0c388bf419efaf22f4d50b2bec9637b36:
| Browser | Passed | Skipped | Failed | Time | Disconnected | Error |
|---------|--------|---------|--------|------|--------------|-------|
| HeadlessChrome 0.0.0 (Linux 0.0.0) | 295 | 1 | 0 | 1 min 32.148 secs | false | undefined |
| Firefox 68.0.0 (Ubuntu 0.0.0) | 295 | 1 | 0 | 2 mins 14.754 secs | false | undefined |
| Safari 11.1.2 (Mac OS X 10.13.6) | 295 | 1 | 0 | 4 mins 17.459 secs | false | undefined |
| Chrome Mobile 67.0.3396 (Android 7.1.1) | 295 | 1 | 0 | 4 mins 47.365 secs | false | undefined |
| Mobile Safari 11.0.0 (iOS 11.2.0) | 295 | 1 | 0 | 5 mins 1.112 secs | false | undefined |
| Edge 17.17134.0 (Windows 10 0.0.0) | 295 | 1 | 0 | 6 mins 51.054 secs | false | undefined |

Patch:
diff --git a/src/commands/commit.js b/src/commands/commit.js
index 1f7f78ee..ae934c6b 100644
--- a/src/commands/commit.js
+++ b/src/commands/commit.js
@@ -28,6 +28,7 @@ import { cores } from '../utils/plugins.js'
  * @param {string} [args.author.timezoneOffset] - Set the author timezone offset field. This is the difference, in minutes, from the current timezone to UTC. Default is `(new Date()).getTimezoneOffset()`.
  * @param {Object} [args.committer = author] - The details about the commit committer, in the same format as the author parameter. If not specified, the author details are used.
  * @param {string} [args.signingKey] - Sign the tag object using this private PGP key.
+ * @param {boolean} [args.dryRun = false] - If true, simulates making a commit so you can test whether it would succeed. Implies `noUpdateBranch`.
  * @param {boolean} [args.noUpdateBranch = false] - If true, does not update the branch pointer after creating the commit.
  * @param {string} [args.ref] - The fully expanded name of the branch to commit to. Default is the current branch pointed to by HEAD. (TODO: fix it so it can expand branch names without throwing if the branch doesn't exist yet.)
  * @param {string[]} [args.parent] - The SHA-1 object ids of the commits to use as parents. If not specified, the commit pointed to by `ref` is used.
@@ -56,6 +57,7 @@ export async function commit ({
   author,
   committer,
   signingKey,
+  dryRun = false,
   noUpdateBranch = false,
   ref,
   parent,
@@ -101,7 +103,7 @@ export async function commit ({
         const inodes = flatFileListToDirectoryStructure(index.entries)
         const inode = inodes.get('.')
         if (!tree) {
-          tree = await constructTree({ fs, gitdir, inode })
+          tree = await constructTree({ fs, gitdir, inode, dryRun })
         }
         if (!parent) {
           try {
@@ -132,9 +134,10 @@ export async function commit ({
           fs,
           gitdir,
           type: 'commit',
-          object: comm.toObject()
+          object: comm.toObject(),
+          dryRun
         })
-        if (!noUpdateBranch) {
+        if (!noUpdateBranch && !dryRun) {
           // Update branch pointer
           await GitRefManager.writeRef({
             fs,
@@ -152,13 +155,13 @@ export async function commit ({
   }
 }
 
-async function constructTree ({ fs, gitdir, inode }) {
+async function constructTree ({ fs, gitdir, inode, dryRun }) {
   // use depth first traversal
   const children = inode.children
   for (const inode of children) {
     if (inode.type === 'tree') {
       inode.metadata.mode = '040000'
-      inode.metadata.oid = await constructTree({ fs, gitdir, inode })
+      inode.metadata.oid = await constructTree({ fs, gitdir, inode, dryRun })
     }
   }
   const entries = children.map(inode => ({
@@ -172,7 +175,8 @@ async function constructTree ({ fs, gitdir, inode }) {
     fs,
     gitdir,
     type: 'tree',
-    object: tree.toObject()
+    object: tree.toObject(),
+    dryRun
   })
   return oid
 }
diff --git a/src/commands/merge.js b/src/commands/merge.js
index 1aa3e7b6..51c9c88f 100644
--- a/src/commands/merge.js
+++ b/src/commands/merge.js
@@ -24,7 +24,16 @@ import { findMergeBase } from './findMergeBase.js'
  */
 
 /**
- * Merge one or more branches *(Currently, only very simple cases are handled.)*
+ * Merge two branches
+ *
+ * ## Limitations
+ *
+ * Currently it does not support incomplete merges. That is, if there are merge conflicts it cannot solve
+ * with the built in diff3 algorithm it will not modify the working dir, and will throw a [`MergeNotSupportedFail`](./errors.md#mergenotsupportedfail) error.
+ *
+ * Currently it will fail if multiple candidate merge bases are found. (It doesn't yet implement the recursive merge strategy.)
+ *
+ * Currently it does not support selecting alternative merge strategies.
  *
  * @param {object} args
  * @param {string} [args.core = 'default'] - The plugin core identifier to use for plugin injection
@@ -35,6 +44,7 @@ import { findMergeBase } from './findMergeBase.js'
  * @param {string} args.theirs - The branch to be merged
  * @param {boolean} [args.fastForwardOnly = false] - If true, then non-fast-forward merges will throw an Error instead of performing a merge.
  * @param {boolean} [args.dryRun = false] - If true, simulates a merge so you can test whether it would succeed.
+ * @param {boolean} [args.noUpdateBranch = false] - If true, does not update the branch pointer after creating the commit.
  * @param {string} [args.message] - Overrides the default auto-generated merge commit message
  * @param {Object} [args.author] - passed to [commit](commit.md) when creating a merge commit
  * @param {Object} [args.committer] - passed to [commit](commit.md) when creating a merge commit
@@ -57,6 +67,7 @@ export async function merge ({
   theirs,
   fastForwardOnly = false,
   dryRun = false,
+  noUpdateBranch = false,
   message,
   author,
   committer,
@@ -107,7 +118,7 @@ export async function merge ({
       }
     }
     if (baseOid === ourOid) {
-      if (!dryRun) {
+      if (!dryRun && !noUpdateBranch) {
         await GitRefManager.writeRef({ fs, gitdir, ref: ours, value: theirOid })
       }
       return {
@@ -137,25 +148,21 @@ export async function merge ({
           ours
         )}`
       }
-      if (!dryRun) {
-        const oid = await commit({
-          fs,
-          gitdir,
-          message,
-          ref: ours,
-          tree,
-          parent: [ourOid, theirOid],
-          author,
-          committer,
-          signingKey
-        })
-        return {
-          oid,
-          tree,
-          mergeCommit: true
-        }
-      }
+      const oid = await commit({
+        fs,
+        gitdir,
+        message,
+        ref: ours,
+        tree,
+        parent: [ourOid, theirOid],
+        author,
+        committer,
+        signingKey,
+        dryRun,
+        noUpdateBranch
+      })
       return {
+        oid,
         tree,
         mergeCommit: true
       }
diff --git a/src/index.d.ts b/src/index.d.ts
index 49d96bf2..f44d7ce7 100644
--- a/src/index.d.ts
+++ b/src/index.d.ts
@@ -318,6 +318,7 @@ export function commit(args: GitDir & {
   };
   signingKey?: string;
   noUpdateBranch?: boolean;
+  dryRun?: boolean;
   ref?: string;
   parent?: string[];
   tree?: string;
@@ -489,6 +490,7 @@ export function merge(args: GitDir & {
   theirs: string;
   fastForwardOnly?: boolean;
   dryRun?: boolean;
+  noUpdateBranch?: boolean;
   message?: string;
   author?: {
     name?: string;


Test patch:
diff --git a/__tests__/test-commit.js b/__tests__/test-commit.js
index 7151d7f8..e4bd19e5 100644
--- a/__tests__/test-commit.js
+++ b/__tests__/test-commit.js
@@ -35,7 +35,7 @@ describe('commit', () => {
 
   it('without updating branch', async () => {
     // Setup
-    const { gitdir } = await makeFixture('test-commit')
+    const { fs, gitdir } = await makeFixture('test-commit')
     const { oid: originalOid } = (await log({ gitdir, depth: 1 }))[0]
     // Test
     const sha = await commit({
@@ -54,6 +54,41 @@ describe('commit', () => {
     const { oid: currentOid } = (await log({ gitdir, depth: 1 }))[0]
     expect(currentOid).toEqual(originalOid)
     expect(currentOid).not.toEqual(sha)
+    // but DID create commit object
+    expect(
+      await fs.exists(
+        `${gitdir}/objects/7a/51c0b1181d738198ff21c4679d3aa32eb52fe0`
+      )
+    ).toBe(true)
+  })
+
+  it('dry run', async () => {
+    // Setup
+    const { fs, gitdir } = await makeFixture('test-commit')
+    const { oid: originalOid } = (await log({ gitdir, depth: 1 }))[0]
+    // Test
+    const sha = await commit({
+      gitdir,
+      author: {
+        name: 'Mr. Test',
+        email: 'mrtest@example.com',
+        timestamp: 1262356920,
+        timezoneOffset: -0
+      },
+      message: 'Initial commit',
+      dryRun: true
+    })
+    expect(sha).toBe('7a51c0b1181d738198ff21c4679d3aa32eb52fe0')
+    // does NOT update branch pointer
+    const { oid: currentOid } = (await log({ gitdir, depth: 1 }))[0]
+    expect(currentOid).toEqual(originalOid)
+    expect(currentOid).not.toEqual(sha)
+    // and did NOT create commit object
+    expect(
+      await fs.exists(
+        `${gitdir}/objects/7a/51c0b1181d738198ff21c4679d3aa32eb52fe0`
+      )
+    ).toBe(false)
   })
 
   it('custom ref', async () => {
diff --git a/__tests__/test-merge.js b/__tests__/test-merge.js
index 6dd60317..94fdbdab 100644
--- a/__tests__/test-merge.js
+++ b/__tests__/test-merge.js
@@ -100,7 +100,36 @@ describe('merge', () => {
     expect(oid).toEqual(desiredOid)
   })
 
-  it('merge newest into master --dryRun', async () => {
+  it('merge newest into master --dryRun (no author needed since fastForward)', async () => {
+    // Setup
+    const { gitdir } = await makeFixture('test-merge')
+    // Test
+    const originalOid = await resolveRef({
+      gitdir,
+      ref: 'master'
+    })
+    const desiredOid = await resolveRef({
+      gitdir,
+      ref: 'newest'
+    })
+    const m = await merge({
+      gitdir,
+      ours: 'master',
+      theirs: 'newest',
+      fastForwardOnly: true,
+      dryRun: true
+    })
+    expect(m.oid).toEqual(desiredOid)
+    expect(m.alreadyMerged).toBeFalsy()
+    expect(m.fastForward).toBeTruthy()
+    const oid = await resolveRef({
+      gitdir,
+      ref: 'master'
+    })
+    expect(oid).toEqual(originalOid)
+  })
+
+  it('merge newest into master --noUpdateBranch', async () => {
     // Setup
     const { gitdir } = await makeFixture('test-merge')
     // Test
@@ -191,6 +220,26 @@ describe('merge', () => {
     expect(mergeCommit.parent).toEqual(commit.parent)
   })
 
+  it("merge 'delete-first-half' and 'delete-second-half' (dryRun, missing author)", async () => {
+    // Setup
+    const { fs, gitdir } = await makeFixture('test-merge')
+    // Test
+    let error = null
+    try {
+      await merge({
+        fs,
+        gitdir,
+        ours: 'delete-first-half',
+        theirs: 'delete-second-half',
+        dryRun: true
+      })
+    } catch (e) {
+      error = e
+    }
+    expect(error).not.toBe(null)
+    expect(error.code).toBe(E.MissingAuthorError)
+  })
+
   it("merge 'delete-first-half' and 'delete-second-half' (dryRun)", async () => {
     // Setup
     const { fs, gitdir } = await makeFixture('test-merge')
@@ -210,6 +259,12 @@ describe('merge', () => {
       gitdir,
       ours: 'delete-first-half',
       theirs: 'delete-second-half',
+      author: {
+        name: 'Mr. Test',
+        email: 'mrtest@example.com',
+        timestamp: 1262356920,
+        timezoneOffset: -0
+      },
       dryRun: true
     })
     expect(report.tree).toBe(commit.tree)
@@ -220,6 +275,55 @@ describe('merge', () => {
       depth: 1
     }))[0]
     expect(notMergeCommit.oid).toEqual(originalCommit.oid)
+    // make sure no commit object was created
+    expect(
+      await fs.exists(
+        `${gitdir}/objects/${report.oid.slice(0, 2)}/${report.oid.slice(2)}`
+      )
+    ).toBe(false)
+  })
+
+  it("merge 'delete-first-half' and 'delete-second-half' (noUpdateBranch)", async () => {
+    // Setup
+    const { fs, gitdir } = await makeFixture('test-merge')
+    const commit = (await log({
+      gitdir,
+      depth: 1,
+      ref: 'delete-first-half-merge-delete-second-half'
+    }))[0]
+    const originalCommit = (await log({
+      gitdir,
+      ref: 'delete-first-half',
+      depth: 1
+    }))[0]
+    // Test
+    const report = await merge({
+      fs,
+      gitdir,
+      ours: 'delete-first-half',
+      theirs: 'delete-second-half',
+      author: {
+        name: 'Mr. Test',
+        email: 'mrtest@example.com',
+        timestamp: 1262356920,
+        timezoneOffset: -0
+      },
+      noUpdateBranch: true
+    })
+    expect(report.tree).toBe(commit.tree)
+    // make sure branch hasn't been moved
+    const notMergeCommit = (await log({
+      gitdir,
+      ref: 'delete-first-half',
+      depth: 1
+    }))[0]
+    expect(notMergeCommit.oid).toEqual(originalCommit.oid)
+    // but make sure the commit object exists
+    expect(
+      await fs.exists(
+        `${gitdir}/objects/${report.oid.slice(0, 2)}/${report.oid.slice(2)}`
+      )
+    ).toBe(true)
   })
 
   it("merge 'delete-first-half' and 'delete-second-half'", async () => {