Merge pull request #29 from npm/nlf/no-shell

fix: do not use a shell for git commands

Author: wraithgar

.github/workflows/ci.yml

@@ -64,10 +64,6 @@ jobs:
           git config --global user.name "Your Name"
           npm test -- --no-coverage --timeout 60
 
-      - name: Run linting
-        run: |
-          npm run lint
-
       ################################################################################
       # Run coverage check
       #

lib/clone.js

@@ -24,7 +24,7 @@ const { basename, resolve } = require('path')
 
 const revs = require('./revs.js')
 const spawn = require('./spawn.js')
-const { isWindows, escapePath } = require('./utils.js')
+const { isWindows } = require('./utils.js')
 
 const pickManifest = require('npm-pick-manifest')
 const fs = require('fs')
@@ -112,7 +112,7 @@ const branch = (repo, revDoc, target, opts) => {
     '-b',
     revDoc.ref,
     repo,
-    escapePath(target, opts),
+    target,
     '--recurse-submodules'
   ]
   if (maybeShallow(repo, opts)) { args.push('--depth=1') }
@@ -125,7 +125,7 @@ const plain = (repo, revDoc, target, opts) => {
   const args = [
     'clone',
     repo,
-    escapePath(target, opts),
+    target,
     '--recurse-submodules'
   ]
   if (maybeShallow(repo, opts)) { args.push('--depth=1') }
@@ -151,7 +151,7 @@ const unresolved = (repo, ref, target, opts) => {
   // can't do this one shallowly, because the ref isn't advertised
   // but we can avoid checking out the working dir twice, at least
   const lp = isWindows(opts) ? ['--config', 'core.longpaths=true'] : []
-  const cloneArgs = ['clone', '--mirror', '-q', repo, escapePath(target + '/.git', opts)]
+  const cloneArgs = ['clone', '--mirror', '-q', repo, target + '/.git']
   const git = (args) => spawn(args, { ...opts, cwd: target })
   return mkdirp(target)
     .then(() => git(cloneArgs.concat(lp)))

lib/opts.js

@@ -7,5 +7,6 @@ const gitEnv = {
 module.exports = (opts = {}) => ({
   stdioString: true,
   ...opts,
+  shell: false,
   env: opts.env || { ...gitEnv, ...process.env }
 })

lib/utils.js

@@ -1,16 +1,3 @@
-const { basename } = require('path')
-
 const isWindows = opts => (opts.fakePlatform || process.platform) === 'win32'
 
-// wrap the target in quotes for Windows when using cmd(.exe) as a shell to
-// avoid clone failures for paths with spaces
-const escapePath = (gitPath, opts) => {
-  const isCmd = opts.shell && (basename(opts.shell.toLowerCase(), '.exe') === 'cmd')
-  if (isWindows(opts) && isCmd && !gitPath.startsWith('"')) {
-    return `"${gitPath}"`
-  }
-  return gitPath
-}
-
-exports.escapePath = escapePath
 exports.isWindows = isWindows

lib/which.js

@@ -1,4 +1,3 @@
-const { escapePath } = require('./utils.js')
 const which = require('which')
 
 let gitPath
@@ -13,5 +12,5 @@ module.exports = (opts = {}) => {
   if (!gitPath || opts.git === false) {
     return Object.assign(new Error('No git binary found in $PATH'), { code: 'ENOGIT' })
   }
-  return escapePath(gitPath, opts)
+  return gitPath
 }

package.json

@@ -19,7 +19,8 @@
     "prepublishOnly": "git push origin --follow-tags",
     "preversion": "npm test",
     "snap": "tap",
-    "test": "tap"
+    "test": "tap",
+    "posttest": "npm run lint"
   },
   "tap": {
     "check-coverage": true,

test/clone.js

@@ -253,5 +253,8 @@ if ((process.platform) === 'win32') {
   )
 } else {
   t.test('cloning to folder with spaces with cmd as the shell not on windows', t =>
-    t.rejects(clone(join(regularRepo, '.git'), 'HEAD', clonedRepoSpaces2, { fakePlatform: 'win32', shell: 'cmd' })))
+    clone(join(regularRepo, '.git'), 'HEAD', clonedRepoSpaces2, { fakePlatform: 'win32', shell: 'cmd' })
+      .then(() => revs(regularRepo))
+      .then((r) => revs(clonedRepoSpaces2).then((r2) => t.same(Object.keys(r.shas), Object.keys(r2.shas))))
+  )
 }

test/opts.js

@@ -7,6 +7,9 @@ const gitEnv = {
 
 t.match(gitOpts().env, gitEnv, 'got the git defaults we want')
 
+t.equal(gitOpts().shell, false, 'shell defaults to false')
+t.equal(gitOpts({ shell: '/bin/bash' }).shell, false, 'shell cannot be overridden')
+
 t.test('does not override', t => {
   const { GIT_ASKPASS, GIT_SSH_COMMAND } = process.env
   t.teardown(() => {