From ab042a936fa32e109e24651314414b57a4a03d08 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sun, 15 Apr 2018 21:41:36 +0200
Subject: [PATCH 1/3] process: add release.compareVersion(str) functionality

This overloads `release.compareVersion` by also accepting a single
string argument to compare the version. It also fixes a former bug
by comparing the pre-release tags properly.

It also adds a stricter input validation to throw on negative values.
---
 doc/api/process.md                    |  12 +++
 lib/internal/process.js               |  55 +++++++++++---
 test/parallel/test-process-release.js | 101 ++++++++++++++++++++------
 3 files changed, 133 insertions(+), 35 deletions(-)

diff --git a/doc/api/process.md b/doc/api/process.md
index 060d741804df54..65b0016a1514ee 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -1554,6 +1554,18 @@ Perform a SemVer comparison to the release version.
   version, `0` if the given version matches the process version, and `-1`
   if the given version is greater than the release version.
 
+## process.release.compareVersion(version)
+<!-- YAML
+added: REPLACEME
+-->
+
+Perform a SemVer comparison to the release version.
+
+* `version` {string} The semver version to compare.
+* Returns: {number} `1` if the given version is lower than the current release
+  version, `0` if the given version matches the process version, and `-1`
+  if the given version is greater than the release version.
+
 ## process.send(message[, sendHandle[, options]][, callback])
 <!-- YAML
 added: v0.5.9
diff --git a/lib/internal/process.js b/lib/internal/process.js
index 027657991a097b..ec6af64d713caf 100644
--- a/lib/internal/process.js
+++ b/lib/internal/process.js
@@ -6,8 +6,10 @@ const {
     ERR_ASSERTION,
     ERR_CPU_USAGE,
     ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_ARG_VALUE,
     ERR_INVALID_ARRAY_LENGTH,
     ERR_INVALID_OPT_VALUE,
+    ERR_OUT_OF_RANGE,
     ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET,
     ERR_UNKNOWN_SIGNAL
   }
@@ -291,15 +293,43 @@ function setupCompareVersion() {
     prereleaseTag,
   } = process.release;
 
-  process.release.compareVersion = (major, minor, patch, tag) => {
-    if (typeof major !== 'number')
-      throw new ERR_INVALID_ARG_TYPE('major', 'number', major);
-    if (typeof minor !== 'number')
-      throw new ERR_INVALID_ARG_TYPE('minor', 'number', minor);
-    if (typeof patch !== 'number')
-      throw new ERR_INVALID_ARG_TYPE('patch', 'number', patch);
-    if (tag !== undefined && typeof tag !== 'string')
-      throw new ERR_INVALID_ARG_TYPE('tag', 'string', tag);
+  process.release.compareVersion = function(major, minor, patch, tag) {
+    if (arguments.length === 1 && typeof major === 'string') {
+      const [start, ...end] = major.split('-');
+      tag = end.join('-');
+      const chunks = start.split('.');
+      if (chunks.length !== 3) {
+        throw new ERR_INVALID_ARG_VALUE('version',
+                                        major,
+                                        'is not a valid Semantic Version');
+      }
+      [major, minor, patch] = chunks.map((chunk) => {
+        const num = Number(chunk);
+        if (Number.isNaN(num) ||
+            chunk === '' ||
+            chunk.length > 1 && chunk.startsWith('0')) {
+          throw new ERR_INVALID_ARG_VALUE('version',
+                                          major,
+                                          'is not a valid Semantic Version');
+        }
+        return num;
+      });
+    } else {
+      if (typeof major !== 'number')
+        throw new ERR_INVALID_ARG_TYPE('major', 'number', major);
+      if (typeof minor !== 'number')
+        throw new ERR_INVALID_ARG_TYPE('minor', 'number', minor);
+      if (typeof patch !== 'number')
+        throw new ERR_INVALID_ARG_TYPE('patch', 'number', patch);
+      if (tag !== undefined && typeof tag !== 'string')
+        throw new ERR_INVALID_ARG_TYPE('tag', 'string', tag);
+    }
+    if (major < 0)
+      throw new ERR_OUT_OF_RANGE('major', '>= 0', major);
+    if (minor < 0)
+      throw new ERR_OUT_OF_RANGE('minor', '>= 0', minor);
+    if (patch < 0)
+      throw new ERR_OUT_OF_RANGE('patch', '>= 0', patch);
 
     if (major > majorVersion)
       return -1;
@@ -315,8 +345,11 @@ function setupCompareVersion() {
       return -1;
     if (patch < patchVersion)
       return 1;
-    if (prereleaseTag)
-      return prereleaseTag === tag ? 0 : 1;
+    if (prereleaseTag) {
+      if (prereleaseTag === tag)
+        return 0;
+      return prereleaseTag < tag ? -1 : 1;
+    }
     if (tag)
       return -1;
 
diff --git a/test/parallel/test-process-release.js b/test/parallel/test-process-release.js
index a7c8e8fa09508f..0179fdce880578 100644
--- a/test/parallel/test-process-release.js
+++ b/test/parallel/test-process-release.js
@@ -1,13 +1,13 @@
 'use strict';
 
-const common = require('../common');
+require('../common');
 
 const assert = require('assert');
 const versionParts = process.versions.node.split('.');
 
 assert.strictEqual(process.release.name, 'node');
 
-// it's expected that future LTS release lines will have additional
+// It's expected that future LTS release lines will have additional
 // branches in here
 if (versionParts[0] === '4' && versionParts[1] >= 2) {
   assert.strictEqual(process.release.lts, 'Argon');
@@ -27,31 +27,84 @@ const {
   compareVersion,
 } = process.release;
 
-assert.strictEqual(compareVersion(major, minor, patch, tag), 0);
+[
+  [major, minor, patch, tag, 0],
+  [major + 1, minor, patch, tag, -1],
+  [major - 1, minor, patch, tag, 1],
+  [major, minor + 1, patch, tag, -1],
+  [major, minor - 1, patch, tag, 1],
+  [major, minor, patch + 1, tag, -1],
+  [major, minor, patch - 1, tag, 1]
+].forEach(([major, minor, patch, tag, expected]) => {
+  // Skip invalid entries.
+  if (major < 0 || minor < 0 || patch < 0)
+    return;
+  assert.strictEqual(compareVersion(major, minor, patch, tag), expected);
+  const semverStr = `${major}.${minor}.${patch}${tag ? `-${tag}` : ''}`;
+  assert.strictEqual(compareVersion(semverStr), expected);
+});
 
-assert.strictEqual(compareVersion(major + 1, minor, patch, tag), -1);
-assert.strictEqual(compareVersion(major - 1, minor, patch, tag), 1);
-
-assert.strictEqual(compareVersion(major, minor + 1, patch, tag), -1);
-assert.strictEqual(compareVersion(major, minor - 1, patch, tag), 1);
-
-assert.strictEqual(compareVersion(major, minor, patch + 1, tag), -1);
-assert.strictEqual(compareVersion(major, minor, patch - 1, tag), 1);
-
-if (tag)
+if (tag) {
   assert.strictEqual(compareVersion(major, minor, patch), 1);
-else
-  assert.strictEqual(compareVersion(major, minor, patch, 'notrealtag'), -1);
-
-for (const args of [
-  ['', 0, 0, ''],
-  [0, '', 0, ''],
-  [0, 0, '', ''],
-  [0, 0, 0, 0],
-]) {
-  common.expectsError(() => {
+  assert.strictEqual(compareVersion(`${major}.${minor}.${patch}`), 1);
+  assert.strictEqual(compareVersion(major, minor, patch, `${tag}-fake`), -1);
+  assert.strictEqual(
+    compareVersion(major, minor, patch, `${tag.slice(0, -1)}`), 1);
+} else {
+  assert.strictEqual(compareVersion(major, minor, patch, 'fake-tag'), -1);
+  assert.strictEqual(compareVersion(`${major}.${minor}.${patch}-fake-tag`), -1);
+  assert.strictEqual(compareVersion(major, minor, patch), 0);
+}
+
+[
+  [['1', 0, 0, ''], 'major', 'string'],
+  [[0, '1', 0, ''], 'minor', 'string'],
+  [[0, 0, '1', ''], 'patch', 'string'],
+  [[0, 0, 0, 1], 'tag', 'number'],
+  [[0, 0, 0, null], 'tag', 'object'],
+].forEach(([args, argName, receivedType]) => {
+  const expectedType = argName === 'tag' ? 'string' : 'number';
+  assert.throws(() => {
     compareVersion(...args);
   }, {
     code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError [ERR_INVALID_ARG_TYPE]',
+    message: `The "${argName}" argument must be of type ${expectedType}. ` +
+             `Received type ${receivedType}`
   });
-}
+});
+
+[
+  [-1, 0, 0, 'major'],
+  [0, -1, 0, 'minor'],
+  [0, 0, -1, 'patch'],
+].forEach(([major, minor, patch, expected]) => {
+  assert.throws(() => {
+    compareVersion(major, minor, patch);
+  }, {
+    code: 'ERR_OUT_OF_RANGE',
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
+    message: `The value of "${expected}" is out of range. ` +
+             'It must be >= 0. Received -1'
+  });
+});
+
+[
+  '1a.0.0',
+  '1.0a.0',
+  '1.0.0a',
+  '1.0.0.0',
+  '01.0.0',
+  '1.01.0',
+  '1.0',
+  '1..0',
+].forEach((version) => {
+  assert.throws(() => {
+    compareVersion(version);
+  }, {
+    code: 'ERR_INVALID_ARG_VALUE',
+    name: 'TypeError [ERR_INVALID_ARG_VALUE]',
+    message: "The argument 'version' is not a valid Semantic Version. " +
+             `Received '${version}'`
+  });
+});

From ea130bc85644393c592020dff9e0b5f5963933af Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Mon, 16 Apr 2018 02:12:02 +0200
Subject: [PATCH 2/3] fixup: address comments

---
 doc/api/process.md                    |  8 ++---
 lib/internal/process.js               | 50 ++++++++++++---------------
 test/parallel/test-process-release.js | 15 ++++++++
 3 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/doc/api/process.md b/doc/api/process.md
index 65b0016a1514ee..2ff69ab8a35e1c 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -1544,8 +1544,6 @@ should not be relied upon to exist.
 added: REPLACEME
 -->
 
-Perform a SemVer comparison to the release version.
-
 * `major` {number} The major version to compare.
 * `minor` {number} The minor version to compare.
 * `patch` {number} The patch version to compare.
@@ -1554,18 +1552,20 @@ Perform a SemVer comparison to the release version.
   version, `0` if the given version matches the process version, and `-1`
   if the given version is greater than the release version.
 
+Perform a SemVer comparison to the release version.
+
 ## process.release.compareVersion(version)
 <!-- YAML
 added: REPLACEME
 -->
 
-Perform a SemVer comparison to the release version.
-
 * `version` {string} The semver version to compare.
 * Returns: {number} `1` if the given version is lower than the current release
   version, `0` if the given version matches the process version, and `-1`
   if the given version is greater than the release version.
 
+Perform a SemVer comparison to the release version.
+
 ## process.send(message[, sendHandle[, options]][, callback])
 <!-- YAML
 added: v0.5.9
diff --git a/lib/internal/process.js b/lib/internal/process.js
index ec6af64d713caf..75a3716043209d 100644
--- a/lib/internal/process.js
+++ b/lib/internal/process.js
@@ -293,43 +293,37 @@ function setupCompareVersion() {
     prereleaseTag,
   } = process.release;
 
+  function validateNum(value, name) {
+    if (!Number.isInteger(value)) {
+      if (typeof value !== 'number')
+        throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
+      throw new ERR_OUT_OF_RANGE(name, 'an integer', value);
+    }
+    // Always accept 0 as input even though it does not adhere to semver because
+    // the first Node.js versions did not use the semver notation.
+    if (value < 0)
+      throw new ERR_OUT_OF_RANGE(name, '>= 0', value);
+  }
+
+  // The tag is not validated here.
+  const semverRegExp = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([0-9A-Za-z.-]+))*?$/;
+
   process.release.compareVersion = function(major, minor, patch, tag) {
     if (arguments.length === 1 && typeof major === 'string') {
-      const [start, ...end] = major.split('-');
-      tag = end.join('-');
-      const chunks = start.split('.');
-      if (chunks.length !== 3) {
+      const match = major.match(semverRegExp);
+      if (match === null) {
         throw new ERR_INVALID_ARG_VALUE('version',
                                         major,
                                         'is not a valid Semantic Version');
       }
-      [major, minor, patch] = chunks.map((chunk) => {
-        const num = Number(chunk);
-        if (Number.isNaN(num) ||
-            chunk === '' ||
-            chunk.length > 1 && chunk.startsWith('0')) {
-          throw new ERR_INVALID_ARG_VALUE('version',
-                                          major,
-                                          'is not a valid Semantic Version');
-        }
-        return num;
-      });
+      [, major, minor, patch, tag] = match;
     } else {
-      if (typeof major !== 'number')
-        throw new ERR_INVALID_ARG_TYPE('major', 'number', major);
-      if (typeof minor !== 'number')
-        throw new ERR_INVALID_ARG_TYPE('minor', 'number', minor);
-      if (typeof patch !== 'number')
-        throw new ERR_INVALID_ARG_TYPE('patch', 'number', patch);
+      validateNum(major, 'major');
+      validateNum(minor, 'minor');
+      validateNum(patch, 'patch');
       if (tag !== undefined && typeof tag !== 'string')
         throw new ERR_INVALID_ARG_TYPE('tag', 'string', tag);
     }
-    if (major < 0)
-      throw new ERR_OUT_OF_RANGE('major', '>= 0', major);
-    if (minor < 0)
-      throw new ERR_OUT_OF_RANGE('minor', '>= 0', minor);
-    if (patch < 0)
-      throw new ERR_OUT_OF_RANGE('patch', '>= 0', patch);
 
     if (major > majorVersion)
       return -1;
@@ -348,6 +342,8 @@ function setupCompareVersion() {
     if (prereleaseTag) {
       if (prereleaseTag === tag)
         return 0;
+      // Note: we do not fully adhere to semver as we do not check numbers
+      // properly. It applies to e.g., 'rc2' > 'rc10'.
       return prereleaseTag < tag ? -1 : 1;
     }
     if (tag)
diff --git a/test/parallel/test-process-release.js b/test/parallel/test-process-release.js
index 0179fdce880578..35d664f579a964 100644
--- a/test/parallel/test-process-release.js
+++ b/test/parallel/test-process-release.js
@@ -89,6 +89,21 @@ if (tag) {
   });
 });
 
+[
+  [1.5, 1, 1, 'major'],
+  [1, 1.5, 1, 'minor'],
+  [1, 1, 1.5, 'patch'],
+].forEach(([major, minor, patch, expected]) => {
+  assert.throws(() => {
+    compareVersion(major, minor, patch);
+  }, {
+    code: 'ERR_OUT_OF_RANGE',
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
+    message: `The value of "${expected}" is out of range. ` +
+             'It must be an integer. Received 1.5'
+  });
+});
+
 [
   '1a.0.0',
   '1.0a.0',

From 88004709434c18c027a60b1461020b5a5288adb1 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Mon, 16 Apr 2018 02:22:06 +0200
Subject: [PATCH 3/3] process: fix release.compareVersion logic

The return value was not intuitive. It is now exactly the other way
around as it was originally implemented.

This also fixes a bug with the pre-release tag where the former
implementation detected `1.0.0` to be "younger" than `1.0.0-pre`.

Fixes: https://github.com/nodejs/node/issues/20053
---
 doc/api/process.md                    |  8 ++++----
 lib/internal/process.js               | 16 ++++++++--------
 test/parallel/test-process-release.js | 20 ++++++++++----------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/doc/api/process.md b/doc/api/process.md
index 2ff69ab8a35e1c..1bb8c01548acac 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -1548,8 +1548,8 @@ added: REPLACEME
 * `minor` {number} The minor version to compare.
 * `patch` {number} The patch version to compare.
 * `tag` {string} The pre-release tag to compare.
-* Returns: {number} `1` if the given version is lower than the current release
-  version, `0` if the given version matches the process version, and `-1`
+* Returns: {number} `-1` if the given version is lower than the current release
+  version, `0` if the given version matches the process version, and `1`
   if the given version is greater than the release version.
 
 Perform a SemVer comparison to the release version.
@@ -1560,8 +1560,8 @@ added: REPLACEME
 -->
 
 * `version` {string} The semver version to compare.
-* Returns: {number} `1` if the given version is lower than the current release
-  version, `0` if the given version matches the process version, and `-1`
+* Returns: {number} `-1` if the given version is lower than the current release
+  version, `0` if the given version matches the process version, and `1`
   if the given version is greater than the release version.
 
 Perform a SemVer comparison to the release version.
diff --git a/lib/internal/process.js b/lib/internal/process.js
index 75a3716043209d..79c44fdcd3e31c 100644
--- a/lib/internal/process.js
+++ b/lib/internal/process.js
@@ -326,28 +326,28 @@ function setupCompareVersion() {
     }
 
     if (major > majorVersion)
-      return -1;
-    if (major < majorVersion)
       return 1;
+    if (major < majorVersion)
+      return -1;
 
     if (minor > minorVersion)
-      return -1;
-    if (minor < minorVersion)
       return 1;
+    if (minor < minorVersion)
+      return -1;
 
     if (patch > patchVersion)
-      return -1;
-    if (patch < patchVersion)
       return 1;
+    if (patch < patchVersion)
+      return -1;
     if (prereleaseTag) {
       if (prereleaseTag === tag)
         return 0;
       // Note: we do not fully adhere to semver as we do not check numbers
       // properly. It applies to e.g., 'rc2' > 'rc10'.
-      return prereleaseTag < tag ? -1 : 1;
+      return prereleaseTag > tag ? -1 : 1;
     }
     if (tag)
-      return -1;
+      return 1;
 
     return 0;
   };
diff --git a/test/parallel/test-process-release.js b/test/parallel/test-process-release.js
index 35d664f579a964..f5e1eb877aef37 100644
--- a/test/parallel/test-process-release.js
+++ b/test/parallel/test-process-release.js
@@ -29,12 +29,12 @@ const {
 
 [
   [major, minor, patch, tag, 0],
-  [major + 1, minor, patch, tag, -1],
-  [major - 1, minor, patch, tag, 1],
-  [major, minor + 1, patch, tag, -1],
-  [major, minor - 1, patch, tag, 1],
-  [major, minor, patch + 1, tag, -1],
-  [major, minor, patch - 1, tag, 1]
+  [major + 1, minor, patch, tag, 1],
+  [major - 1, minor, patch, tag, -1],
+  [major, minor + 1, patch, tag, 1],
+  [major, minor - 1, patch, tag, -1],
+  [major, minor, patch + 1, tag, 1],
+  [major, minor, patch - 1, tag, -1]
 ].forEach(([major, minor, patch, tag, expected]) => {
   // Skip invalid entries.
   if (major < 0 || minor < 0 || patch < 0)
@@ -47,12 +47,12 @@ const {
 if (tag) {
   assert.strictEqual(compareVersion(major, minor, patch), 1);
   assert.strictEqual(compareVersion(`${major}.${minor}.${patch}`), 1);
-  assert.strictEqual(compareVersion(major, minor, patch, `${tag}-fake`), -1);
+  assert.strictEqual(compareVersion(major, minor, patch, `${tag}-fake`), 1);
   assert.strictEqual(
-    compareVersion(major, minor, patch, `${tag.slice(0, -1)}`), 1);
+    compareVersion(major, minor, patch, `${tag.slice(0, -1)}`), -1);
 } else {
-  assert.strictEqual(compareVersion(major, minor, patch, 'fake-tag'), -1);
-  assert.strictEqual(compareVersion(`${major}.${minor}.${patch}-fake-tag`), -1);
+  assert.strictEqual(compareVersion(major, minor, patch, 'fake-tag'), 1);
+  assert.strictEqual(compareVersion(`${major}.${minor}.${patch}-fake-tag`), 1);
   assert.strictEqual(compareVersion(major, minor, patch), 0);
 }