Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Ken Brown]

On 8/24/2022 12:20 PM, Eli Zaretskii wrote:
> branch: master
> commit 95b1eacd4750da7329380aabcb383a8f9d96a59b
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
> 
>      Fix handling of UNCs in 'parse-colon-path
>      
>      * lisp/files.el (parse-colon-path): Don't remove the second
>      leading slash on systems that support UNCs.  (Bug#57353)
>      
>      * test/lisp/files-tests.el (files-tests-bug-21454): Update
>      expected results.
>      (files-colon-path): Add a new test pattern.

After this commit I'm getting the following test failure on Cygwin.  I don't 
have time to look into it now, but I can do so in a few days if the fix isn't 
obvious.

Test files-tests-bug-21454 backtrace:
   signal(ert-test-failed (((should (equal res (parse-colon-path "$FOO"
   ert-fail(((should (equal res (parse-colon-path "$FOO"))) :form (equa
   #f(compiled-function () #<bytecode 0x197d9660f67edbed>)()
   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
   ert-run-test(#s(ert-test :name files-tests-bug-21454 :documentation
   ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
   ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
   ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
   ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
   eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
   command-line-1(("-L" ":../../master/test" "-l" "ert" "-l" "lisp/file
   command-line()
   normal-top-level()
Test files-tests-bug-21454 condition:
     (ert-test-failed
      ((should
        (equal res
	      (parse-colon-path "$FOO")))
       :form
       (equal
        ("/foo/bar/" "/bar/qux/" "/qux/foo/")
        ("//foo/bar/" "/bar/qux/" "/qux/foo/"))
       :value nil :explanation
       (list-elt 0
		(arrays-of-different-length 9 10 "/foo/bar/" "//foo/bar/" first-mismatch-at 1))))

Ken

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Eli Zaretskii]

> Date: Wed, 24 Aug 2022 14:34:02 -0400
> From: Ken Brown <kbrown@cornell.edu>
> 
> On 8/24/2022 12:20 PM, Eli Zaretskii wrote:
> > branch: master
> > commit 95b1eacd4750da7329380aabcb383a8f9d96a59b
> > Author: Eli Zaretskii <eliz@gnu.org>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> > 
> >      Fix handling of UNCs in 'parse-colon-path
> >      
> >      * lisp/files.el (parse-colon-path): Don't remove the second
> >      leading slash on systems that support UNCs.  (Bug#57353)
> >      
> >      * test/lisp/files-tests.el (files-tests-bug-21454): Update
> >      expected results.
> >      (files-colon-path): Add a new test pattern.
> 
> After this commit I'm getting the following test failure on Cygwin.  I don't 
> have time to look into it now, but I can do so in a few days if the fix isn't 
> obvious.

Ugh, it means the test data needs to be split three-way: one for
MS-Windows/MS-DOS, another for Cygwin, and one more for the rest.
Because Cygwin is like Posix systems, but it does want to support
UNCs.

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Ken Brown]

On 8/24/2022 2:48 PM, Eli Zaretskii wrote:
>> Date: Wed, 24 Aug 2022 14:34:02 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>>
>> On 8/24/2022 12:20 PM, Eli Zaretskii wrote:
>>> branch: master
>>> commit 95b1eacd4750da7329380aabcb383a8f9d96a59b
>>> Author: Eli Zaretskii <eliz@gnu.org>
>>> Commit: Eli Zaretskii <eliz@gnu.org>
>>>
>>>       Fix handling of UNCs in 'parse-colon-path
>>>       
>>>       * lisp/files.el (parse-colon-path): Don't remove the second
>>>       leading slash on systems that support UNCs.  (Bug#57353)
>>>       
>>>       * test/lisp/files-tests.el (files-tests-bug-21454): Update
>>>       expected results.
>>>       (files-colon-path): Add a new test pattern.
>>
>> After this commit I'm getting the following test failure on Cygwin.  I don't
>> have time to look into it now, but I can do so in a few days if the fix isn't
>> obvious.
> 
> Ugh, it means the test data needs to be split three-way: one for
> MS-Windows/MS-DOS, another for Cygwin, and one more for the rest.
> Because Cygwin is like Posix systems, but it does want to support
> UNCs.

It's not just the test data that's wrong on Cygwin, but parse-colon-path is 
wrong: It always collapses multiple leading slashes to two.

Posix says that multiple leading slashes are equivalent to one slash *except* in 
the case of exactly two leading slashes.  In that case, the interpretation is 
implementation-dependent.

Cygwin follows Posix and chooses to interpret precisely two leading slashes as 
referring to a UNC path.  In particular, 3 or more slashes should be collapsed 
to 1 slash, as on other Posix systems, while exactly 2 leading slashes should be 
left alone.

Ken

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Eli Zaretskii]

> Date: Wed, 24 Aug 2022 18:03:35 -0400
> Cc: emacs-devel@gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> > Ugh, it means the test data needs to be split three-way: one for
> > MS-Windows/MS-DOS, another for Cygwin, and one more for the rest.
> > Because Cygwin is like Posix systems, but it does want to support
> > UNCs.
> 
> It's not just the test data that's wrong on Cygwin, but parse-colon-path is 
> wrong: It always collapses multiple leading slashes to two.

That's what substitute-in-file-name does as well (in the Cygwin and
MS-Windows/MS-DOS builds), and the change I made was meant to restore
backward compatibility with what parse-colon-path did as result of
using substitute-in-file-name.

> Posix says that multiple leading slashes are equivalent to one slash *except* in 
> the case of exactly two leading slashes.  In that case, the interpretation is 
> implementation-dependent.

This is not about Posix, this is about the Emacs-specific feature of
handling multiple consecutive slashes.  In particular, in Emacs,
"/foo//bar" yields "/bar", not "/foo/bar" per Posix.

> Cygwin follows Posix and chooses to interpret precisely two leading slashes as 
> referring to a UNC path.  In particular, 3 or more slashes should be collapsed 
> to 1 slash, as on other Posix systems, while exactly 2 leading slashes should be 
> left alone.

I'm not against changing the behavior of substitute-in-file-name and
parse-colon-path in this regard, but it would be a separate change,
and of a long-standing behavior.  This particular change just restored
what parse-colon-path did back when it used substitute-in-file-name.

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Ken Brown]

On 8/25/2022 1:18 AM, Eli Zaretskii wrote:
>> Date: Wed, 24 Aug 2022 18:03:35 -0400
>> Cc: emacs-devel@gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>>> Ugh, it means the test data needs to be split three-way: one for
>>> MS-Windows/MS-DOS, another for Cygwin, and one more for the rest.
>>> Because Cygwin is like Posix systems, but it does want to support
>>> UNCs.
>>
>> It's not just the test data that's wrong on Cygwin, but parse-colon-path is
>> wrong: It always collapses multiple leading slashes to two.
> 
> That's what substitute-in-file-name does as well (in the Cygwin and
> MS-Windows/MS-DOS builds), and the change I made was meant to restore
> backward compatibility with what parse-colon-path did as result of
> using substitute-in-file-name.
> 
>> Posix says that multiple leading slashes are equivalent to one slash *except* in
>> the case of exactly two leading slashes.  In that case, the interpretation is
>> implementation-dependent.
> 
> This is not about Posix, this is about the Emacs-specific feature of
> handling multiple consecutive slashes.  In particular, in Emacs,
> "/foo//bar" yields "/bar", not "/foo/bar" per Posix.

OK, I completely missed the point.

>> Cygwin follows Posix and chooses to interpret precisely two leading slashes as
>> referring to a UNC path.  In particular, 3 or more slashes should be collapsed
>> to 1 slash, as on other Posix systems, while exactly 2 leading slashes should be
>> left alone.
> 
> I'm not against changing the behavior of substitute-in-file-name and
> parse-colon-path in this regard, but it would be a separate change,
> and of a long-standing behavior.  This particular change just restored
> what parse-colon-path did back when it used substitute-in-file-name.

I think you're right, and it's just a matter of fixing the test.  Thanks for the 
explanation.

Ken

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 164 bytes --]

On 8/25/2022 8:56 AM, Ken Brown wrote:
> I think you're right, and it's just a matter of fixing the test.  Thanks for the 
> explanation.

How's the attached?

Ken

[-- Attachment #2: 0001-Fix-data-for-files-tests-bug-21454-on-Cygwin.patch --]
[-- Type: text/plain, Size: 3504 bytes --]

From 31dc56031ae7519419ba2a5f12dd184e71ff5f39 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sat, 27 Aug 2022 17:42:42 -0400
Subject: [PATCH] Fix data for files-tests-bug-21454 on Cygwin

* test/lisp/files-tests.el (files-tests-bug-21454): Fix test data
to reflect the fact that Cygwin supports UNC paths.
---
 test/lisp/files-tests.el | 47 ++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 20c712226e..682b5cdb44 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -223,20 +223,39 @@ files-tests-bug-21454
                 ("x:/foo/bar/baz/" "z:/qux/foo/"))
                ("///foo/bar/" "$FOO/baz/;/qux/foo/"
                 ("//foo/bar//baz/" "/qux/foo/")))
-           '(("/foo/bar//baz/:/bar/foo/baz//" nil
-              ("/foo/bar//baz/" "/bar/foo/baz//"))
-             ("/foo/bar/:/bar/qux/:/qux/foo" nil
-              ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
-             ("//foo/bar/:/bar/qux/:/qux/foo/" nil
-              ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
-             ("/foo/bar/:/bar/qux/:/qux/foo/" nil
-              ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
-             ("/foo//bar/:/bar/qux/:/qux/foo/" nil
-              ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
-             ("/foo//bar/:/bar/qux/:/qux/foo" nil
-              ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
-             ("/foo/bar" "$FOO/baz/:/qux/foo/" ("/foo/bar/baz/" "/qux/foo/"))
-             ("//foo/bar/" "$FOO/baz/:/qux/foo/" ("/foo/bar//baz/" "/qux/foo/")))))
+           (if (eq system-type 'cygwin)
+               '(("/foo/bar//baz/:/bar/foo/baz//" nil
+                  ("/foo/bar//baz/" "/bar/foo/baz//"))
+                 ("/foo/bar/:/bar/qux/:/qux/foo" nil
+                  ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
+                 ("//foo/bar/:/bar/qux/:/qux/foo/" nil
+                  ("//foo/bar/" "/bar/qux/" "/qux/foo/"))
+                 ("/foo/bar/:/bar/qux/:/qux/foo/" nil
+                  ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
+                 ("/foo//bar/:/bar/qux/:/qux/foo/" nil
+                  ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
+                 ("/foo//bar/:/bar/qux/:/qux/foo" nil
+                  ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
+                 ("/foo/bar" "$FOO/baz/:/qux/foo/"
+                  ("/foo/bar/baz/" "/qux/foo/"))
+                 ("///foo/bar/" "$FOO/baz/:/qux/foo/"
+                  ("//foo/bar//baz/" "/qux/foo/")))
+             '(("/foo/bar//baz/:/bar/foo/baz//" nil
+                ("/foo/bar//baz/" "/bar/foo/baz//"))
+               ("/foo/bar/:/bar/qux/:/qux/foo" nil
+                ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
+               ("//foo/bar/:/bar/qux/:/qux/foo/" nil
+                ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
+               ("/foo/bar/:/bar/qux/:/qux/foo/" nil
+                ("/foo/bar/" "/bar/qux/" "/qux/foo/"))
+               ("/foo//bar/:/bar/qux/:/qux/foo/" nil
+                ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
+               ("/foo//bar/:/bar/qux/:/qux/foo" nil
+                ("/foo//bar/" "/bar/qux/" "/qux/foo/"))
+               ("/foo/bar" "$FOO/baz/:/qux/foo/"
+                ("/foo/bar/baz/" "/qux/foo/"))
+               ("//foo/bar/" "$FOO/baz/:/qux/foo/"
+                ("/foo/bar//baz/" "/qux/foo/"))))))
         (foo-env (getenv "FOO"))
         (bar-env (getenv "BAR")))
     (unwind-protect
-- 
2.37.1

Re: master 95b1eacd47: Fix handling of UNCs in 'parse-colon-path

[Eli Zaretskii]

> Date: Sat, 27 Aug 2022 19:04:32 -0400
> From: Ken Brown <kbrown@cornell.edu>
> Cc: emacs-devel@gnu.org
> 
> On 8/25/2022 8:56 AM, Ken Brown wrote:
> > I think you're right, and it's just a matter of fixing the test.  Thanks for the 
> > explanation.
> 
> How's the attached?

LGTM, thanks.