badly-named man files won't install (fixes #7000)

Before, if you tried to put a filename in the `man` stanza of
package.json that didn't follow the rules as listed in `npm help 5
package.json`, installing would work, but upgrading or uninstalling
would fail with a cryptic failure. Now, fail on build with a descriptive
message, and log the same descriptive message as an error (but don't
fail) on unbuild.

Author: othiym23

lib/build.js

@@ -212,6 +212,7 @@ function linkMans (pkg, folder, parent, gtop, cb) {
   if (!pkg.man || !gtop || process.platform === "win32") return cb()
 
   var manRoot = path.resolve(npm.config.get("prefix"), "share", "man")
+  log.verbose("linkMans", "man files are", pkg.man, "in", manRoot)
 
   // make sure that the mans are unique.
   // otherwise, if there are dupes, it'll fail with EEXIST
@@ -225,11 +226,20 @@ function linkMans (pkg, folder, parent, gtop, cb) {
 
   asyncMap(pkg.man, function (man, cb) {
     if (typeof man !== "string") return cb()
+    log.silly("linkMans", "preparing to link", man)
     var parseMan = man.match(/(.*\.([0-9]+)(\.gz)?)$/)
-      , stem = parseMan[1]
-      , sxn = parseMan[2]
-      , bn = path.basename(stem)
-      , manDest = path.join(manRoot, "man" + sxn, bn)
+    if (!parseMan) {
+      return cb(new Error(
+        man+" is not a valid name for a man file.  " +
+        "Man files must end with a number, " +
+        "and optionally a .gz suffix if they are compressed."
+      ))
+    }
+
+    var stem = parseMan[1]
+    var sxn = parseMan[2]
+    var bn = path.basename(stem)
+    var manDest = path.join(manRoot, "man" + sxn, bn)
 
     linkIfExists(man, manDest, gtop && folder, cb)
   }, cb)

lib/unbuild.js

@@ -81,25 +81,35 @@ function rmMans (pkg, folder, parent, top, cb) {
     return cb()
   }
   var manRoot = path.resolve(npm.config.get("prefix"), "share", "man")
+  log.verbose("rmMans", "man files are", pkg.man, "in", manRoot)
   asyncMap(pkg.man, function (man, cb) {
     if (Array.isArray(man)) {
       man.forEach(rmMan)
     } else {
       rmMan(man)
     }
 
-    function rmMan(man) {
-      var parseMan = man.match(/(.*)\.([0-9]+)(\.gz)?$/)
-        , stem = parseMan[1]
-        , sxn = parseMan[2]
-        , gz = parseMan[3] || ""
-        , bn = path.basename(stem)
-        , manDest = path.join( manRoot
-                            , "man"+sxn
-                            , (bn.indexOf(pkg.name) === 0 ? bn
-                              : pkg.name + "-" + bn)
-                              + "." + sxn + gz
-                            )
+    function rmMan (man) {
+      log.silly("rmMan", "preparing to remove", man)
+      var parseMan = man.match(/(.*\.([0-9]+)(\.gz)?)$/)
+      if (!parseMan) {
+        log.error(
+          "rmMan", man, "is not a valid name for a man file.",
+          "Man files must end with a number, " +
+          "and optionally a .gz suffix if they are compressed."
+        )
+        return cb()
+      }
+
+      var stem = parseMan[1]
+      var sxn = parseMan[2]
+      var gz = parseMan[3] || ""
+      var bn = path.basename(stem)
+      var manDest = path.join(
+        manRoot,
+        "man"+sxn,
+        (bn.indexOf(pkg.name) === 0 ? bn : pkg.name+"-"+bn)+"."+sxn+gz
+      )
       gentlyRm(manDest, true, cb)
     }
   }, cb)

test/tap/install-bad-man.js

@@ -0,0 +1,79 @@
+var fs = require("fs")
+var resolve = require("path").resolve
+
+var osenv = require("osenv")
+var mkdirp = require("mkdirp")
+var rimraf = require("rimraf")
+var test = require("tap").test
+
+var common = require("../common-tap.js")
+
+var pkg = resolve(__dirname, "install-bad-man")
+var target = resolve(__dirname, "install-bad-man-target")
+
+var EXEC_OPTS = {
+  cwd: target
+}
+
+test("setup", function (t) {
+  setup()
+  t.pass("setup ran")
+  t.end()
+})
+
+test("install from repo on 'OS X'", function (t) {
+  common.npm(
+    [
+      "install",
+      "--prefix", target,
+      "--global",
+      pkg
+    ],
+    EXEC_OPTS,
+    function (err, code, stdout, stderr) {
+      t.ifError(err, "npm command ran from test")
+      t.equals(code, 1, "install exited with failure (1)")
+      t.notOk(stdout, "no output indicating success")
+      t.notOk(
+        stderr.match(/Cannot read property '1' of null/),
+        "no longer has cryptic error output"
+      )
+      t.ok(
+        stderr.match(/install-bad-man\.1\.lol is not a valid name/),
+        "got expected error output"
+      )
+
+      t.end()
+    }
+  )
+})
+
+test("clean", function (t) {
+  cleanup()
+  t.pass("cleaned up")
+  t.end()
+})
+
+var json = {
+  name    : "install-bad-man",
+  version : "1.2.3",
+  man     : [ "./install-bad-man.1.lol" ]
+}
+
+function setup () {
+  cleanup()
+  mkdirp.sync(pkg)
+  // make sure it installs locally
+  mkdirp.sync(resolve(target, "node_modules"))
+  fs.writeFileSync(
+    resolve(pkg, "package.json"),
+    JSON.stringify(json, null, 2)+"\n"
+  )
+  fs.writeFileSync(resolve(pkg, "install-bad-man.1.lol"), "lol\n")
+}
+
+function cleanup () {
+  process.chdir(osenv.tmpdir())
+  rimraf.sync(pkg)
+  rimraf.sync(target)
+}