From 7f07b5d6d74a2914f4dcb0f59490122f797f6f23 Mon Sep 17 00:00:00 2001
From: Louis-Dominique Dubeau <ldd@lddubeau.com>
Date: Tue, 2 Jul 2019 16:09:36 -0400
Subject: [PATCH] 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: https://github.com/npm/pacote/pull/1
Close: #1
Reviewed-by: @isaacs
---
 extract.js          | 21 +++++++++++----------
 test/git.extract.js | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 10 deletions(-)
 create mode 100644 test/git.extract.js

diff --git a/extract.js b/extract.js
index d2ab47de..f49a5442 100644
--- a/extract.js
+++ b/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
     })
 }
diff --git a/test/git.extract.js b/test/git.extract.js
new file mode 100644
index 00000000..8bad8515
--- /dev/null
+++ b/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')
+    })
+})