bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Richard Copley]

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

The function `parse-colon-path' changes the leading double slash
in a UNC path component to a single slash:

(let path (mapconcat path-separator "one" "//server/share/dir")
     (parse-colon-path path))
## -> ("one" "/server/share/dir")

A comment in `parse-colon-path' says:

;; Previous implementation used `substitute-in-file-name'
;; which collapse multiple "/" in front.  Do the same for
;; backward compatibility.

However, `substitute-in-file-name' does not do that:

(substitute-in-file-name "//foo/a/b") // -> "//foo/a/b"

There's no reason to do it in `parse-colon-path' either.

[-- Attachment #2: 0001-Fix-parse-colon-path-with-UNC-directory-names.patch --]
[-- Type: text/plain, Size: 1323 bytes --]

From 55eb8c37eeabf11f9fc1ec1f84c64fc234ecf59f Mon Sep 17 00:00:00 2001
From: Richard Copley <rcopley@gmail.com>
Date: Tue, 23 Aug 2022 12:11:20 +0100
Subject: [PATCH] Fix parse-colon-path with UNC directory names

* lisp/files.el (parse-colon-path): don't collapse multiple / at
beginning of path component
---
 lisp/files.el | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index cf2a522193..569fd0224e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -853,13 +853,8 @@ nil (meaning `default-directory') as the associated list element."
   (when (stringp search-path)
     (let ((spath (substitute-env-vars search-path)))
       (mapcar (lambda (f)
-                (if (equal "" f) nil
-                  (let ((dir (file-name-as-directory f)))
-                    ;; Previous implementation used `substitute-in-file-name'
-                    ;; which collapse multiple "/" in front.  Do the same for
-                    ;; backward compatibility.
-                    (if (string-match "\\`/+" dir)
-                        (substring dir (1- (match-end 0))) dir))))
+                (unless (equal "" f)
+                  (file-name-as-directory f)))
               (split-string spath path-separator)))))
 
 (defun cd-absolute (dir)
-- 
2.37.0.windows.1

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Tue, 23 Aug 2022 12:34:02 +0100
> 
> A comment in `parse-colon-path' says:
> 
> ;; Previous implementation used `substitute-in-file-name'
> ;; which collapse multiple "/" in front.  Do the same for
> ;; backward compatibility.
> 
> However, `substitute-in-file-name' does not do that:
> 
> (substitute-in-file-name "//foo/a/b") // -> "//foo/a/b"

That is true, but:

  (substitute-in-file-name "///foo/a/b") => "//foo/a/b"

So it does collapse multiple "/", at least sometimes.  Moreover, the
above is on MS-Windows, but on GNU/Linux:

  (substitute-in-file-name "///foo/a/b") => "/foo/a/b"

So (a) this is system-dependent, and (b) substitute-in-file-name does
collapse multiple slashes, but preserves UNCs on MS-Windows.

Therefore, your patch needs some (minor) amendments.

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Richard Copley]

On Tue, 23 Aug 2022 at 14:21, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Richard Copley <rcopley@gmail.com>
> > Date: Tue, 23 Aug 2022 12:34:02 +0100
> >
> > A comment in `parse-colon-path' says:
> >
> > ;; Previous implementation used `substitute-in-file-name'
> > ;; which collapse multiple "/" in front.  Do the same for
> > ;; backward compatibility.
> >
> > However, `substitute-in-file-name' does not do that:
> >
> > (substitute-in-file-name "//foo/a/b") // -> "//foo/a/b"
>
> That is true, but:
>
>   (substitute-in-file-name "///foo/a/b") => "//foo/a/b"
>
> So it does collapse multiple "/", at least sometimes.  Moreover, the
> above is on MS-Windows, but on GNU/Linux:
>
>   (substitute-in-file-name "///foo/a/b") => "/foo/a/b"
>
> So (a) this is system-dependent, and (b) substitute-in-file-name does
> collapse multiple slashes, but preserves UNCs on MS-Windows.
>
> Therefore, your patch needs some (minor) amendments.

If the goal is to be backward-compatible with substitute-in-file-name,
we should do everything that function does, which is quite involved,
and involves configuration that is not available to lisp. Easier to
just revert the commits. But that wasn't the goal, see #21454.

Tino doesn't explain why the particular case of multiple slashes at
the start of a path component is any different from the other cases of
multiple slashes, which are no longer changed. I'm afraid I don't
understand the point of it.

I'll apply a workaround locally, and leave it up to the developers to
decide what to do, if anything.

Thanks.

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Eli Zaretskii]

> Cc: 57353@debbugs.gnu.org
> Date: Tue, 23 Aug 2022 16:21:17 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Richard Copley <rcopley@gmail.com>
> > Date: Tue, 23 Aug 2022 12:34:02 +0100
> > 
> > A comment in `parse-colon-path' says:
> > 
> > ;; Previous implementation used `substitute-in-file-name'
> > ;; which collapse multiple "/" in front.  Do the same for
> > ;; backward compatibility.
> > 
> > However, `substitute-in-file-name' does not do that:
> > 
> > (substitute-in-file-name "//foo/a/b") // -> "//foo/a/b"
> 
> That is true, but:
> 
>   (substitute-in-file-name "///foo/a/b") => "//foo/a/b"
> 
> So it does collapse multiple "/", at least sometimes.  Moreover, the
> above is on MS-Windows, but on GNU/Linux:
> 
>   (substitute-in-file-name "///foo/a/b") => "/foo/a/b"
> 
> So (a) this is system-dependent, and (b) substitute-in-file-name does
> collapse multiple slashes, but preserves UNCs on MS-Windows.
> 
> Therefore, your patch needs some (minor) amendments.

Does the patch below give good results in your use cases?

diff --git a/lisp/files.el b/lisp/files.el
index 8596d9a..26730df 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -856,10 +856,16 @@ parse-colon-path
                 (if (equal "" f) nil
                   (let ((dir (file-name-as-directory f)))
                     ;; Previous implementation used `substitute-in-file-name'
-                    ;; which collapse multiple "/" in front.  Do the same for
-                    ;; backward compatibility.
-                    (if (string-match "\\`/+" dir)
-                        (substring dir (1- (match-end 0))) dir))))
+                    ;; which collapses multiple "/" in front, while
+                    ;; preserving double slash where it matters.  Do
+                    ;; the same for backward compatibility.
+                    (if (string-match "\\`//+" dir)
+                        (substring dir
+                                   (- (match-end 0)
+                                      (if (memq system-type
+                                                '(windows-nt 'cygwin 'ms-dos))
+                                          2 1)))
+                      dir))))
               (split-string spath path-separator)))))
 
 (defun cd-absolute (dir)

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Richard Copley]

On Wed, 24 Aug 2022 at 15:15, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Does the patch below give good results in your use cases?
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 8596d9a..26730df 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -856,10 +856,16 @@ parse-colon-path
>                  (if (equal "" f) nil
>                    (let ((dir (file-name-as-directory f)))
>                      ;; Previous implementation used `substitute-in-file-name'
> -                    ;; which collapse multiple "/" in front.  Do the same for
> -                    ;; backward compatibility.
> -                    (if (string-match "\\`/+" dir)
> -                        (substring dir (1- (match-end 0))) dir))))
> +                    ;; which collapses multiple "/" in front, while
> +                    ;; preserving double slash where it matters.  Do
> +                    ;; the same for backward compatibility.
> +                    (if (string-match "\\`//+" dir)
> +                        (substring dir
> +                                   (- (match-end 0)
> +                                      (if (memq system-type
> +                                                '(windows-nt 'cygwin 'ms-dos))
> +                                          2 1)))
> +                      dir))))
>                (split-string spath path-separator)))))
>
>  (defun cd-absolute (dir)

It does.

bug#57353: [PATCH] Fix parse-colon-path with UNC directory names

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Wed, 24 Aug 2022 15:23:56 +0100
> Cc: 57353@debbugs.gnu.org
> 
> On Wed, 24 Aug 2022 at 15:15, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Does the patch below give good results in your use cases?
> >
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 8596d9a..26730df 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -856,10 +856,16 @@ parse-colon-path
> >                  (if (equal "" f) nil
> >                    (let ((dir (file-name-as-directory f)))
> >                      ;; Previous implementation used `substitute-in-file-name'
> > -                    ;; which collapse multiple "/" in front.  Do the same for
> > -                    ;; backward compatibility.
> > -                    (if (string-match "\\`/+" dir)
> > -                        (substring dir (1- (match-end 0))) dir))))
> > +                    ;; which collapses multiple "/" in front, while
> > +                    ;; preserving double slash where it matters.  Do
> > +                    ;; the same for backward compatibility.
> > +                    (if (string-match "\\`//+" dir)
> > +                        (substring dir
> > +                                   (- (match-end 0)
> > +                                      (if (memq system-type
> > +                                                '(windows-nt 'cygwin 'ms-dos))
> > +                                          2 1)))
> > +                      dir))))
> >                (split-string spath path-separator)))))
> >
> >  (defun cd-absolute (dir)
> 
> It does.

Thanks, installed with minor changes, and closing the bug.