Repository: isomorphic-git/isomorphic-git
Pull request #1036 (2020-02-09 20:22:42)

chore(push): improve schema design of PushResult
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.

Bonus: the entire `__tests__` folder now type-checks! And will continue to do so because I added it to the linting step.

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 96d893ef74a3dcdd6a435a1495b9b9d9290b7697:
| Browser                                 | Pass | Skip | Fail | Time   | Disconnect? |
| --------------------------------------- | ---- | ---- | ---- | ------ | ----------- |
| Chrome 79.0.3945 (Windows 10 0.0.0)     | 360  | 1    | 0    | 3m:17s | false       |
| Chrome Mobile 78.0.3904 (Android 0.0.0) | 360  | 1    | 0    | 1m:48s | false       |
| Firefox 72.0.0 (Ubuntu 0.0.0)           | 360  | 1    | 0    | 1m:24s | false       |
| HeadlessChrome 0.0.0 (Linux 0.0.0)      | 360  | 1    | 0    | 0m:48s | false       |
| Mobile Safari 13.0.0 (iOS 13.0.0)       | 360  | 1    | 0    | 2m:40s | false       |
| Safari 13.0.3 (Mac OS X 10.15.1)        | 360  | 1    | 0    | 3m:5s  | false       |

### Longest running tests
- 12.002 secs - fetch shallow fetch relative (from Github) - _Firefox_
- 10.26 secs - fetch shallow fetch (from Github) - _Firefox_
- 9.875 secs - GitPackIndex to .idx file from .pack - _Firefox_
- 9.153 secs - GitPackIndex from .pack - _Firefox_
- 8.226 secs - fetch fetch (from Github) - _Firefox_
- 8.137 secs - fetch shallow fetch (from Github) - _HeadlessChrome_
- 7.842 secs - fetch fetch (from Github) - _HeadlessChrome_
- 7.52 secs - fetch shallow fetch (from Github) - _Chrome Mobile_
- 7.429 secs - fetch shallow fetch relative (from Github) - _Chrome_
- 7.279 secs - fetch shallow fetch (from Github) - _Chrome_
- 7.057 secs - fetch fetch --prune-tags from git-http-mock-server - _Safari_


Patch:
diff --git a/cli.js b/cli.js
index 18f5c683..41edd26c 100755
--- 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
index 1b6aa8c6..647c10ba 100644
--- 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
index 6b26ebc6..844dc9a2 100644
--- 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
index ff256da2..14d675cc 100644
--- 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
index c7ad2fa3..c4376af2 100644
--- 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
index ea472561..63eb40a5 100644
--- 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
index e4d4917f..33c8b655 100644
--- 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
index 7118455b..8795a21b 100644
--- a/src/wire/parseReceivePackResponse.js
+++ b/src/wire/parseReceivePackResponse.js
@@ -1,7 +1,10 @@
+import '../commands/typedefs.js'
+
 import { E, GitError } from '../models/GitError.js'
 import { GitPktLine } from '../models/GitPktLine.js'
 
 export async function parseReceivePackResponse (packfile) {
+  /** @type PushResult */
   const result = {}
   let response = ''
   const read = GitPktLine.streamReader(packfile)
@@ -17,20 +20,22 @@ export async function parseReceivePackResponse (packfile) {
   if (!line.startsWith('unpack ')) {
     throw new GitError(E.UnparseableServerResponseFail, { line })
   }
-  if (line === 'unpack ok') {
-    result.ok = ['unpack']
-  } else {
-    result.errors = [line.trim()]
+  result.ok = line === 'unpack ok'
+  if (!result.ok) {
+    result.error = line.slice('unpack '.length)
   }
+  result.refs = {}
   for (const line of lines) {
+    if (line.trim() === '') continue
     const status = line.slice(0, 2)
     const refAndMessage = line.slice(3)
-    if (status === 'ok') {
-      result.ok = result.ok || []
-      result.ok.push(refAndMessage)
-    } else if (status === 'ng') {
-      result.errors = result.errors || []
-      result.errors.push(refAndMessage)
+    let space = refAndMessage.indexOf(' ')
+    if (space === -1) space = refAndMessage.length
+    const ref = refAndMessage.slice(0, space)
+    const error = refAndMessage.slice(space + 1)
+    result.refs[ref] = {
+      ok: status === 'ok',
+      error
     }
   }
   return result


Test patch:
diff --git a/__tests__/test-config.js b/__tests__/test-config.js
index 4cb137e3..9051b759 100644
--- 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
index ae72bb10..7b405ea5 100644
--- 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
index daabf526..4a1cb726 100644
--- 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
index 088a1f3f..2aa5c9a9 100644
--- 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