fix: work better with system manpages (#4610)

In certain edge cases, the glob could find files that manNumberRegex wouldn't match.

A possible solution would be to try to bring the two closer, but since globs and regexes are different syntaxes, the two will never be exactly the same.
It's always asking for bugs; better to just handle the issue directly.

In addition, when npm is installed globally in a system directory, the
glob can find other manpages with similar names.
For instance "install.1", "init.1", etc. Preferring low-numbered
sections isn't enough in these cases; it's also necessary to prefer
the pages prefixed with "npm-".

Author: d0sboots

lib/commands/help.js

@@ -11,6 +11,8 @@ const BaseCommand = require('../base-command.js')
 // We don't currently compress our man pages but if we ever did this would
 // seemlessly continue supporting it
 const manNumberRegex = /\.(\d+)(\.[^/\\]*)?$/
+// Searches for the "npm-" prefix in page names, to prefer those.
+const manNpmPrefixRegex = /\/npm-/
 
 class Help extends BaseCommand {
   static description = 'Get help on npm'
@@ -61,13 +63,27 @@ class Help extends BaseCommand {
     const f = `${manroot}/${manSearch}/?(npm-)${section}.[0-9]*`
     let mans = await glob(f)
     mans = mans.sort((a, b) => {
-      // Because of the glob we know the manNumberRegex will pass
-      const aManNumber = a.match(manNumberRegex)[1]
-      const bManNumber = b.match(manNumberRegex)[1]
+      // Prefer the page with an npm prefix, if there's only one.
+      const aHasPrefix = manNpmPrefixRegex.test(a)
+      const bHasPrefix = manNpmPrefixRegex.test(b)
+      if (aHasPrefix !== bHasPrefix) {
+        return aHasPrefix ? -1 : 1
+      }
 
-      // man number sort first so that 1 aka commands are preferred
-      if (aManNumber !== bManNumber) {
-        return aManNumber - bManNumber
+      // Because the glob is (subtly) different from manNumberRegex,
+      // we can't rely on it passing.
+      const aManNumberMatch = a.match(manNumberRegex)
+      const bManNumberMatch = b.match(manNumberRegex)
+      if (aManNumberMatch) {
+        if (!bManNumberMatch) {
+          return -1
+        }
+        // man number sort first so that 1 aka commands are preferred
+        if (aManNumberMatch[1] !== bManNumberMatch[1]) {
+          return aManNumberMatch[1] - bManNumberMatch[1]
+        }
+      } else if (bManNumberMatch) {
+        return 1
       }
 
       return localeCompare(a, b)

test/lib/commands/help.js

@@ -310,3 +310,41 @@ t.test('npm help with complex installation path finds proper help file', async t
 
   t.match(openUrlArg, /commands(\/|\\)npm-install.html$/, 'attempts to open the correct url')
 })
+
+t.test('npm help - prefers npm help pages', async t => {
+  // Unusual ordering is to get full test coverage of all branches inside the
+  // sort function.
+  globResult = [
+    '/root/man/man6/npm-install.6',
+    '/root/man/man1/install.1',
+    '/root/man/man5/npm-install.5',
+  ]
+  t.teardown(() => {
+    globResult = globDefaults
+    spawnBin = null
+    spawnArgs = null
+  })
+  await help.exec(['install'])
+
+  t.equal(spawnBin, 'man', 'calls man by default')
+  t.strictSame(spawnArgs, ['/root/man/man5/npm-install.5'], 'passes the correct arguments')
+})
+
+t.test('npm help - works in the presence of strange man pages', async t => {
+  // Unusual ordering is to get full test coverage of all branches inside the
+  // sort function.
+  globResult = [
+    '/root/man/man6/config.6strange',
+    '/root/man/man1/config.1',
+    '/root/man/man5/config.5ssl',
+  ]
+  t.teardown(() => {
+    globResult = globDefaults
+    spawnBin = null
+    spawnArgs = null
+  })
+  await help.exec(['config'])
+
+  t.equal(spawnBin, 'man', 'calls man by default')
+  t.strictSame(spawnArgs, ['/root/man/man1/config.1'], 'passes the correct arguments')
+})