git: make 'from' and 'resolved' consistent and useful

- Always track a 'from' value for hosted git urls as the shortcut.
- Always track a 'resolved' for hosted git urls as the sshurl (even when
  we can actually fetch a rendered tarball, because we have the full
  sha.)

This makes 'from' and 'resolved' suitable to track and compare in
lockfiles and dependency lists.

Part 1 of fix for #15

Author: isaacs

lib/git.js

@@ -23,20 +23,23 @@ const _prepareDir = Symbol('_prepareDir')
 
 // get the repository url.  prefer ssh, fall back to git://
 const repoUrl = (hosted, opts) =>
-  hosted.ssh && hosted.ssh(opts) ||
+  hosted.sshurl && hosted.sshurl(opts) ||
   hosted.git && hosted.git(opts)
 
 class GitFetcher extends Fetcher {
   constructor (spec, opts) {
     super(spec, opts)
     this.resolvedRef = null
+    if (this.spec.hosted)
+      this.from = this.spec.hosted.shortcut({ noCommittish: false })
+
     // shortcut: avoid full clone when we can go straight to the tgz
     // if we have the full sha and it's a hosted git platform
     if (this.spec.gitCommittish && hashre.test(this.spec.gitCommittish)) {
       this.resolvedSha = this.spec.gitCommittish
-      // use hosted.tarball() and then later shell to RemoteFetcher
+      // use hosted.tarball() when we shell to RemoteFetcher later
       this.resolved = this.spec.hosted
-        ? this.spec.hosted.tarball({ noCommittish: false })
+        ? repoUrl(this.spec.hosted, { noCommittish: false })
         : this.spec.fetchSpec + '#' + this.spec.gitCommittish
     } else
       this.resolvedSha = ''
@@ -92,7 +95,7 @@ class GitFetcher extends Fetcher {
     // we haven't cloned, so a tgz download is still faster
     // of course, if it's not a known host, we can't do that.
     this.resolved = !this.spec.hosted ? withSha
-      : npa(withSha).hosted.tarball({ noCommittish: false })
+      : repoUrl(npa(withSha).hosted, { noCommittish: false })
   }
 
   // when we get the git sha, we affix it to our spec to build up
@@ -178,10 +181,10 @@ class GitFetcher extends Fetcher {
     const resolved = this.resolved
 
     return cacache.tmp.withTmp(this.cache, o, tmp => {
-      // if we're resolved, and it's a tarball url, shell out to RemoteFetcher
-      if (h && resolved === h.tarball({ noCommittish: false })) {
+      // if we're resolved, and have a tarball url, shell out to RemoteFetcher
+      if (h && resolved === repoUrl(h, { noCommittish: false }) && h.tarball) {
         const nameat = this.spec.name ? `${this.spec.name}@` : ''
-        return new RemoteFetcher(this.resolved, {
+        return new RemoteFetcher(h.tarball({ noCommittish: false }), {
           ...this.opts,
           pkgid: `git:${nameat}${this.resolved}`,
           resolved: this.resolved,
@@ -213,8 +216,9 @@ class GitFetcher extends Fetcher {
           readPackageJson(dir + '/package.json')
             .then(mani => this.package = {
               ...mani,
-              _integrity: String(this.integrity),
+              _integrity: this.integrity && String(this.integrity),
               _resolved: this.resolved,
+              _from: this.from,
             }))
   }
 

test/git.js

@@ -292,6 +292,7 @@ t.test('weird hosted that doesnt provide .git()', t => {
   spec.hosted = {
     git () { return null },
     tarball () { return null },
+    shortcut () { return `weird:${remote}` },
   }
   const r = new GitFetcher(spec, {cache})
   return t.rejects(r.resolve(), {
@@ -300,66 +301,79 @@ t.test('weird hosted that doesnt provide .git()', t => {
 })
 
 t.test('extract from tarball from hosted git service', t => {
-  const spec = npa(`repo@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' ],
-    readme: 'some docs and some more docs',
-    readmeFilename: 'README.md',
-    _id: 'repo@1.0.0',
-    _integrity: /^sha512-/,
-    _resolved: `${hostedUrl}/repo-HEAD.tgz`,
-  }))
-  .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' ],
-        readme: 'some docs and some more docs',
-        readmeFilename: 'README.md',
-        _id: 'repo@1.0.0',
-        _integrity: /^sha512-/,
-        _resolved: `${hostedUrl}/repo-HEAD.tgz`,
-        dist: {},
+  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' ],
+      readme: 'some docs and some more docs',
+      readmeFilename: 'README.md',
+      _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' ],
+          readme: 'some docs and some more docs',
+          readmeFilename: 'README.md',
+          _id: 'repo@1.0.0',
+          _integrity: /^sha512-/,
+          _resolved: `${remoteHosted}#${REPO_HEAD}`,
+          dist: {},
+        }
       }
-    }
-  }))
-  .then(() => g.extract(me + '/hosted'))
-  .then(() => {
-    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.test('add git sha to hosted git shorthand', t =>
   new GitFetcher('localhost:repo/x', {cache})
-    .resolve().then(r => t.equal(r, `${hostedUrl}/repo-HEAD.tgz`)))
+    .resolve().then(r => t.equal(r, `${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 => t.equal(result.resolved, `${hostedUrl}/repo-HEAD.tgz`)))
+    .then(result => {
+      head3 = result.resolved.split('#').pop()
+      t.match(result.resolved, /^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')
+    }))
 
   t.test('reglar', t =>
     new GitFetcher(`${remote}#HEAD~3`, {cache}).extract(me + '/h3r')
-    .then(result => t.notEqual(result.resolved,
-      `${remote}#${REPO_HEAD}`, 'something other than default head')))
+    .then(result => t.equal(result.resolved, `${remote}#${head3}`,
+      'got the same HEAD~3 sha as before')))
 
   t.end()
 })