refactor maybe* methods to be more consistent

5 files changed, 125 insertions, 75 deletions

diff --git a/lib/cache.js b/lib/cache.js
index 1ff8718ba..8038e222f 100644
--- a/lib/cache.js
+++ b/lib/cache.js
@@ -127,6 +127,7 @@ function read (name, ver, forceBypass, cb) {
   var jsonFile = path.join(npm.cache, name, ver, "package", "package.json")
   function c (er, data) {
     if (data) deprCheck(data)
+
     return cb(er, data)
   }
 
@@ -140,7 +141,9 @@ function read (name, ver, forceBypass, cb) {
     er = needVersion(er, data)
     if (er && er.code !== "ENOENT" && er.code !== "ENOTDIR") return cb(er)
     if (er) return addNamed(name, ver, null, c)
+
     deprCheck(data)
+
     c(er, data)
   })
 }
@@ -251,15 +254,27 @@ function add (args, cb) {
   // in that case, we will not have a protocol now, but if we
   // split and check, we will.
   if (!name && !p.protocol) {
-    if (spec.indexOf("/") !== -1 ||
-        process.platform === "win32" && spec.indexOf("\\") !== -1) {
-      return maybeFile(spec, p, cb)
-    } else if (spec.indexOf("@") !== -1) {
-      return maybeAt(spec, cb)
+    return maybeFile(spec, cb)
+  }
+  else {
+    switch (p.protocol) {
+      case "http:":
+      case "https:":
+        return addRemoteTarball(spec, null, name, "", cb)
+
+      default:
+        if (isGitUrl(p)) return addRemoteGit(spec, p, false, cb)
+
+        // if we have a name and a spec, then try name@spec
+        if (name) {
+          addNamed(name, spec, null, cb)
+        }
+        // if not, then try just spec (which may try name@"" if not found)
+        else {
+          addLocal(spec, "", cb)
+        }
     }
   }
-
-  add_(name, spec, p, cb)
 }
 
 function unpack (pkg, ver, unpackTarget, dMode, fMode, uid, gid, cb) {
@@ -284,26 +299,6 @@ function unpack (pkg, ver, unpackTarget, dMode, fMode, uid, gid, cb) {
   })
 }
 
-function add_ (name, spec, p, cb) {
-  switch (p.protocol) {
-    case "http:":
-    case "https:":
-      return addRemoteTarball(spec, null, name, "", cb)
-
-    default:
-      if (isGitUrl(p))
-        return addRemoteGit(spec, p, false, cb)
-
-      // if we have a name and a spec, then try name@spec
-      // if not, then try just spec (which may try name@"" if not found)
-      if (name) {
-        addNamed(name, spec, null, cb)
-      } else {
-        addLocal(spec, "", cb)
-      }
-  }
-}
-
 function afterAdd (arg, cb) { return function (er, data) {
   if (er || !data || !data.name || !data.version) {
     return cb(er, data)
@@ -319,29 +314,30 @@ function afterAdd (arg, cb) { return function (er, data) {
   })
 }}
 
-function maybeFile (spec, p, cb) {
+function maybeFile (spec, cb) {
+  // split name@2.3.4 only if name is a valid package name,
+  // don't split in case of "./test@example.com/" (local path)
   fs.stat(spec, function (er) {
     if (!er) {
       // definitely a local thing
-      addLocal(spec, "", cb)
-    } else if (er && spec.indexOf("@") !== -1) {
-      // bar@baz/loofa
-      maybeAt(spec, cb)
-    } else {
-      // Already know it's not a url, so must be local
-      addLocal(spec, "", cb)
+      return addLocal(spec, "", cb)
     }
+
+    maybeAt(spec, cb)
   })
 }
 
 function maybeAt (spec, cb) {
-  var tmp = spec.split("@")
-
-  // split name@2.3.4 only if name is a valid package name,
-  // don't split in case of "./test@example.com/" (local path)
-  var name = tmp.shift()
-  spec = tmp.join("@")
-  return add([name, spec], cb)
+  if (spec.indexOf("@") !== -1) {
+    var tmp = spec.split("@")
+
+    var name = tmp.shift()
+    spec = tmp.join("@")
+    add([name, spec], cb)
+  } else {
+    // already know it's not a url, so must be local
+    addLocal(spec, "", cb)
+  }
 }
 
 function needName(er, data) {
diff --git a/lib/cache/add-local.js b/lib/cache/add-local.js
index 1bb6a6dbe..7b5923574 100644
--- a/lib/cache/add-local.js
+++ b/lib/cache/add-local.js
@@ -52,7 +52,7 @@ function addLocal (p, name, cb_) {
         // might be username/project
         // in that case, try it as a github url.
         if (p.split("/").length === 2) {
-          return maybeGithub(p, name, er, cb)
+          return maybeGithub(p, er, cb)
         }
         return cb(er)
       }
diff --git a/lib/cache/add-named.js b/lib/cache/add-named.js
index 2ef06eda1..caeb7cc24 100644
--- a/lib/cache/add-named.js
+++ b/lib/cache/add-named.js
@@ -15,8 +15,8 @@ var path = require("path")
   , locker = require("../utils/locker.js")
   , lock = locker.lock
   , unlock = locker.unlock
+  , maybeGithub = require("./maybe-github.js")
   , addRemoteTarball = require("./add-remote-tarball.js")
-  , addRemoteGit = require("./add-remote-git.js")
 
 
 module.exports = addNamed
@@ -62,7 +62,7 @@ function addNameTag (name, tag, data, cb_) {
     // might be username/project
     // in that case, try it as a github url.
     if (er && tag.split("/").length === 2) {
-      return maybeGithub(tag, name, er, cb_)
+      return maybeGithub(tag, er, cb_)
     }
     return cb_(er, data)
   }
@@ -239,33 +239,6 @@ function addNameRange (name, range, data, cb) {
   }
 }
 
-function maybeGithub (p, name, er, cb) {
-  var u = "git://github.com/" + p
-    , up = url.parse(u)
-  log.info("maybeGithub", "Attempting %s from %s", p, u)
-
-  return addRemoteGit(u, up, true, function (er2, data) {
-    if (er2) {
-      var upriv = "git+ssh://git@github.com:" + p
-        , uppriv = url.parse(upriv)
-
-      log.info("maybeGithub", "Attempting %s from %s", p, upriv)
-
-      return addRemoteGit(upriv, uppriv, false, function (er3, data) {
-        if (er3) return cb(er)
-        success(upriv, data)
-      })
-    }
-    success(u, data)
-  })
-
-  function success (u, data) {
-    data._from = u
-    data._fromGithub = true
-    return cb(null, data)
-  }
-}
-
 function installTargetsError (requested, data) {
   var targets = Object.keys(data["dist-tags"]).filter(function (f) {
     return (data.versions || {}).hasOwnProperty(f)
diff --git a/lib/cache/maybe-github.js b/lib/cache/maybe-github.js
index d3f54eb12..fd04eadc8 100644
--- a/lib/cache/maybe-github.js
+++ b/lib/cache/maybe-github.js
@@ -1,10 +1,15 @@
 'use strict';
 
-var log = require("npmlog")
-  , url = require("url")
+var url = require("url")
+  , assert = require("assert")
+  , log = require("npmlog")
   , addRemoteGit = require("./add-remote-git.js")
 
-module.exports = function maybeGithub (p, name, er, cb) {
+module.exports = function maybeGithub (p, er, cb) {
+  assert(typeof p === "string", "must pass package name")
+  assert(er instanceof Error, "must include error")
+  assert(typeof cb === "function", "must pass callback")
+
   var u = "git://github.com/" + p
     , up = url.parse(u)
   log.info("maybeGithub", "Attempting %s from %s", p, u)
diff --git a/test/tap/maybe-github.js b/test/tap/maybe-github.js
new file mode 100644
index 000000000..8b7105e6e
--- /dev/null
+++ b/test/tap/maybe-github.js
@@ -0,0 +1,76 @@
+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(_, error, __, cb) {
+    if (found) {
+      cb(null, {})
+    }
+    else {
+      cb(error)
+    }
+  }
+}
+
+// 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(3)
+
+  t.throws(function () {
+    maybeGithub({}, new Error(), function () {})
+  }, "expects only a package name")
+
+  t.throws(function () {
+    maybeGithub("npm/xxx-noexist", null, function () {})
+  }, "expects to be called after a previous check already failed")
+
+  t.throws(function () {
+    maybeGithub("npm/xxx-noexist", new Error(), "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", new Error("not on filesystem"), 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", new Error("not on filesystem"), 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()
+      })
+    })
+  })
+})