chore(push): improve schema design of PushResult (#1036)

billiegoose · web-flow · commit 08ce40e5eff4 · 2020-02-09T16:46:09.000-05:00
BREAKING CHANGE: the `push` function now throws if any of the refs on the remote were not updated successfully. It also returns a nicely typed result object organized by ref, rather than a loose collection of strings organized by outcome.
diff --git a/__tests__/test-config.js b/__tests__/test-config.js
@@ -19,8 +19,7 @@ describe('config', () => {
     const fetches = await getConfigAll({
       fs,
       gitdir,
-      path: 'remote.upstream.fetch',
-      all: true
+      path: 'remote.upstream.fetch'
     })
     expect(sym).toBe(false)
     expect(url).toBe('https://github.com/isomorphic-git/isomorphic-git')
diff --git a/__tests__/test-hosting-providers.js b/__tests__/test-hosting-providers.js
@@ -60,9 +60,8 @@ describe('Hosting Providers', () => {
         force: true
       })
       expect(res).toBeTruthy()
-      expect(res.ok).toBeTruthy()
-      expect(res.ok[0]).toBe('unpack')
-      expect(res.ok[1]).toBe('refs/heads/master')
+      expect(res.ok).toBe(true)
+      expect(res.refs['refs/heads/master'].ok).toBe(true)
     })
   })
   describe('Azure DevOps', () => {
@@ -114,9 +113,8 @@ describe('Hosting Providers', () => {
         force: true
       })
       expect(res).toBeTruthy()
-      expect(res.ok).toBeTruthy()
-      expect(res.ok[0]).toBe('unpack')
-      expect(res.ok[1]).toBe('refs/heads/master')
+      expect(res.ok).toBe(true)
+      expect(res.refs['refs/heads/master'].ok).toBe(true)
     })
   })
   describe('Bitbucket', () => {
@@ -144,9 +142,8 @@ describe('Hosting Providers', () => {
         force: true
       })
       expect(res).toBeTruthy()
-      expect(res.ok).toBeTruthy()
-      expect(res.ok[0]).toBe('unpack')
-      expect(res.ok[1]).toBe('refs/heads/master')
+      expect(res.ok).toBe(true)
+      expect(res.refs['refs/heads/master'].ok).toBe(true)
     })
     it('fetch', async () => {
       // This App Password is for the test account 'isomorphic-git' user on Bitbucket,
@@ -224,9 +221,8 @@ describe('Hosting Providers', () => {
         force: true
       })
       expect(res).toBeTruthy()
-      expect(res.ok).toBeTruthy()
-      expect(res.ok[0]).toBe('unpack')
-      expect(res.ok[1]).toBe('refs/heads/master')
+      expect(res.ok).toBe(true)
+      expect(res.refs['refs/heads/master'].ok).toBe(true)
     })
   })
   describe('GitLab', () => {
@@ -280,9 +276,8 @@ describe('Hosting Providers', () => {
         force: true
       })
       expect(res).toBeTruthy()
-      expect(res.ok).toBeTruthy()
-      expect(res.ok[0]).toBe('unpack')
-      expect(res.ok[1]).toBe('refs/heads/master')
+      expect(res.ok).toBe(true)
+      expect(res.refs['refs/heads/master'].ok).toBe(true)
     })
   })
 })
diff --git a/__tests__/test-merge.js b/__tests__/test-merge.js
@@ -304,10 +304,10 @@ describe('merge', () => {
       depth: 1
     }))[0]
     expect(notMergeCommit.oid).toEqual(originalCommit.oid)
+    if (!report.oid) throw new Error('type error')
     // make sure no commit object was created
     expect(
       await fs.exists(
-        // @ts-ignore
         `${gitdir}/objects/${report.oid.slice(0, 2)}/${report.oid.slice(2)}`
       )
     ).toBe(false)
@@ -351,10 +351,10 @@ describe('merge', () => {
       depth: 1
     }))[0]
     expect(notMergeCommit.oid).toEqual(originalCommit.oid)
+    if (!report.oid) throw new Error('type error')
     // but make sure the commit object exists
     expect(
       await fs.exists(
-        // @ts-ignore
         `${gitdir}/objects/${report.oid.slice(0, 2)}/${report.oid.slice(2)}`
       )
     ).toBe(true)
diff --git a/__tests__/test-push.js b/__tests__/test-push.js
@@ -37,9 +37,8 @@ describe('push', () => {
       ref: 'refs/heads/master'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/master')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/master'].ok).toBe(true)
     expect(output).toMatchSnapshot()
   })
   it('push without ref', async () => {
@@ -59,9 +58,8 @@ describe('push', () => {
       remote: 'karma'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/master')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/master'].ok).toBe(true)
   })
   it('push with ref !== remoteRef', async () => {
     // Setup
@@ -82,9 +80,8 @@ describe('push', () => {
       remoteRef: 'foobar'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/foobar')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/foobar'].ok).toBe(true)
     expect(await listBranches({ fs, gitdir, remote: 'karma' })).toContain(
       'foobar'
     )
@@ -107,9 +104,8 @@ describe('push', () => {
       ref: 'lightweight-tag'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/tags/lightweight-tag')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/tags/lightweight-tag'].ok).toBe(true)
   })
   it('push with annotated tag', async () => {
     // Setup
@@ -129,9 +125,8 @@ describe('push', () => {
       ref: 'annotated-tag'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/tags/annotated-tag')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/tags/annotated-tag'].ok).toBe(true)
   })
   it('push delete', async () => {
     // Setup
@@ -163,9 +158,8 @@ describe('push', () => {
       delete: true
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/foobar')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/foobar'].ok).toBe(true)
     expect(await listBranches({ fs, gitdir, remote: 'karma' })).not.toContain(
       'foobar'
     )
@@ -191,9 +185,8 @@ describe('push', () => {
       ref: 'master'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/master')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/master'].ok).toBe(true)
   })
   it('push with Basic Auth credentials in the URL', async () => {
     // Setup
@@ -213,9 +206,8 @@ describe('push', () => {
       ref: 'master'
     })
     expect(res).toBeTruthy()
-    expect(res.ok).toBeTruthy()
-    expect(res.ok[0]).toBe('unpack')
-    expect(res.ok[1]).toBe('refs/heads/master')
+    expect(res.ok).toBe(true)
+    expect(res.refs['refs/heads/master'].ok).toBe(true)
   })
   it('throws an Error if no credentials supplied', async () => {
     // Setup
diff --git a/cli.js b/cli.js
@@ -2,13 +2,16 @@
 const fs = require('fs')
 const minimisted = require('minimisted')
 const git = require('.')
+const { http } = require('./dist/http.cjs')
 
 // This really isn't much of a CLI. It's mostly for testing.
 // But it's very versatile and works surprisingly well.
 
 minimisted(async function ({ _: [command, ...args], ...opts }) {
   try {
-    const result = await git[command](Object.assign({ fs, dir: '.' }, opts))
+    const result = await git[command](
+      Object.assign({ fs, http, dir: '.' }, opts)
+    )
     if (result === undefined) return
     // detect streams
     if (typeof result.on === 'function') {
diff --git a/package-scripts.js b/package-scripts.js
@@ -93,6 +93,7 @@ module.exports = {
         ? series.nps(
           'lint',
           'build',
+          'lint.typescriptTests',
           'test.setup',
           'test.jest',
           'test.karma',
@@ -101,6 +102,7 @@ module.exports = {
         : series.nps(
           'lint',
           'build',
+          'lint.typescriptTests',
           'test.setup',
           'test.jest',
           'test.karma',
diff --git a/src/api/addNote.js b/src/api/addNote.js
@@ -2,9 +2,8 @@
 import '../commands/typedefs.js'
 
 import { addNote as _addNote } from '../commands/addNote.js'
-import { E } from '../index.js'
 import { FileSystem } from '../models/FileSystem.js'
-import { GitError } from '../models/GitError.js'
+import { E, GitError } from '../models/GitError.js'
 import { assertParameter } from '../utils/assertParameter.js'
 import { join } from '../utils/join'
 import { normalizeAuthorObject } from '../utils/normalizeAuthorObject.js'
diff --git a/src/api/push.js b/src/api/push.js
@@ -6,15 +6,6 @@ import { FileSystem } from '../models/FileSystem.js'
 import { assertParameter } from '../utils/assertParameter.js'
 import { join } from '../utils/join.js'
 
-/**
- *
- * @typedef {Object} PushResult - Returns an object with a schema like this:
- * @property {string[]} [ok]
- * @property {string[]} [errors]
- * @property {object} [headers]
- *
- */
-
 /**
  * Push a branch or tag
  *
@@ -52,6 +43,7 @@ import { join } from '../utils/join.js'
  *
  * @returns {Promise<PushResult>} Resolves successfully when push completes with a detailed description of the operation from the server.
  * @see PushResult
+ * @see RefUpdateStatus
  *
  * @example
  * let pushResult = await git.push({
diff --git a/src/commands/push.js b/src/commands/push.js
@@ -19,15 +19,6 @@ import { splitLines } from '../utils/splitLines.js'
 import { parseReceivePackResponse } from '../wire/parseReceivePackResponse.js'
 import { writeReceivePackRequest } from '../wire/writeReceivePackRequest.js'
 
-/**
- *
- * @typedef {Object} PushResult - Returns an object with a schema like this:
- * @property {string[]} [ok]
- * @property {string[]} [errors]
- * @property {object} [headers]
- *
- */
-
 /**
  * @param {object} args
  * @param {import('../models/FileSystem.js').FileSystem} args.fs
@@ -52,18 +43,7 @@ import { writeReceivePackRequest } from '../wire/writeReceivePackRequest.js'
  * @param {'github' | 'bitbucket' | 'gitlab'} [args.oauth2format]
  * @param {Object<string, string>} [args.headers]
  *
- * @returns {Promise<PushResult>} Resolves successfully when push completes with a detailed description of the operation from the server.
- * @see PushResult
- *
- * @example
- * let pushResult = await git.push({
- *   dir: '$input((/))',
- *   remote: '$input((origin))',
- *   ref: '$input((master))',
- *   token: $input((process.env.GITHUB_TOKEN)),
- * })
- * console.log(pushResult)
- *
+ * @returns {Promise<PushResult>}
  */
 export async function push ({
   fs,
@@ -248,8 +228,8 @@ export async function push ({
   }
 
   // Update the local copy of the remote ref
-  // TODO: I think this should actually be using a refspec transform rather than assuming 'refs/remotes/{remote}'
-  if (remote && result.ok && result.ok.includes(fullRemoteRef)) {
+  if (remote && result.ok && result.refs[fullRemoteRef].ok) {
+    // TODO: I think this should actually be using a refspec transform rather than assuming 'refs/remotes/{remote}'
     const ref = `refs/remotes/${remote}/${fullRemoteRef.replace(
       'refs/heads',
       ''
@@ -260,5 +240,13 @@ export async function push ({
       await GitRefManager.writeRef({ fs, gitdir, ref, value: oid })
     }
   }
-  return result
+  if (result.ok && Object.values(result.refs).every(result => result.ok)) {
+    return result
+  } else {
+    const prettyDetails = Object.entries(result.refs)
+      .filter(([k, v]) => !v.ok)
+      .map(([k, v]) => `\n  - ${k}: ${v.error}`)
+      .join('')
+    throw new GitError(E.GitPushError, Object.assign({ prettyDetails }, result))
+  }
 }
diff --git a/src/commands/typedefs.js b/src/commands/typedefs.js
@@ -275,3 +275,21 @@
  * @param {IterableIterator<WalkerEntry[]>} children
  * @returns {Promise<any[]>}
  */
+
+/**
+ *
+ * @typedef {Object} RefUpdateStatus
+ * @property {boolean} ok
+ * @property {string} error
+ *
+ */
+
+/**
+ *
+ * @typedef {Object} PushResult
+ * @property {boolean} ok
+ * @property {?string} error
+ * @property {Object<string, RefUpdateStatus>} refs
+ * @property {Object<string, string>} [headers]
+ *
+ */
diff --git a/src/models/GitError.js b/src/models/GitError.js
@@ -73,7 +73,8 @@ const messages = {
   AmbiguousShortOid: `Found multiple oids matching "{ short }" ({ matches }). Use a longer abbreviation length to disambiguate them.`,
   ShortOidNotFound: `Could not find an object matching "{ short }".`,
   CheckoutConflictError: `Your local changes to the following files would be overwritten by checkout: { filepaths }`,
-  NoteAlreadyExistsError: `A note object { note } already exists on object { oid }. Use 'force: true' parameter to overwrite existing notes.`
+  NoteAlreadyExistsError: `A note object { note } already exists on object { oid }. Use 'force: true' parameter to overwrite existing notes.`,
+  GitPushError: `One or more branches were not updated: { prettyDetails }`
 }
 
 export const E = {
@@ -220,7 +221,9 @@ export const E = {
   /** @type {'CheckoutConflictError'} */
   CheckoutConflictError: `CheckoutConflictError`,
   /** @type {'NoteAlreadyExistsError'} */
-  NoteAlreadyExistsError: `NoteAlreadyExistsError`
+  NoteAlreadyExistsError: `NoteAlreadyExistsError`,
+  /** @type {'GitPushError'} */
+  GitPushError: `GitPushError`
 }
 
 function renderTemplate (template, values) {
diff --git a/src/wire/parseReceivePackResponse.js b/src/wire/parseReceivePackResponse.js
