gh-117381: Improve error messages for ntpath.commonpath() (GH-117382)

3 files changed, 54 insertions, 53 deletions

diff --git a/Lib/ntpath.py b/Lib/ntpath.py
index 0650f14..f9f6c78 100644
--- a/Lib/ntpath.py
+++ b/Lib/ntpath.py
@@ -857,9 +857,6 @@ def commonpath(paths):
         drivesplits = [splitroot(p.replace(altsep, sep).lower()) for p in paths]
         split_paths = [p.split(sep) for d, r, p in drivesplits]
 
-        if len({r for d, r, p in drivesplits}) != 1:
-            raise ValueError("Can't mix absolute and relative paths")
-
         # Check that all drive letters or UNC paths match. The check is made only
         # now otherwise type errors for mixing strings and bytes would not be
         # caught.
@@ -867,6 +864,12 @@ def commonpath(paths):
             raise ValueError("Paths don't have the same drive")
 
         drive, root, path = splitroot(paths[0].replace(altsep, sep))
+        if len({r for d, r, p in drivesplits}) != 1:
+            if drive:
+                raise ValueError("Can't mix absolute and relative paths")
+            else:
+                raise ValueError("Can't mix rooted and not-rooted paths")
+
         common = path.split(sep)
         common = [c for c in common if c and c != curdir]
 
diff --git a/Lib/test/test_ntpath.py b/Lib/test/test_ntpath.py
index c816f99..3115613 100644
--- a/Lib/test/test_ntpath.py
+++ b/Lib/test/test_ntpath.py
@@ -866,46 +866,47 @@ class TestNtpath(NtpathTestCase):
         def check(paths, expected):
             tester(('ntpath.commonpath(%r)' % paths).replace('\\\\', '\\'),
                    expected)
-        def check_error(exc, paths):
-            self.assertRaises(exc, ntpath.commonpath, paths)
-            self.assertRaises(exc, ntpath.commonpath,
-                              [os.fsencode(p) for p in paths])
+        def check_error(paths, expected):
+            self.assertRaisesRegex(ValueError, expected, ntpath.commonpath, paths)
+            self.assertRaisesRegex(ValueError, expected, ntpath.commonpath, paths[::-1])
+            self.assertRaisesRegex(ValueError, expected, ntpath.commonpath,
+                                   [os.fsencode(p) for p in paths])
+            self.assertRaisesRegex(ValueError, expected, ntpath.commonpath,
+                                   [os.fsencode(p) for p in paths[::-1]])
 
         self.assertRaises(TypeError, ntpath.commonpath, None)
         self.assertRaises(ValueError, ntpath.commonpath, [])
         self.assertRaises(ValueError, ntpath.commonpath, iter([]))
-        check_error(ValueError, ['C:\\Program Files', 'Program Files'])
-        check_error(ValueError, ['C:\\Program Files', 'C:Program Files'])
-        check_error(ValueError, ['\\Program Files', 'Program Files'])
-        check_error(ValueError, ['Program Files', 'C:\\Program Files'])
-
-        check(['C:\\Program Files'], 'C:\\Program Files')
-        check(['C:\\Program Files', 'C:\\Program Files'], 'C:\\Program Files')
-        check(['C:\\Program Files\\', 'C:\\Program Files'],
-              'C:\\Program Files')
-        check(['C:\\Program Files\\', 'C:\\Program Files\\'],
-              'C:\\Program Files')
-        check(['C:\\\\Program Files', 'C:\\Program Files\\\\'],
-              'C:\\Program Files')
-        check(['C:\\.\\Program Files', 'C:\\Program Files\\.'],
-              'C:\\Program Files')
-        check(['C:\\', 'C:\\bin'], 'C:\\')
-        check(['C:\\Program Files', 'C:\\bin'], 'C:\\')
-        check(['C:\\Program Files', 'C:\\Program Files\\Bar'],
-              'C:\\Program Files')
-        check(['C:\\Program Files\\Foo', 'C:\\Program Files\\Bar'],
-              'C:\\Program Files')
-        check(['C:\\Program Files', 'C:\\Projects'], 'C:\\')
-        check(['C:\\Program Files\\', 'C:\\Projects'], 'C:\\')
-
-        check(['C:\\Program Files\\Foo', 'C:/Program Files/Bar'],
-              'C:\\Program Files')
-        check(['C:\\Program Files\\Foo', 'c:/program files/bar'],
-              'C:\\Program Files')
-        check(['c:/program files/bar', 'C:\\Program Files\\Foo'],
-              'c:\\program files')
-
-        check_error(ValueError, ['C:\\Program Files', 'D:\\Program Files'])
+
+        # gh-117381: Logical error messages
+        check_error(['C:\\Foo', 'C:Foo'], "Can't mix absolute and relative paths")
+        check_error(['C:\\Foo', '\\Foo'], "Paths don't have the same drive")
+        check_error(['C:\\Foo', 'Foo'], "Paths don't have the same drive")
+        check_error(['C:Foo', '\\Foo'], "Paths don't have the same drive")
+        check_error(['C:Foo', 'Foo'], "Paths don't have the same drive")
+        check_error(['\\Foo', 'Foo'], "Can't mix rooted and not-rooted paths")
+
+        check(['C:\\Foo'], 'C:\\Foo')
+        check(['C:\\Foo', 'C:\\Foo'], 'C:\\Foo')
+        check(['C:\\Foo\\', 'C:\\Foo'], 'C:\\Foo')
+        check(['C:\\Foo\\', 'C:\\Foo\\'], 'C:\\Foo')
+        check(['C:\\\\Foo', 'C:\\Foo\\\\'], 'C:\\Foo')
+        check(['C:\\.\\Foo', 'C:\\Foo\\.'], 'C:\\Foo')
+        check(['C:\\', 'C:\\baz'], 'C:\\')
+        check(['C:\\Bar', 'C:\\baz'], 'C:\\')
+        check(['C:\\Foo', 'C:\\Foo\\Baz'], 'C:\\Foo')
+        check(['C:\\Foo\\Bar', 'C:\\Foo\\Baz'], 'C:\\Foo')
+        check(['C:\\Bar', 'C:\\Baz'], 'C:\\')
+        check(['C:\\Bar\\', 'C:\\Baz'], 'C:\\')
+
+        check(['C:\\Foo\\Bar', 'C:/Foo/Baz'], 'C:\\Foo')
+        check(['C:\\Foo\\Bar', 'c:/foo/baz'], 'C:\\Foo')
+        check(['c:/foo/bar', 'C:\\Foo\\Baz'], 'c:\\foo')
+
+        # gh-117381: Logical error messages
+        check_error(['C:\\Foo', 'D:\\Foo'], "Paths don't have the same drive")
+        check_error(['C:\\Foo', 'D:Foo'], "Paths don't have the same drive")
+        check_error(['C:Foo', 'D:Foo'], "Paths don't have the same drive")
 
         check(['spam'], 'spam')
         check(['spam', 'spam'], 'spam')
@@ -919,20 +920,16 @@ class TestNtpath(NtpathTestCase):
 
         check([''], '')
         check(['', 'spam\\alot'], '')
-        check_error(ValueError, ['', '\\spam\\alot'])
-
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          [b'C:\\Program Files', 'C:\\Program Files\\Foo'])
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          [b'C:\\Program Files', 'Program Files\\Foo'])
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          [b'Program Files', 'C:\\Program Files\\Foo'])
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          ['C:\\Program Files', b'C:\\Program Files\\Foo'])
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          ['C:\\Program Files', b'Program Files\\Foo'])
-        self.assertRaises(TypeError, ntpath.commonpath,
-                          ['Program Files', b'C:\\Program Files\\Foo'])
+
+        # gh-117381: Logical error messages
+        check_error(['', '\\spam\\alot'], "Can't mix rooted and not-rooted paths")
+
+        self.assertRaises(TypeError, ntpath.commonpath, [b'C:\\Foo', 'C:\\Foo\\Baz'])
+        self.assertRaises(TypeError, ntpath.commonpath, [b'C:\\Foo', 'Foo\\Baz'])
+        self.assertRaises(TypeError, ntpath.commonpath, [b'Foo', 'C:\\Foo\\Baz'])
+        self.assertRaises(TypeError, ntpath.commonpath, ['C:\\Foo', b'C:\\Foo\\Baz'])
+        self.assertRaises(TypeError, ntpath.commonpath, ['C:\\Foo', b'Foo\\Baz'])
+        self.assertRaises(TypeError, ntpath.commonpath, ['Foo', b'C:\\Foo\\Baz'])
 
     @unittest.skipIf(is_emscripten, "Emscripten cannot fstat unnamed files.")
     def test_sameopenfile(self):
diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-03-29-21-43-19.gh-issue-117381.fT0JFM.rst b/Misc/NEWS.d/next/Core and Builtins/2024-03-29-21-43-19.gh-issue-117381.fT0JFM.rst
new file mode 100644
index 0000000..88b6c32
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2024-03-29-21-43-19.gh-issue-117381.fT0JFM.rst
@@ -0,0 +1 @@
+Fix error message for :func:`ntpath.commonpath`.