Revert "path: fix bugs and inconsistencies"

This reverts commit efbba60e5b. PR-URL: https://github.com/nodejs/node/pull/55414 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
2024-11-21 10:59:27 +00:00 · 2024-10-21 03:10:47 -04:00 · 2024-10-21 03:10:47 -04:00 · ee46d2297c
commit ee46d2297c
parent 7e60b5e15b
9 changed files with 71 additions and 186 deletions
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@ -700,7 +700,7 @@ Module._findPath = function(request, paths, isMain) {

  let exts;
  const trailingSlash = request.length > 0 &&
-    ((StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || (
+    (StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || (
      StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_DOT &&
      (
        request.length === 1 ||
@ -710,18 +710,7 @@ Module._findPath = function(request, paths, isMain) {
          StringPrototypeCharCodeAt(request, request.length - 3) === CHAR_FORWARD_SLASH
        ))
      )
-    )) || (isWindows && (
-      StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_BACKWARD_SLASH || (
-        StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_DOT &&
-        (
-          request.length === 1 ||
-          StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_BACKWARD_SLASH ||
-          (StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_DOT && (
-            request.length === 2 ||
-            StringPrototypeCharCodeAt(request, request.length - 3) === CHAR_BACKWARD_SLASH
-          ))
-        )
-      ))));
+    ));

  const isRelative = StringPrototypeCharCodeAt(request, 0) === CHAR_DOT &&
    (
--- a/lib/internal/url.js
+++ b/lib/internal/url.js
@ -74,7 +74,9 @@ const {
 } = require('internal/errors');
 const {
  CHAR_AMPERSAND,
+  CHAR_BACKWARD_SLASH,
  CHAR_EQUAL,
+  CHAR_FORWARD_SLASH,
  CHAR_LOWERCASE_A,
  CHAR_LOWERCASE_Z,
  CHAR_PERCENT,
@ -1597,7 +1599,14 @@ function pathToFileURL(filepath, options = kEmptyObject) {
    );
    return outURL;
  }
-  const resolved = windows ? path.win32.resolve(filepath) : path.posix.resolve(filepath);
+  let resolved = windows ? path.win32.resolve(filepath) : path.posix.resolve(filepath);
+  // path.resolve strips trailing slashes so we must add them back
+  const filePathLast = StringPrototypeCharCodeAt(filepath,
+                                                 filepath.length - 1);
+  if ((filePathLast === CHAR_FORWARD_SLASH ||
+       ((windows ?? isWindows) && filePathLast === CHAR_BACKWARD_SLASH)) &&
+      resolved[resolved.length - 1] !== path.sep)
+    resolved += '/';

  return new URL(`file://${encodePathChars(resolved, { windows })}`);
 }
--- a/lib/path.js
+++ b/lib/path.js
@ -190,7 +190,6 @@ const win32 = {
    let resolvedDevice = '';
    let resolvedTail = '';
    let resolvedAbsolute = false;
-    let slashCheck = false;

    for (let i = args.length - 1; i >= -1; i--) {
      let path;
@ -222,10 +221,6 @@ const win32 = {
        }
      }

-      if (i === args.length - 1 &&
-          isPathSeparator(StringPrototypeCharCodeAt(path, path.length - 1))) {
-        slashCheck = true;
-      }
      const len = path.length;
      let rootEnd = 0;
      let device = '';
@ -273,16 +268,10 @@ const win32 = {
                j++;
              }
              if (j === len || j !== last) {
-                if (firstPart !== '.' && firstPart !== '?') {
                // We matched a UNC root
                device =
                  `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`;
                rootEnd = j;
-                } else {
-                  // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0)
-                  device = `\\\\${firstPart}`;
-                  rootEnd = 4;
-                }
              }
            }
          }
@ -334,21 +323,9 @@ const win32 = {
    resolvedTail = normalizeString(resolvedTail, !resolvedAbsolute, '\\',
                                   isPathSeparator);

-    if (!resolvedAbsolute) {
-      return `${resolvedDevice}${resolvedTail}` || '.';
-    }
-
-    if (resolvedTail.length === 0) {
-      return slashCheck ? `${resolvedDevice}\\` : resolvedDevice;
-    }
-
-    if (slashCheck) {
-      return resolvedTail === '\\' ?
-        `${resolvedDevice}\\` :
-        `${resolvedDevice}\\${resolvedTail}\\`;
-    }
-
-    return `${resolvedDevice}\\${resolvedTail}`;
+    return resolvedAbsolute ?
+      `${resolvedDevice}\\${resolvedTail}` :
+      `${resolvedDevice}${resolvedTail}` || '.';
  },

  /**
@ -404,17 +381,13 @@ const win32 = {
                   !isPathSeparator(StringPrototypeCharCodeAt(path, j))) {
              j++;
            }
-            if (j === len || j !== last) {
-              if (firstPart === '.' || firstPart === '?') {
-                // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0)
-                device = `\\\\${firstPart}`;
-                rootEnd = 4;
-              } else if (j === len) {
+            if (j === len) {
              // We matched a UNC root only
              // Return the normalized version of the UNC root since there
              // is nothing left to process
              return `\\\\${firstPart}\\${StringPrototypeSlice(path, last)}\\`;
-              } else {
+            }
+            if (j !== last) {
              // We matched a UNC root with leftovers
              device =
                `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`;
@ -422,7 +395,6 @@ const win32 = {
            }
          }
        }
-        }
      } else {
        rootEnd = 1;
      }
@ -1190,7 +1162,6 @@ const posix = {
  resolve(...args) {
    let resolvedPath = '';
    let resolvedAbsolute = false;
-    let slashCheck = false;

    for (let i = args.length - 1; i >= 0 && !resolvedAbsolute; i--) {
      const path = args[i];
@ -1200,17 +1171,8 @@ const posix = {
      if (path.length === 0) {
        continue;
      }
-      if (i === args.length - 1 &&
-          isPosixPathSeparator(StringPrototypeCharCodeAt(path,
-                                                         path.length - 1))) {
-        slashCheck = true;
-      }

-      if (resolvedPath.length !== 0) {
      resolvedPath = `${path}/${resolvedPath}`;
-      } else {
-        resolvedPath = path;
-      }
      resolvedAbsolute =
        StringPrototypeCharCodeAt(path, 0) === CHAR_FORWARD_SLASH;
    }
@ -1229,20 +1191,10 @@ const posix = {
    resolvedPath = normalizeString(resolvedPath, !resolvedAbsolute, '/',
                                   isPosixPathSeparator);

-    if (!resolvedAbsolute) {
-      if (resolvedPath.length === 0) {
-        return '.';
+    if (resolvedAbsolute) {
+      return `/${resolvedPath}`;
    }
-      if (slashCheck) {
-        return `${resolvedPath}/`;
-      }
-      return resolvedPath;
-    }
-
-    if (resolvedPath.length === 0 || resolvedPath === '/') {
-      return '/';
-    }
-    return slashCheck ? `/${resolvedPath}/` : `/${resolvedPath}`;
+    return resolvedPath.length > 0 ? resolvedPath : '.';
  },

  /**
@ -1326,35 +1278,11 @@ const posix = {
    if (from === to)
      return '';

-    // Trim any leading slashes
-    let fromStart = 0;
-    while (fromStart < from.length &&
-           StringPrototypeCharCodeAt(from, fromStart) === CHAR_FORWARD_SLASH) {
-      fromStart++;
-    }
-    // Trim trailing slashes
-    let fromEnd = from.length;
-    while (
-      fromEnd - 1 > fromStart &&
-      StringPrototypeCharCodeAt(from, fromEnd - 1) === CHAR_FORWARD_SLASH
-    ) {
-      fromEnd--;
-    }
+    const fromStart = 1;
+    const fromEnd = from.length;
    const fromLen = fromEnd - fromStart;
-
-    // Trim any leading slashes
-    let toStart = 0;
-    while (toStart < to.length &&
-           StringPrototypeCharCodeAt(to, toStart) === CHAR_FORWARD_SLASH) {
-      toStart++;
-    }
-    // Trim trailing slashes
-    let toEnd = to.length;
-    while (toEnd - 1 > toStart &&
-           StringPrototypeCharCodeAt(to, toEnd - 1) === CHAR_FORWARD_SLASH) {
-      toEnd--;
-    }
-    const toLen = toEnd - toStart;
+    const toStart = 1;
+    const toLen = to.length - toStart;

    // Compare paths to find the longest common path from root
    const length = (fromLen < toLen ? fromLen : toLen);
--- a/src/path.cc
+++ b/src/path.cc
@ -97,7 +97,6 @@ std::string PathResolve(Environment* env,
  std::string resolvedDevice = "";
  std::string resolvedTail = "";
  bool resolvedAbsolute = false;
-  bool slashCheck = false;
  const size_t numArgs = paths.size();
  auto cwd = env->GetCwd(env->exec_path());

@ -127,10 +126,6 @@ std::string PathResolve(Environment* env,
      }
    }

-    if (static_cast<size_t>(i) == numArgs - 1 &&
-        IsPathSeparator(path[path.length() - 1])) {
-      slashCheck = true;
-    }
    const size_t len = path.length();
    int rootEnd = 0;
    std::string device = "";
@ -175,16 +170,9 @@ std::string PathResolve(Environment* env,
              j++;
            }
            if (j == len || j != last) {
-              if (firstPart != "." && firstPart != "?") {
              // We matched a UNC root
-                device =
-                    "\\\\" + firstPart + "\\" + path.substr(last, j - last);
+              device = "\\\\" + firstPart + "\\" + path.substr(last, j - last);
              rootEnd = j;
-              } else {
-                // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0)
-                device = "\\\\" + firstPart;
-                rootEnd = 4;
-              }
            }
          }
        }
@ -232,27 +220,15 @@ std::string PathResolve(Environment* env,
  // Normalize the tail path
  resolvedTail = NormalizeString(resolvedTail, !resolvedAbsolute, "\\");

-  if (!resolvedAbsolute) {
+  if (resolvedAbsolute) {
+    return resolvedDevice + "\\" + resolvedTail;
+  }
+
  if (!resolvedDevice.empty() || !resolvedTail.empty()) {
    return resolvedDevice + resolvedTail;
  }
+
  return ".";
-  }
-
-  if (resolvedTail.empty()) {
-    if (slashCheck) {
-      return resolvedDevice + "\\";
-    }
-    return resolvedDevice;
-  }
-
-  if (slashCheck) {
-    if (resolvedTail == "\\") {
-      return resolvedDevice + "\\";
-    }
-    return resolvedDevice + "\\" + resolvedTail + "\\";
-  }
-  return resolvedDevice + "\\" + resolvedTail;
 }
 #else   // _WIN32
 std::string PathResolve(Environment* env,
@ -261,16 +237,11 @@ std::string PathResolve(Environment* env,
  bool resolvedAbsolute = false;
  auto cwd = env->GetCwd(env->exec_path());
  const size_t numArgs = paths.size();
-  bool slashCheck = false;

  for (int i = numArgs - 1; i >= -1 && !resolvedAbsolute; i--) {
    const std::string& path = (i >= 0) ? std::string(paths[i]) : cwd;

    if (!path.empty()) {
-      if (static_cast<size_t>(i) == numArgs - 1 && path.back() == '/') {
-        slashCheck = true;
-      }
-
      resolvedPath = std::string(path) + "/" + resolvedPath;

      if (path.front() == '/') {
@ -283,21 +254,15 @@ std::string PathResolve(Environment* env,
  // Normalize the path
  auto normalizedPath = NormalizeString(resolvedPath, !resolvedAbsolute, "/");

-  if (!resolvedAbsolute) {
+  if (resolvedAbsolute) {
+    return "/" + normalizedPath;
+  }
+
  if (normalizedPath.empty()) {
    return ".";
  }
-    if (slashCheck) {
-      return normalizedPath + "/";
-    }
+
  return normalizedPath;
-  }
-
-  if (normalizedPath.empty() || normalizedPath == "/") {
-    return "/";
-  }
-
-  return slashCheck ? "/" + normalizedPath + "/" : "/" + normalizedPath;
 }
 #endif  // _WIN32

--- a/test/cctest/test_path.cc
+++ b/test/cctest/test_path.cc
@ -25,28 +25,26 @@ TEST_F(PathTest, PathResolve) {
            "d:\\e.exe");
  EXPECT_EQ(PathResolve(*env, {"c:/ignore", "c:/some/file"}), "c:\\some\\file");
  EXPECT_EQ(PathResolve(*env, {"d:/ignore", "d:some/dir//"}),
-            "d:\\ignore\\some\\dir\\");
+            "d:\\ignore\\some\\dir");
  EXPECT_EQ(PathResolve(*env, {"."}), cwd);
  EXPECT_EQ(PathResolve(*env, {"//server/share", "..", "relative\\"}),
-            "\\\\server\\share\\relative\\");
+            "\\\\server\\share\\relative");
  EXPECT_EQ(PathResolve(*env, {"c:/", "//"}), "c:\\");
  EXPECT_EQ(PathResolve(*env, {"c:/", "//dir"}), "c:\\dir");
-  EXPECT_EQ(PathResolve(*env, {"c:/", "//server/share"}), "\\\\server\\share");
-  EXPECT_EQ(PathResolve(*env, {"c:/", "//server//share"}), "\\\\server\\share");
+  EXPECT_EQ(PathResolve(*env, {"c:/", "//server/share"}),
+            "\\\\server\\share\\");
+  EXPECT_EQ(PathResolve(*env, {"c:/", "//server//share"}),
+            "\\\\server\\share\\");
  EXPECT_EQ(PathResolve(*env, {"c:/", "///some//dir"}), "c:\\some\\dir");
  EXPECT_EQ(
      PathResolve(*env, {"C:\\foo\\tmp.3\\", "..\\tmp.3\\cycles\\root.js"}),
      "C:\\foo\\tmp.3\\cycles\\root.js");
-  EXPECT_EQ(PathResolve(*env, {"\\\\.\\PHYSICALDRIVE0"}),
-            "\\\\.\\PHYSICALDRIVE0");
-  EXPECT_EQ(PathResolve(*env, {"\\\\?\\PHYSICALDRIVE0"}),
-            "\\\\?\\PHYSICALDRIVE0");
 #else
-  EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file/");
-  EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file/");
+  EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file");
+  EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file");
  EXPECT_EQ(PathResolve(*env, {"a/b/c/", "../../.."}), cwd);
  EXPECT_EQ(PathResolve(*env, {"."}), cwd);
-  EXPECT_EQ(PathResolve(*env, {"/some/dir", ".", "/absolute/"}), "/absolute/");
+  EXPECT_EQ(PathResolve(*env, {"/some/dir", ".", "/absolute/"}), "/absolute");
  EXPECT_EQ(PathResolve(*env, {"/foo/tmp.3/", "../tmp.3/cycles/root.js"}),
            "/foo/tmp.3/cycles/root.js");
 #endif
--- a/test/parallel/test-fs-utils-get-dirents.js
+++ b/test/parallel/test-fs-utils-get-dirents.js
@ -92,7 +92,7 @@ const filename = 'foo';
        'DeprecationWarning',
        'dirent.path is deprecated in favor of dirent.parentPath',
        'DEP0178');
-      assert.deepStrictEqual(dirent.path, Buffer.from(tmpdir.resolve(`${filename}`)));
+      assert.deepStrictEqual(dirent.path, Buffer.from(tmpdir.resolve(`${filename}/`)));
    },
    ));
 }
--- a/test/parallel/test-path-makelong.js
+++ b/test/parallel/test-path-makelong.js
@ -76,10 +76,10 @@ if (common.isWindows) {
 assert.strictEqual(path.win32.toNamespacedPath('C:\\foo'), '\\\\?\\C:\\foo');
 assert.strictEqual(path.win32.toNamespacedPath('C:/foo'), '\\\\?\\C:\\foo');
 assert.strictEqual(path.win32.toNamespacedPath('\\\\foo\\bar'),
-                   '\\\\?\\UNC\\foo\\bar');
+                   '\\\\?\\UNC\\foo\\bar\\');
 assert.strictEqual(path.win32.toNamespacedPath('//foo//bar'),
-                   '\\\\?\\UNC\\foo\\bar');
-assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo');
+                   '\\\\?\\UNC\\foo\\bar\\');
+assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\');
 assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\c:\\Windows/System'), '\\\\?\\c:\\Windows\\System');
 assert.strictEqual(path.win32.toNamespacedPath(null), null);
 assert.strictEqual(path.win32.toNamespacedPath(true), true);
--- a/test/parallel/test-path-normalize.js
+++ b/test/parallel/test-path-normalize.js
@ -40,8 +40,6 @@ assert.strictEqual(
  '..\\..\\..\\..\\baz'
 );
 assert.strictEqual(path.win32.normalize('foo/bar\\baz'), 'foo\\bar\\baz');
-assert.strictEqual(path.win32.normalize('\\\\.\\foo'), '\\\\.\\foo');
-assert.strictEqual(path.win32.normalize('\\\\.\\foo\\'), '\\\\.\\foo\\');

 assert.strictEqual(path.posix.normalize('./fixtures///b/../b/c.js'),
                   'fixtures/b/c.js');
--- a/test/parallel/test-path-resolve.js
+++ b/test/parallel/test-path-resolve.js
@ -23,27 +23,25 @@ const resolveTests = [
    [[['c:/blah\\blah', 'd:/games', 'c:../a'], 'c:\\blah\\a'],
     [['c:/ignore', 'd:\\a/b\\c/d', '\\e.exe'], 'd:\\e.exe'],
     [['c:/ignore', 'c:/some/file'], 'c:\\some\\file'],
-     [['d:/ignore', 'd:some/dir//'], 'd:\\ignore\\some\\dir\\'],
+     [['d:/ignore', 'd:some/dir//'], 'd:\\ignore\\some\\dir'],
     [['.'], process.cwd()],
-     [['//server/share', '..', 'relative\\'], '\\\\server\\share\\relative\\'],
+     [['//server/share', '..', 'relative\\'], '\\\\server\\share\\relative'],
     [['c:/', '//'], 'c:\\'],
     [['c:/', '//dir'], 'c:\\dir'],
-     [['c:/', '//server/share'], '\\\\server\\share'],
-     [['c:/', '//server//share'], '\\\\server\\share'],
+     [['c:/', '//server/share'], '\\\\server\\share\\'],
+     [['c:/', '//server//share'], '\\\\server\\share\\'],
     [['c:/', '///some//dir'], 'c:\\some\\dir'],
     [['C:\\foo\\tmp.3\\', '..\\tmp.3\\cycles\\root.js'],
      'C:\\foo\\tmp.3\\cycles\\root.js'],
-     [['\\\\.\\PHYSICALDRIVE0'], '\\\\.\\PHYSICALDRIVE0'],
-     [['\\\\?\\PHYSICALDRIVE0'], '\\\\?\\PHYSICALDRIVE0'],
    ],
  ],
  [ path.posix.resolve,
    // Arguments                    result
-    [[['/var/lib', '../', 'file/'], '/var/file/'],
-     [['/var/lib', '/../', 'file/'], '/file/'],
+    [[['/var/lib', '../', 'file/'], '/var/file'],
+     [['/var/lib', '/../', 'file/'], '/file'],
     [['a/b/c/', '../../..'], posixyCwd],
     [['.'], posixyCwd],
-     [['/some/dir', '.', '/absolute/'], '/absolute/'],
+     [['/some/dir', '.', '/absolute/'], '/absolute'],
     [['/foo/tmp.3/', '../tmp.3/cycles/root.js'], '/foo/tmp.3/cycles/root.js'],
    ],
  ],