fix(git): ensure stream failures are reported

Prior to the change, race conditions could cause errors on the stream to
be lost. The symptom for npm users would be the infamous "cb() never
called" error.

PR-URL: #1
Close: #1
Reviewed-by: @isaacs

extract.js

@@ -50,21 +50,22 @@ function extract (spec, dest, opts) {
 function tryExtract (spec, tarStream, dest, opts) {
   return new BB((resolve, reject) => {
     tarStream.on('error', reject)
-    setImmediate(resolve)
+
+    rimraf(dest)
+      .then(() => mkdirp(dest))
+      .then(() => {
+        const xtractor = extractStream(spec, dest, opts)
+        xtractor.on('error', reject)
+        xtractor.on('close', resolve)
+        tarStream.pipe(xtractor)
+      })
+      .catch(reject)
   })
-    .then(() => rimraf(dest))
-    .then(() => mkdirp(dest))
-    .then(() => new BB((resolve, reject) => {
-      const xtractor = extractStream(spec, dest, opts)
-      tarStream.on('error', reject)
-      xtractor.on('error', reject)
-      xtractor.on('close', resolve)
-      tarStream.pipe(xtractor)
-    }))
     .catch(err => {
       if (err.code === 'EINTEGRITY') {
         err.message = `Verification failed while extracting ${spec}:\n${err.message}`
       }
+
       throw err
     })
 }

test/git.extract.js

@@ -0,0 +1,41 @@
+'use strict'
+
+const BB = require('bluebird')
+const fs = BB.promisifyAll(require('fs'))
+const mkdirp = BB.promisify(require('mkdirp'))
+const npmlog = require('npmlog')
+const path = require('path')
+const test = require('tap').test
+
+const testDir = require('./util/test-dir')(__filename)
+
+const extract = require('../extract.js')
+
+npmlog.level = process.env.LOGLEVEL || 'silent'
+const OPTS = {
+  git: 'git',
+  cache: path.join(testDir, 'cache'),
+  log: npmlog,
+  registry: 'https://my.mock.registry/',
+  retry: false
+}
+
+test('extracting a git target reports failures', t => {
+  const oldPath = process.env.PATH
+  process.env.PATH = ''
+  const dest = path.join(testDir, 'foo')
+  return mkdirp(dest)
+    .then(() => fs.writeFileAsync(path.join(dest, 'q'), 'foo'))
+    .then(() => extract('github:zkat/pacote', dest,
+      Object.assign({}, OPTS)))
+    .finally(() => {
+      process.env.PATH = oldPath
+    })
+    .then(() => {
+      t.fail('the promise should not have resolved')
+    }, (err) => {
+      // We're not testing the specific text of the error message. We just check
+      // that it is an execution error.
+      t.equal(err.code, 'ENOENT')
+    })
+})