From f4001ecb0a6a7136cbd7b04e89a75688bfc62d49 Mon Sep 17 00:00:00 2001
From: David Walker <d0sboots@gmail.com>
Date: Thu, 24 Mar 2022 02:10:21 -0700
Subject: [PATCH] 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-".
---
 lib/commands/help.js      | 28 ++++++++++++++++++++++------
 test/lib/commands/help.js | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/lib/commands/help.js b/lib/commands/help.js
index 40f5ad9b30092..266b7e40a53f7 100644
--- a/lib/commands/help.js
+++ b/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 = a.match(manNpmPrefixRegex)
+      const bHasPrefix = b.match(manNpmPrefixRegex)
+      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)
diff --git a/test/lib/commands/help.js b/test/lib/commands/help.js
index b76234d996627..f84f94ad2f8d9 100644
--- a/test/lib/commands/help.js
+++ 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')
+})