git: prefer git+https over git+ssh for hosted repo

This uses the faster and more headless-friendly git+https url when
fetching git remotes on hosted repos, falling back to ssh when https
fails.

Using ssh is better for private repos that require authorization, but
for public repos, https is faster and does not require user interaction
to authorize the use of an SSH key (which may require a passphrase or
2FA).

Resolved url is still reported as the git+ssh url, for consistency.

Fix: #20

lib/git.js

@@ -14,17 +14,21 @@ const readPackageJson = require('read-package-json-fast')
 const npm = require('./util/npm.js')
 
 const _resolvedFromRepo = Symbol('_resolvedFromRepo')
+const _resolvedFromHosted = Symbol('_resolvedFromHosted')
 const _resolvedFromClone = Symbol('_resolvedFromClone')
 const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
 const _addGitSha = Symbol('_addGitSha')
 const _clone = Symbol('_clone')
+const _cloneHosted = Symbol('_cloneHosted')
+const _cloneRepo = Symbol('_cloneRepo')
 const _setResolvedWithSha = Symbol('_setResolvedWithSha')
 const _prepareDir = Symbol('_prepareDir')
 
 // get the repository url.  prefer ssh, fall back to git://
+// We have to add the git+ back because npa suppresses it.
 const repoUrl = (hosted, opts) =>
   hosted.sshurl && addGitPlus(hosted.sshurl(opts)) ||
-  hosted.git && hosted.git(opts)
+  hosted.https && addGitPlus(hosted.https(opts))
 
 const addGitPlus = url => url && `git+${url}`
 
@@ -60,10 +64,23 @@ class GitFetcher extends Fetcher {
     // fetch the git repo and then look at the current hash
     const h = this.spec.hosted
     // try to use ssh, fall back to git.
-    return h ? this[_resolvedFromRepo](repoUrl(h))
+    return h ? this[_resolvedFromHosted](h)
       : this[_resolvedFromRepo](this.spec.fetchSpec)
   }
 
+  // first try https, since that's faster and passphrase-less for
+  // public repos.  Fall back to SSH to support private repos.
+  // NB: we always store the SSH url in the 'resolved' field.
+  [_resolvedFromHosted] (hosted) {
+    return this[_resolvedFromRepo](hosted.https && hosted.https())
+      .catch(er => {
+        const ssh = hosted.sshurl && hosted.sshurl()
+        if (!ssh)
+          throw er
+        return this[_resolvedFromRepo](ssh)
+      })
+  }
+
   [_resolvedFromRepo] (gitRemote) {
     // XXX make this a custom error class
     if (!gitRemote)
@@ -182,7 +199,7 @@ class GitFetcher extends Fetcher {
     const h = this.spec.hosted
     const resolved = this.resolved
 
-    // can be set manually to false to fall back to sshurl
+    // can be set manually to false to fall back to actual git clone
     tarballOk = tarballOk &&
       h && resolved === repoUrl(h, { noCommittish: false }) && h.tarball
 
@@ -204,20 +221,35 @@ class GitFetcher extends Fetcher {
         })
       }
 
-      const repo = h && !this.spec.fetchSpec
-        ? repoUrl(h, { noCommittish: true })
-        : this.spec.fetchSpec
-
-      return git.clone(repo, ref, tmp, this.spec, this.opts)
-        .then(sha => {
-          this.resolvedSha = sha
-          if (!this.resolved)
-            this[_addGitSha](sha)
-        })
-        .then(() => handler(tmp))
+      return (
+        h ? this[_cloneHosted](ref, tmp)
+        : this[_cloneRepo](this.spec.fetchSpec, ref, tmp)
+      ).then(sha => {
+        this.resolvedSha = sha
+        if (!this.resolved)
+          this[_addGitSha](sha)
+      })
+      .then(() => handler(tmp))
     })
   }
 
+  [_cloneHosted] (ref, tmp) {
+    const hosted = this.spec.hosted
+    const https = hosted.https()
+    return this[_cloneRepo](hosted.https({ noCommittish: true }), ref, tmp)
+      .catch(er => {
+        const ssh = hosted.sshurl && hosted.sshurl({ noCommittish: true })
+        /* istanbul ignore if - should be covered by the resolve() call */
+        if (!ssh)
+          throw er
+        return this[_cloneRepo](ssh, ref, tmp)
+      })
+  }
+
+  [_cloneRepo] (repo, ref, tmp) {
+    return git.clone(repo, ref, tmp, this.spec, this.opts)
+  }
+
   manifest () {
     if (this.package)
       return Promise.resolve(this.package)

test/git.js

@@ -3,13 +3,24 @@ const httpPort = 18000 + (+process.env.TAP_CHILD_ID || 0)
 const hostedUrl = `http://localhost:${httpPort}`
 const ghi = require('hosted-git-info/git-host-info.js')
 const gitPort = 12345 + (+process.env.TAP_CHILD_ID || 0)
-const remote = `git://localhost:${gitPort}/repo`
-const remoteHosted = `git://127.0.0.1:${gitPort}/repo`
-const submodsRemote = `git://localhost:${gitPort}/submodule-repo`
 ghi.localhost = {
-  protocols: [ 'git' ],
+  protocols: [ 'git+https', 'git+ssh' ],
   domain: `127.0.0.1:${gitPort}`,
-  gittemplate: 'git://{domain}/{user}{#committish}',
+  httpstemplate: 'git://{domain}/{user}{#committish}',
+  treepath: 'not-implemented',
+  tarballtemplate: `${hostedUrl}/repo-HEAD.tgz`,
+  // shortcut MUST have a user and project, at least
+  shortcuttemplate: '{type}:{user}/x{#committish}',
+  pathtemplate: '/{user}{#committish}',
+  pathmatch: /^\/(repo|submodule-repo)/,
+  hashformat: h => h,
+  protocols_re: /^(git):$/
+}
+ghi.localhostssh = {
+  protocols: [ 'git+ssh' ],
+  domain: `localhostssh:${gitPort}`,
+  sshtemplate: `git://127.0.0.1:${gitPort}/{user}{#committish}`,
+  sshurltemplate: `git://127.0.0.1:${gitPort}/{user}{#committish}`,
   treepath: 'not-implemented',
   tarballtemplate: `${hostedUrl}/repo-HEAD.tgz`,
   // shortcut MUST have a user and project, at least
@@ -20,6 +31,13 @@ ghi.localhost = {
   protocols_re: /^(git):$/
 }
 
+
+const remote = `git://localhost:${gitPort}/repo`
+const remoteHosted = `git://127.0.0.1:${gitPort}/repo`
+const submodsRemote = `git://localhost:${gitPort}/submodule-repo`
+
+const remoteHostedSSH = `git://localhostssh:${gitPort}/repo`
+
 const GitFetcher = require('../lib/git.js')
 const t = require('tap')
 const fs = require('fs')
@@ -287,80 +305,102 @@ t.test('basic stuff', async t => {
   fs.statSync(me + '/s/fooblz/package.json')
 })
 
-t.test('weird hosted that doesnt provide .git()', t => {
-  const spec = npa(remote)
-  spec.hosted = {
+t.test('weird hosted that doesnt provide any fetch targets', t => {
+  const hosted = {
     git () { return null },
+    ssh () { return null },
+    sshurl () { return null },
+    https () { return null },
     tarball () { return null },
     shortcut () { return `weird:${remote}` },
   }
-  const r = new GitFetcher(spec, {cache})
-  return t.rejects(r.resolve(), {
+
+  const spec = npa(remote)
+  spec.hosted = hosted
+  t.rejects(new GitFetcher(Object.assign(spec, {hosted}), {cache}).resolve(), {
     message: `No git url for ${remote}`,
   })
+
+  const resSpec= npa(`${remote}#${REPO_HEAD}`)
+  resSpec.hosted = hosted
+  t.rejects(new GitFetcher(Object.assign(resSpec, {hosted}), {cache})
+    .extract(`${me}/weird-hosted-extract`), {
+    message: `No git url for ${remote}`,
+  })
+
+  t.end()
 })
 
 t.test('extract from tarball from hosted git service', t => {
-  const runTest = nameat => t => {
-    const spec = npa(`${nameat}localhost:repo/x#${REPO_HEAD}`)
-    const g = new GitFetcher(spec, {cache})
-    return g.manifest().then(m => t.match(m, {
-      name: 'repo',
-      version: '1.0.0',
-      description: 'just some random thing',
-      devDependencies: {
-        abbrev: abbrevSpec
-      },
-      scripts: { prepare: 'node prepare.js', test: 'node index.js' },
-      files: [ 'index.js' ],
-      _id: 'repo@1.0.0',
-      _integrity: /^sha512-/,
-      _resolved: `${remoteHosted}#${REPO_HEAD}`,
-    }))
-    .then(() => g.packument())
-    .then(p => t.match(p, {
-      name: 'repo',
-      'dist-tags': { latest: '1.0.0' },
-      versions: {
-        '1.0.0': {
-          name: 'repo',
-          version: '1.0.0',
-          description: 'just some random thing',
-          devDependencies: {
-            abbrev: abbrevSpec,
-          },
-          scripts: { prepare: 'node prepare.js', test: 'node index.js' },
-          files: [ 'index.js' ],
-          _id: 'repo@1.0.0',
-          _integrity: /^sha512-/,
-          _resolved: `${remoteHosted}#${REPO_HEAD}`,
-          dist: {},
+  // run in both ssh and https url types from a hosted service
+  // both of these actually produce a git:// url so that the test
+  // doesn't hang waiting for SSH key approval/passphrases.
+  const domains = ['localhost', 'localhostssh']
+
+  t.plan(domains.length)
+  domains.forEach(domain => t.test(domain, t => {
+    const runTest = nameat => t => {
+      const spec = npa(`${nameat}${domain}:repo/x#${REPO_HEAD}`)
+      const g = new GitFetcher(spec, {cache})
+      return g.manifest().then(m => t.match(m, {
+        name: 'repo',
+        version: '1.0.0',
+        description: 'just some random thing',
+        devDependencies: {
+          abbrev: abbrevSpec
+        },
+        scripts: { prepare: 'node prepare.js', test: 'node index.js' },
+        files: [ 'index.js' ],
+        _id: 'repo@1.0.0',
+        _integrity: /^sha512-/,
+        _resolved: `${remoteHosted}#${REPO_HEAD}`,
+      }))
+      .then(() => g.packument())
+      .then(p => t.match(p, {
+        name: 'repo',
+        'dist-tags': { latest: '1.0.0' },
+        versions: {
+          '1.0.0': {
+            name: 'repo',
+            version: '1.0.0',
+            description: 'just some random thing',
+            devDependencies: {
+              abbrev: abbrevSpec,
+            },
+            scripts: { prepare: 'node prepare.js', test: 'node index.js' },
+            files: [ 'index.js' ],
+            _id: 'repo@1.0.0',
+            _integrity: /^sha512-/,
+            _resolved: `${remoteHosted}#${REPO_HEAD}`,
+            dist: {},
+          }
         }
-      }
-    }))
-    .then(() => g.extract(me + '/hosted'))
-    .then(result => {
-      t.throws(() => fs.statSync(me + '/hosted/prepare.js'))
-      fs.statSync(me + '/hosted/index.js')
-    })
-  }
+      }))
+      .then(() => g.extract(me + '/hosted'))
+      .then(result => {
+        t.throws(() => fs.statSync(me + '/hosted/prepare.js'))
+        fs.statSync(me + '/hosted/index.js')
+      })
+    }
 
-  t.plan(2)
-  t.test('with repo@ on the spec', runTest('repo@'))
-  t.test('without repo@on the spec', runTest(''))
+    t.plan(2)
+    t.test('with repo@ on the spec', runTest('repo@'))
+    t.test('without repo@on the spec', runTest(''))
+  }))
 })
 
 t.test('add git sha to hosted git shorthand', t =>
   new GitFetcher('localhost:repo/x', {cache})
-    .resolve().then(r => t.equal(r, `${remoteHosted}#${REPO_HEAD}`)))
+    // it adds the git+ because it thinks it's https
+    .resolve().then(r => t.equal(r, `git+${remoteHosted}#${REPO_HEAD}`)))
 
 t.test('fetch a weird ref', t => {
   let head3 = ''
   t.test('hosted', t =>
     new GitFetcher('localhost:repo/x#HEAD~3', {cache}).extract(me + '/h3h')
     .then(result => {
       head3 = result.resolved.split('#').pop()
-      t.match(result.resolved, /^git:\/\/127\.0\.0\.1:[0-9]+\/repo#[a-z0-9]{40}$/,
+      t.match(result.resolved, /^git\+git:\/\/127\.0\.0\.1:[0-9]+\/repo#[a-z0-9]{40}$/,
         'got git url as resolved value')
       t.notEqual(result.resolved, `${remoteHosted}#${REPO_HEAD}`,
         'git url for HEAD~3 is not the same as HEAD')
@@ -396,3 +436,9 @@ t.test('resolved is a git+ssh url for hosted repos that support it', t => {
   t.equal(gf.resolved, `git+ssh://git@github.com/x/y.git#${hash}`)
   t.end()
 })
+
+t.test('fall back to ssh url if https url fails or is missing', t => {
+  const gf = new GitFetcher(`localhostssh:repo/x`, {cache})
+  return gf.extract(`${me}/localhostssh`).then(({resolved}) =>
+    t.equal(resolved, `git+git://127.0.0.1:${gitPort}/repo#${REPO_HEAD}`))
+})