git: use paths as presented by user

* Save the lightest-possibly normalized URL into `package.json` and
  package.json _from fields, because read-package-json, read-installed,
  and the cache all know how to handle them now.
* Add support for github:, gist:, bitbucket:, and gitlab: shortcut
  syntax. GitHub shortcuts will continue to be normalized to org/repo
  instead of being saved with a github: prefix (for now).
* If presented with shortcuts, try cloning via git, SSH, and HTTPS (in
  that order, skipping any methods that aren't supported by a given
  hosting provider).
* No longer prompt for credentials -- it didn't work right with the
  spinner, and wasn't guaranteed to work anyway. We may experiment with
  doing this a better way in the future. Users can override this by
  setting GIT_ASKPASS in their environment if they want to experiment
  with interactive cloning, but should also set --no-spin on the npm
  command line (or run `npm config set spin=false`).

9 files changed, 334 insertions, 391 deletions

diff --git a/lib/cache.js b/lib/cache.js
index b3c237509..cae09b181 100644
--- a/lib/cache.js
+++ b/lib/cache.js
@@ -76,7 +76,6 @@ var npm = require("./npm.js")
   , addLocal = require("./cache/add-local.js")
   , addRemoteTarball = require("./cache/add-remote-tarball.js")
   , addRemoteGit = require("./cache/add-remote-git.js")
-  , maybeGithub = require("./cache/maybe-github.js")
   , inflight = require("inflight")
   , realizePackageSpecifier = require("realize-package-specifier")
   , npa = require("npm-package-arg")
@@ -296,14 +295,8 @@ function add (args, where, cb) {
         })
         break
       case "git":
-        addRemoteGit(p.spec, false, cb)
-        break
       case "hosted":
-        if (p.hosted.type === "github") {
-          maybeGithub(p.rawSpec, cb)
-        } else {
-          addRemoteGit(p.spec, false, cb)
-        }
+        addRemoteGit(p.rawSpec, cb)
         break
       default:
         if (p.name) return addNamed(p.name, p.spec, null, cb)
diff --git a/lib/cache/add-remote-git.js b/lib/cache/add-remote-git.js
index 84d5207dd..78f0717c6 100644
--- a/lib/cache/add-remote-git.js
+++ b/lib/cache/add-remote-git.js
@@ -1,73 +1,134 @@
-var mkdir = require('mkdirp')
 var assert = require('assert')
-var git = require('../utils/git.js')
+var crypto = require('crypto')
 var fs = require('graceful-fs')
-var log = require('npmlog')
 var path = require('path')
 var url = require('url')
+
 var chownr = require('chownr')
-var crypto = require('crypto')
-var npm = require('../npm.js')
-var rm = require('../utils/gently-rm.js')
+var dezalgo = require('dezalgo')
+var hostedFromURL = require('hosted-git-info').fromUrl
 var inflight = require('inflight')
-var getCacheStat = require('./get-stat.js')
-var addLocal = require('./add-local.js')
-var realizePackageSpecifier = require('realize-package-specifier')
+var log = require('npmlog')
+var mkdir = require('mkdirp')
 var normalizeGitUrl = require('normalize-git-url')
-var randomBytes = require('crypto').pseudoRandomBytes // only need uniqueness
+var npa = require('npm-package-arg')
+var realizePackageSpecifier = require('realize-package-specifier')
+
+var addLocal = require('./add-local.js')
+var getCacheStat = require('./get-stat.js')
+var git = require('../utils/git.js')
+var npm = require('../npm.js')
+var rm = require('../utils/gently-rm.js')
 
 var remotes = path.resolve(npm.config.get('cache'), '_git-remotes')
 var templates = path.join(remotes, '_templates')
 
 var VALID_VARIABLES = [
-  'GIT_SSH',
-  'GIT_SSL_NO_VERIFY',
+  'GIT_ASKPASS',
   'GIT_PROXY_COMMAND',
-  'GIT_SSL_CAINFO'
+  'GIT_SSH',
+  'GIT_SSL_CAINFO',
+  'GIT_SSL_NO_VERIFY'
 ]
 
-module.exports = function addRemoteGit (uri, silent, cb) {
+module.exports = function addRemoteGit (uri, _cb) {
   assert(typeof uri === 'string', 'must have git URL')
-  assert(typeof cb === 'function', 'must have callback')
+  assert(typeof _cb === 'function', 'must have callback')
+  var cb = dezalgo(_cb)
+
+  // the URL comes in exactly as it was passed on the command line, or as
+  // normalized by normalize-package-data / read-package-json / read-installed,
+  // so figure out what to do with it using hosted-git-info
+  var parsed = hostedFromURL(uri)
+  if (parsed) {
+    // normalize GitHub syntax to org/repo (for now)
+    var from
+    if (parsed.type === 'github' && parsed.default === 'shortcut') {
+      from = parsed.path()
+    } else {
+      from = parsed.toString()
+    }
+
+    log.verbose('addRemoteGit', from, 'is a repository hosted by', parsed.type)
+
+    // prefer explicit URLs to pushing everything through shortcuts
+    if (parsed.default !== 'shortcut') {
+      return tryClone(from, parsed.toString(), false, cb)
+    }
 
-  // reconstruct the URL as it was passed in – realizePackageSpecifier
-  // strips off `git+` and `maybeGithub` doesn't include it.
-  var originalURL
-  if (!/^git[+:]/.test(uri)) {
-    originalURL = 'git+' + uri
+    // try git:, then git+ssh:, then git+https: before failing
+    tryGitProto(from, parsed, cb)
   } else {
-    originalURL = uri
+    // verify that this is a Git URL before continuing
+    parsed = npa(uri)
+    if (parsed.type !== 'git') {
+      return cb(new Error(uri + 'is not a Git or GitHub URL'))
+    }
+
+    tryClone(parsed.rawSpec, uri, false, cb)
+  }
+}
+
+function tryGitProto (from, hostedInfo, cb) {
+  var gitURL = hostedInfo.git()
+  if (!gitURL) return trySSH(from, hostedInfo, cb)
+
+  log.silly('tryGitProto', 'attempting to clone', gitURL)
+  tryClone(from, gitURL, true, function (er) {
+    if (er) return trySSH(from, hostedInfo, cb)
+
+    cb.apply(this, arguments)
+  })
+}
+
+function trySSH (from, hostedInfo, cb) {
+  var sshURL = hostedInfo.ssh()
+  if (!sshURL) return tryHTTPS(from, hostedInfo, cb)
+
+  log.silly('trySSH', 'attempting to clone', sshURL)
+  tryClone(from, sshURL, true, function (er) {
+    if (er) return tryHTTPS(from, hostedInfo, cb)
+
+    cb.apply(this, arguments)
+  })
+}
+
+function tryHTTPS (from, hostedInfo, cb) {
+  var httpsURL = hostedInfo.https()
+  if (!httpsURL) {
+    return cb(new Error(from + ' can not be cloned via Git, SSH, or HTTPS'))
   }
 
-  // break apart the origin URL and the branch / tag / commitish
-  var normalized = normalizeGitUrl(uri)
-  var gitURL = normalized.url
+  log.silly('tryHTTPS', 'attempting to clone', httpsURL)
+  tryClone(from, httpsURL, false, cb)
+}
+
+function tryClone (from, combinedURL, silent, cb) {
+  log.silly('tryClone', 'cloning', from, 'via', combinedURL)
+
+  var normalized = normalizeGitUrl(combinedURL)
+  var cloneURL = normalized.url
   var treeish = normalized.branch
 
   // ensure that similarly-named remotes don't collide
-  var repoID = gitURL.replace(/[^a-zA-Z0-9]+/g, '-') + '-' +
-    crypto.createHash('sha1').update(gitURL).digest('hex').slice(0, 8)
+  var repoID = cloneURL.replace(/[^a-zA-Z0-9]+/g, '-') + '-' +
+    crypto.createHash('sha1').update(cloneURL).digest('hex').slice(0, 8)
   var cachedRemote = path.join(remotes, repoID)
 
-  // set later, as the callback flow proceeds
-  var resolvedURL
-  var resolvedTreeish
-  var tmpdir
-
   cb = inflight(repoID, cb)
   if (!cb) {
-    return log.verbose('addRemoteGit', repoID, 'already in flight; waiting')
+    return log.verbose('tryClone', repoID, 'already in flight; waiting')
   }
-  log.verbose('addRemoteGit', repoID, 'not in flight; caching')
+  log.verbose('tryClone', repoID, 'not in flight; caching')
 
   // initialize the remotes cache with the correct perms
   getGitDir(function (er) {
     if (er) return cb(er)
     fs.stat(cachedRemote, function (er, s) {
-      if (er) return mirrorRemote(finish)
-      if (!s.isDirectory()) return resetRemote(finish)
+      if (er) return mirrorRemote(from, cloneURL, treeish, cachedRemote, silent, finish)
+      if (!s.isDirectory()) return resetRemote(from, cloneURL, treeish, cachedRemote, finish)
 
-      validateExistingRemote(finish)
+      validateExistingRemote(from, cloneURL, treeish, cachedRemote, finish)
     })
 
     // always set permissions on the cached remote
@@ -78,235 +139,229 @@ module.exports = function addRemoteGit (uri, silent, cb) {
       })
     }
   })
+}
 
-  // don't try too hard to hold on to a remote
-  function resetRemote (cb) {
-    log.info('addRemoteGit', 'resetting', cachedRemote)
-    rm(cachedRemote, function (er) {
-      if (er) return cb(er)
-      mirrorRemote(cb)
-    })
-  }
-
-  // reuse a cached remote when possible, but nuke it if it's in an
-  // inconsistent state
-  function validateExistingRemote (cb) {
-    git.whichAndExec(
-      ['config', '--get', 'remote.origin.url'],
-      { cwd: cachedRemote, env: gitEnv() },
-      function (er, stdout, stderr) {
-        var originURL
-        if (stdout) {
-          originURL = stdout.trim()
-          log.verbose('addRemoteGit', 'remote.origin.url:', originURL)
-        }
-
-        if (stderr) stderr = stderr.trim()
-        if (stderr || er) {
-          log.warn('addRemoteGit', 'resetting remote', cachedRemote, 'because of error:', stderr || er)
-          return resetRemote(cb)
-        } else if (gitURL !== originURL) {
-          log.warn(
-            'addRemoteGit',
-            'pre-existing cached repo', cachedRemote, 'points to', originURL, 'and not', gitURL
-          )
-          return resetRemote(cb)
-        }
+// don't try too hard to hold on to a remote
+function resetRemote (from, cloneURL, treeish, cachedRemote, cb) {
+  log.info('resetRemote', 'resetting', cachedRemote, 'for', from)
+  rm(cachedRemote, function (er) {
+    if (er) return cb(er)
+    mirrorRemote(from, cloneURL, treeish, cachedRemote, false, cb)
+  })
+}
 
-        log.verbose('addRemoteGit', 'updating existing cached remote', cachedRemote)
-        updateRemote(cb)
+// reuse a cached remote when possible, but nuke it if it's in an
+// inconsistent state
+function validateExistingRemote (from, cloneURL, treeish, cachedRemote, cb) {
+  git.whichAndExec(
+    ['config', '--get', 'remote.origin.url'],
+    { cwd: cachedRemote, env: gitEnv() },
+    function (er, stdout, stderr) {
+      var originURL
+      if (stdout) {
+        originURL = stdout.trim()
+        log.silly('validateExistingRemote', from, 'remote.origin.url:', originURL)
       }
-    )
-  }
-
-  // make a complete bare mirror of the remote repo
-  // NOTE: npm uses a blank template directory to prevent weird inconsistencies
-  // https://github.com/npm/npm/issues/5867
-  function mirrorRemote (cb) {
-    mkdir(cachedRemote, function (er) {
-      if (er) return cb(er)
 
-      var args = [
-        'clone',
-        '--template=' + templates,
-        '--mirror',
-        gitURL, cachedRemote
-      ]
-      git.whichAndExec(
-        ['clone', '--template=' + templates, '--mirror', gitURL, cachedRemote],
-        { cwd: cachedRemote, env: gitEnv() },
-        function (er, stdout, stderr) {
-          if (er) {
-            var combined = (stdout + '\n' + stderr).trim()
-            var command = 'git ' + args.join(' ') + ':'
-            if (silent) {
-              log.verbose(command, combined)
-            } else {
-              log.error(command, combined)
-            }
-            return cb(er)
-          }
-          log.verbose('addRemoteGit', 'git clone ' + gitURL, stdout.trim())
-          setPermissions(cb)
-        }
-      )
-    })
-  }
-
-  function setPermissions (cb) {
-    if (process.platform === 'win32') {
-      log.verbose('addRemoteGit', 'skipping chownr on Windows')
-      resolveHead(cb)
-    } else {
-      getGitDir(function (er, cs) {
-        if (er) {
-          log.error('addRemoteGit', 'could not get cache stat')
-          return cb(er)
-        }
-
-        chownr(cachedRemote, cs.uid, cs.gid, function (er) {
-          if (er) {
-            log.error(
-              'addRemoteGit',
-              'Failed to change folder ownership under npm cache for',
-              cachedRemote
-            )
-            return cb(er)
-          }
+      if (stderr) stderr = stderr.trim()
+      if (stderr || er) {
+        log.warn('addRemoteGit', from, 'resetting remote', cachedRemote, 'because of error:', stderr || er)
+        return resetRemote(from, cloneURL, treeish, cachedRemote, cb)
+      } else if (cloneURL !== originURL) {
+        log.warn(
+          'addRemoteGit',
+          from,
+          'pre-existing cached repo', cachedRemote, 'points to', originURL, 'and not', cloneURL
+        )
+        return resetRemote(from, cloneURL, treeish, cachedRemote, cb)
+      }
 
-          log.verbose('addRemoteGit', 'set permissions on', cachedRemote)
-          resolveHead(cb)
-        })
-      })
+      log.verbose('validateExistingRemote', from, 'is updating existing cached remote', cachedRemote)
+      updateRemote(from, cloneURL, treeish, cachedRemote, cb)
     }
-  }
+  )
+}
 
-  // always fetch the origin, even right after mirroring, because this way
-  // permissions will get set correctly
-  function updateRemote (cb) {
+// make a complete bare mirror of the remote repo
+// NOTE: npm uses a blank template directory to prevent weird inconsistencies
+// https://github.com/npm/npm/issues/5867
+function mirrorRemote (from, cloneURL, treeish, cachedRemote, silent, cb) {
+  mkdir(cachedRemote, function (er) {
+    if (er) return cb(er)
+
+    var args = [
+      'clone',
+      '--template=' + templates,
+      '--mirror',
+      cloneURL, cachedRemote
+    ]
     git.whichAndExec(
-      ['fetch', '-a', 'origin'],
+      ['clone', '--template=' + templates, '--mirror', cloneURL, cachedRemote],
       { cwd: cachedRemote, env: gitEnv() },
       function (er, stdout, stderr) {
         if (er) {
           var combined = (stdout + '\n' + stderr).trim()
-          log.error('git fetch -a origin (' + gitURL + ')', combined)
+          var command = 'git ' + args.join(' ') + ':'
+          if (silent) {
+            log.verbose(command, combined)
+          } else {
+            log.error(command, combined)
+          }
           return cb(er)
         }
-        log.verbose('addRemoteGit', 'git fetch -a origin (' + gitURL + ')', stdout.trim())
-
-        setPermissions(cb)
+        log.verbose('mirrorRemote', from, 'git clone ' + cloneURL, stdout.trim())
+        setPermissions(from, cloneURL, treeish, cachedRemote, cb)
       }
     )
-  }
+  })
+}
 
-  // branches and tags are both symbolic labels that can be attached to different
-  // commits, so resolve the commitish to the current actual treeish the label
-  // corresponds to
-  //
-  // important for shrinkwrap
-  function resolveHead (cb) {
-    log.verbose('addRemoteGit', 'original treeish:', treeish)
-    var args = ['rev-list', '-n1', treeish]
-    git.whichAndExec(
-      args,
-      { cwd: cachedRemote, env: gitEnv() },
-      function (er, stdout, stderr) {
+function setPermissions (from, cloneURL, treeish, cachedRemote, cb) {
+  if (process.platform === 'win32') {
+    log.verbose('setPermissions', from, 'skipping chownr on Windows')
+    resolveHead(from, cloneURL, treeish, cachedRemote, cb)
+  } else {
+    getGitDir(function (er, cs) {
+      if (er) {
+        log.error('setPermissions', from, 'could not get cache stat')
+        return cb(er)
+      }
+
+      chownr(cachedRemote, cs.uid, cs.gid, function (er) {
         if (er) {
-          log.error('git ' + args.join(' ') + ':', stderr)
+          log.error(
+            'setPermissions',
+            'Failed to change git repository ownership under npm cache for',
+            cachedRemote
+          )
           return cb(er)
         }
 
-        resolvedTreeish = stdout.trim()
-        log.silly('addRemoteGit', 'resolved treeish:', resolvedTreeish)
+        log.verbose('setPermissions', from, 'set permissions on', cachedRemote)
+        resolveHead(from, cloneURL, treeish, cachedRemote, cb)
+      })
+    })
+  }
+}
 
-        resolvedURL = getResolved(originalURL, resolvedTreeish)
-        log.verbose('addRemoteGit', 'resolved Git URL:', resolvedURL)
+// always fetch the origin, even right after mirroring, because this way
+// permissions will get set correctly
+function updateRemote (from, cloneURL, treeish, cachedRemote, cb) {
+  git.whichAndExec(
+    ['fetch', '-a', 'origin'],
+    { cwd: cachedRemote, env: gitEnv() },
+    function (er, stdout, stderr) {
+      if (er) {
+        var combined = (stdout + '\n' + stderr).trim()
+        log.error('git fetch -a origin (' + cloneURL + ')', combined)
+        return cb(er)
+      }
+      log.verbose('updateRemote', 'git fetch -a origin (' + cloneURL + ')', stdout.trim())
 
-        // generate a unique filename
-        tmpdir = path.join(
-          npm.tmp,
-          'git-cache-' + randomBytes(6).toString('hex'),
-          resolvedTreeish
-        )
-        log.silly('addRemoteGit', 'Git working directory:', tmpdir)
+      setPermissions(from, cloneURL, treeish, cachedRemote, cb)
+    }
+  )
+}
 
-        mkdir(tmpdir, function (er) {
-          if (er) return cb(er)
+// branches and tags are both symbolic labels that can be attached to different
+// commits, so resolve the commitish to the current actual treeish the label
+// corresponds to
+//
+// important for shrinkwrap
+function resolveHead (from, cloneURL, treeish, cachedRemote, cb) {
+  log.verbose('resolveHead', from, 'original treeish:', treeish)
+  var args = ['rev-list', '-n1', treeish]
+  git.whichAndExec(
+    args,
+    { cwd: cachedRemote, env: gitEnv() },
+    function (er, stdout, stderr) {
+      if (er) {
+        log.error('git ' + args.join(' ') + ':', stderr)
+        return cb(er)
+      }
 
-          cloneResolved(cb)
-        })
+      var resolvedTreeish = stdout.trim()
+      log.silly('resolveHead', from, 'resolved treeish:', resolvedTreeish)
+
+      var resolvedURL = getResolved(cloneURL, resolvedTreeish)
+      log.verbose('resolveHead', from, 'resolved Git URL:', resolvedURL)
+
+      // generate a unique filename
+      var tmpdir = path.join(
+        npm.tmp,
+        'git-cache-' + crypto.pseudoRandomBytes(6).toString('hex'),
+        resolvedTreeish
+      )
+      log.silly('resolveHead', 'Git working directory:', tmpdir)
+
+      mkdir(tmpdir, function (er) {
+        if (er) return cb(er)
+
+        cloneResolved(from, resolvedURL, resolvedTreeish, cachedRemote, tmpdir, cb)
+      })
+    }
+  )
+}
+
+// make a clone from the mirrored cache so we have a temporary directory in
+// which we can check out the resolved treeish
+function cloneResolved (from, resolvedURL, resolvedTreeish, cachedRemote, tmpdir, cb) {
+  var args = ['clone', cachedRemote, tmpdir]
+  git.whichAndExec(
+    args,
+    { cwd: cachedRemote, env: gitEnv() },
+    function (er, stdout, stderr) {
+      stdout = (stdout + '\n' + stderr).trim()
+      if (er) {
+        log.error('git ' + args.join(' ') + ':', stderr)
+        return cb(er)
       }
-    )
-  }
+      log.verbose('cloneResolved', from, 'clone', stdout)
 
-  // make a clone from the mirrored cache so we have a temporary directory in
-  // which we can check out the resolved treeish
-  function cloneResolved (cb) {
-    var args = ['clone', cachedRemote, tmpdir]
-    git.whichAndExec(
-      args,
-      { cwd: cachedRemote, env: gitEnv() },
-      function (er, stdout, stderr) {
-        stdout = (stdout + '\n' + stderr).trim()
-        if (er) {
-          log.error('git ' + args.join(' ') + ':', stderr)
-          return cb(er)
-        }
-        log.verbose('addRemoteGit', 'clone', stdout)
+      checkoutTreeish(from, resolvedURL, resolvedTreeish, tmpdir, cb)
+    }
+  )
+}
 
-        checkoutTreeish(cb)
+// there is no safe way to do a one-step clone to a treeish that isn't
+// guaranteed to be a branch, so explicitly check out the treeish once it's
+// cloned
+function checkoutTreeish (from, resolvedURL, resolvedTreeish, tmpdir, cb) {
+  var args = ['checkout', resolvedTreeish]
+  git.whichAndExec(
+    args,
+    { cwd: tmpdir, env: gitEnv() },
+    function (er, stdout, stderr) {
+      stdout = (stdout + '\n' + stderr).trim()
+      if (er) {
+        log.error('git ' + args.join(' ') + ':', stderr)
+        return cb(er)
       }
-    )
-  }
+      log.verbose('checkoutTreeish', from, 'checkout', stdout)
 
-  // there is no safe way to do a one-step clone to a treeish that isn't
-  // guaranteed to be a branch, so explicitly check out the treeish once it's
-  // cloned
-  function checkoutTreeish (cb) {
-    var args = ['checkout', resolvedTreeish]
-    git.whichAndExec(
-      args,
-      { cwd: tmpdir, env: gitEnv() },
-      function (er, stdout, stderr) {
-        stdout = (stdout + '\n' + stderr).trim()
+      // convince addLocal that the checkout is a local dependency
+      realizePackageSpecifier(tmpdir, function (er, spec) {
         if (er) {
-          log.error('git ' + args.join(' ') + ':', stderr)
+          log.error('addRemoteGit', 'Failed to map', tmpdir, 'to a package specifier')
           return cb(er)
         }
-        log.verbose('addRemoteGit', 'checkout', stdout)
 
-        // convince addLocal that the checkout is a local dependency
-        realizePackageSpecifier(tmpdir, function (er, spec) {
-          if (er) {
-            log.error('addRemoteGit', 'Failed to map', tmpdir, 'to a package specifier')
-            return cb(er)
+        // ensure pack logic is applied
+        // https://github.com/npm/npm/issues/6400
+        addLocal(spec, null, function (er, data) {
+          if (data) {
+            log.silly('addRemoteGit', 'data._from:', from)
+            data._from = from
+
+            log.verbose('addRemoteGit', 'data._resolved:', resolvedURL)
+            data._resolved = resolvedURL
           }
 
-          // ensure pack logic is applied
-          // https://github.com/npm/npm/issues/6400
-          addLocal(spec, null, function (er, data) {
-            if (data) {
-              log.verbose('addRemoteGit', 'data._resolved:', resolvedURL)
-              data._resolved = resolvedURL
-
-              // the spec passed to addLocal is not what the user originally requested,
-              // so remap
-              // https://github.com/npm/npm/issues/7121
-              if (!data._fromGitHub) {
-                log.silly('addRemoteGit', 'data._from:', originalURL)
-                data._from = originalURL
-              } else {
-                log.silly('addRemoteGit', 'data._from:', data._from, '(GitHub)')
-              }
-            }
-
-            cb(er, data)
-          })
+          cb(er, data)
         })
-      }
-    )
-  }
+      })
+    }
+  )
 }
 
 function getGitDir (cb) {
@@ -336,7 +391,12 @@ function gitEnv () {
   // git responds to env vars in some weird ways in post-receive hooks
   // so don't carry those along.
   if (gitEnv_) return gitEnv_
-  gitEnv_ = {}
+
+  // allow users to override npm's insistence on not prompting for
+  // passphrases, but default to just failing when credentials
+  // aren't available
+  gitEnv_ = { GIT_ASKPASS: 'echo' }
+
   for (var k in process.env) {
     if (!~VALID_VARIABLES.indexOf(k) && k.match(/^GIT/)) continue
     gitEnv_[k] = process.env[k]
@@ -350,13 +410,12 @@ function getResolved (uri, treeish) {
   if (!/^git[+:]/.test(parsed.protocol)) {
     parsed.protocol = 'git+' + parsed.protocol
   }
-  var resolved = url.format(parsed)
 
   // node incorrectly sticks a / at the start of the path We know that the host
   // won't change, so split and detect this
   // https://github.com/npm/npm/issues/3224
   var spo = uri.split(parsed.host)
-  var spr = resolved.split(parsed.host)
+  var spr = url.format(parsed).split(parsed.host)
   if (spo[1] && spo[1].charAt(0) === ':' && spr[1] && spr[1].charAt(0) === '/') {
     spr[1] = spr[1].slice(1)
   }
diff --git a/lib/cache/maybe-github.js b/lib/cache/maybe-github.js
deleted file mode 100644
index 60c14a022..000000000
--- a/lib/cache/maybe-github.js
+++ /dev/null
@@ -1,35 +0,0 @@
-var assert = require('assert')
-
-var hosted = require('hosted-git-info')
-var log = require('npmlog')
-
-var addRemoteGit = require('./add-remote-git.js')
-
-module.exports = function maybeGithub (p, cb) {
-  assert(typeof p === 'string', 'must pass package name')
-  assert(typeof cb === 'function', 'must pass callback')
-
-  var parsed = hosted.fromUrl(p)
-  log.info('maybeGithub', 'Attempting %s from %s', p, parsed.git())
-
-  return addRemoteGit(parsed.git(), true, function (er, data) {
-    if (er) {
-      log.info('maybeGithub', "Couldn't clone %s", parsed.git())
-      log.info('maybeGithub', 'Now attempting %s from %s', p, parsed.sshurl())
-
-      return addRemoteGit(parsed.sshurl(), false, function (er, data) {
-        if (er) return cb(er)
-
-        success(parsed.sshurl(), data)
-      })
-    }
-
-    success(parsed.git(), data)
-  })
-
-  function success (u, data) {
-    data._from = u
-    data._fromGithub = true
-    return cb(null, data)
-  }
-}
diff --git a/test/tap/add-remote-git-file.js b/test/tap/add-remote-git-file.js
index fc102faac..b2a7be903 100644
--- a/test/tap/add-remote-git-file.js
+++ b/test/tap/add-remote-git-file.js
@@ -28,7 +28,7 @@ test('setup', function (t) {
 test('cache from repo', function (t) {
   process.chdir(pkg)
   var addRemoteGit = require('../../lib/cache/add-remote-git.js')
-  addRemoteGit(cloneURL, false, function (er, data) {
+  addRemoteGit(cloneURL, function (er, data) {
     t.ifError(er, 'cached via git')
     t.equal(
       url.parse(data._resolved).protocol,
diff --git a/test/tap/git-cache-locking.js b/test/tap/git-cache-locking.js
index 39f8b279c..e08c96e1b 100644
--- a/test/tap/git-cache-locking.js
+++ b/test/tap/git-cache-locking.js
@@ -1,48 +1,46 @@
-var test = require("tap").test
-  , common = require("../common-tap")
-  , path = require("path")
-  , rimraf = require("rimraf")
-  , mkdirp = require("mkdirp")
-  , pkg = path.resolve(__dirname, "git-cache-locking")
-  , tmp = path.join(pkg, "tmp")
-  , cache = path.join(pkg, "cache")
+var test = require('tap').test
+var common = require('../common-tap')
+var path = require('path')
+var rimraf = require('rimraf')
+var mkdirp = require('mkdirp')
+var pkg = path.resolve(__dirname, 'git-cache-locking')
+var tmp = path.join(pkg, 'tmp')
+var cache = path.join(pkg, 'cache')
 
-
-test("setup", function (t) {
+test('setup', function (t) {
   rimraf.sync(pkg)
-  mkdirp.sync(path.resolve(pkg, "node_modules"))
+  mkdirp.sync(path.resolve(pkg, 'node_modules'))
   t.end()
 })
 
-test("git-cache-locking: install a git dependency", function (t) {
-
+test('git-cache-locking: install a git dependency', function (t) {
   // disable git integration tests on Travis.
   if (process.env.TRAVIS) return t.end()
 
   // package c depends on a.git#master and b.git#master
   // package b depends on a.git#master
   common.npm([
-    "install",
-    "git://github.com/nigelzor/npm-4503-c.git"
+    'install',
+    'git://github.com/nigelzor/npm-4503-c.git'
   ], {
     cwd: pkg,
     env: {
-      "npm_config_cache": cache,
-      "npm_config_tmp": tmp,
-      "npm_config_prefix": pkg,
-      "npm_config_global": "false",
+      npm_config_cache: cache,
+      npm_config_tmp: tmp,
+      npm_config_prefix: pkg,
+      npm_config_global: 'false',
       HOME: process.env.HOME,
       Path: process.env.PATH,
       PATH: process.env.PATH
     }
-  }, function (err, code) {
-    t.ifErr(err, "npm install finished without error")
-    t.equal(0, code, "npm install should succeed")
+  }, function (err, code, stdout, stderr) {
+    t.ifErr(err, 'npm install finished without error')
+    t.equal(0, code, 'npm install should succeed')
     t.end()
   })
 })
 
-test("cleanup", function(t) {
+test('cleanup', function (t) {
   rimraf.sync(pkg)
   t.end()
 })
diff --git a/test/tap/github-shortcut.js b/test/tap/github-shortcut.js
index accc16f39..e95b8bdfb 100644
--- a/test/tap/github-shortcut.js
+++ b/test/tap/github-shortcut.js
@@ -4,8 +4,9 @@ var test = require('tap').test
 
 test('github-shortcut', function (t) {
   var cloneUrls = [
-    ['git://github.com/foo/private.git', 'github shortcuts try git:// first'],
-    ['ssh://git@github.com/foo/private.git', 'github shortcuts try ssh:// urls second']
+    ['git://github.com/foo/private.git', 'github shortcuts try git URLs first'],
+    ['git@github.com:foo/private.git', 'github shortcuts try SSH second'],
+    ['https://github.com/foo/private.git', 'github shortcuts try HTTPS URLs third']
   ]
   var npm = requireInject.installGlobally('../../lib/npm.js', {
     'child_process': {
diff --git a/test/tap/ls-l-depth-0.js b/test/tap/ls-l-depth-0.js
index 09298bdd7..5bbc1278c 100644
--- a/test/tap/ls-l-depth-0.js
+++ b/test/tap/ls-l-depth-0.js
@@ -19,7 +19,7 @@ var expected =
   '│ \n' +
   '└── glock@1.8.7\n' +
   '    an inexplicably hostile sample package\n' +
-  '    https://github.com/npm/glo.ck\n' +
+  '    git+https://github.com/npm/glo.ck.git\n' +
   '    https://glo.ck\n' +
   '\n'
 
diff --git a/test/tap/maybe-github.js b/test/tap/maybe-github.js
deleted file mode 100644
index 52a62e11b..000000000
--- a/test/tap/maybe-github.js
+++ /dev/null
@@ -1,72 +0,0 @@
-require("../common-tap.js")
-var test = require("tap").test
-var npm = require("../../lib/npm.js")
-
-// this is the narrowest way to replace a function in the module cache
-var found = true
-var remoteGitPath = require.resolve("../../lib/cache/add-remote-git.js")
-require("module")._cache[remoteGitPath] = {
-  id: remoteGitPath,
-  exports: function stub(_, __, cb) {
-    if (found) {
-      cb(null, {})
-    }
-    else {
-      cb(new Error("not on filesystem"))
-    }
-  }
-}
-
-// only load maybeGithub now, so it gets the stub from cache
-var maybeGithub = require("../../lib/cache/maybe-github.js")
-
-test("should throw with no parameters", function (t) {
-  t.plan(1)
-
-  t.throws(function () {
-    maybeGithub()
-  }, "throws when called without parameters")
-})
-
-test("should throw with wrong parameter types", function (t) {
-  t.plan(2)
-
-  t.throws(function () {
-    maybeGithub({}, function () {})
-  }, "expects only a package name")
-
-  t.throws(function () {
-    maybeGithub("npm/xxx-noexist", "ham")
-  }, "is always async")
-})
-
-test("should find an existing package on Github", function (t) {
-  found = true
-  npm.load({}, function (error) {
-    t.notOk(error, "bootstrapping succeeds")
-    t.doesNotThrow(function () {
-      maybeGithub("npm/npm", function (error, data) {
-        t.notOk(error, "no issues in looking things up")
-        t.ok(data, "received metadata from Github")
-        t.end()
-      })
-    })
-  })
-})
-
-test("shouldn't find a nonexistent package on Github", function (t) {
-  found = false
-  npm.load({}, function () {
-    t.doesNotThrow(function () {
-      maybeGithub("npm/xxx-noexist", function (error, data) {
-        t.equal(
-          error.message,
-          "not on filesystem",
-          "passed through original error message"
-        )
-        t.notOk(data, "didn't pass any metadata")
-        t.end()
-      })
-    })
-  })
-})
diff --git a/test/tap/outdated-git.js b/test/tap/outdated-git.js
index 32bfead67..0ef243bee 100644
--- a/test/tap/outdated-git.js
+++ b/test/tap/outdated-git.js
@@ -11,6 +11,18 @@ var npm = require('../../')
 // config
 var pkg = path.resolve(__dirname, 'outdated-git')
 var cache = path.resolve(pkg, 'cache')
+var json = {
+  name: 'outdated-git',
+  author: 'Rocko Artischocko',
+  description: 'fixture',
+  version: '0.0.1',
+  main: 'index.js',
+  dependencies: {
+    'foo-private': 'git://github.com/robertkowalski/foo-private.git',
+    'foo-private-credentials': 'git://user:pass@github.com/robertkowalski/foo-private.git',
+    'foo-github': 'robertkowalski/foo'
+  }
+}
 
 test('setup', function (t) {
   setup()
@@ -22,11 +34,11 @@ test('discovers new versions in outdated', function (t) {
   t.plan(5)
   npm.load({cache: cache, registry: common.registry, loglevel: 'silent'}, function () {
     npm.commands.outdated([], function (er, d) {
-      t.equal('git', d[0][3])
-      t.equal('git', d[0][4])
-      t.equal('git://github.com/robertkowalski/foo-private.git', d[0][5])
-      t.equal('git://user:pass@github.com/robertkowalski/foo-private.git', d[1][5])
-      t.equal('git+https://github.com/robertkowalski/foo', d[2][5])
+      t.equal(d[0][3], 'git')
+      t.equal(d[0][4], 'git')
+      t.equal(d[0][5], 'git://github.com/robertkowalski/foo-private.git')
+      t.equal(d[1][5], 'git://user:pass@github.com/robertkowalski/foo-private.git')
+      t.equal(d[2][5], 'github:robertkowalski/foo')
     })
   })
 })
@@ -36,19 +48,6 @@ test('cleanup', function (t) {
   t.end()
 })
 
-var json = {
-  name: 'outdated-git',
-  author: 'Rocko Artischocko',
-  description: 'fixture',
-  version: '0.0.1',
-  main: 'index.js',
-  dependencies: {
-    'foo-private': 'git://github.com/robertkowalski/foo-private.git',
-    'foo-private-credentials': 'git://user:pass@github.com/robertkowalski/foo-private.git',
-    'foo-github': 'robertkowalski/foo'
-  }
-}
-
 function setup () {
   mkdirp.sync(cache)
   fs.writeFileSync(path.join(pkg, 'package.json'), JSON.stringify(json, null, 2), 'utf8')