GNU bug report logs - #57353
[PATCH] Fix parse-colon-path with UNC directory names

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57353 in the body.
You can then email your comments to 57353 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Copley <rcopley <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix parse-colon-path with UNC directory names
Date: Tue, 23 Aug 2022 12:34:02 +0100

[Message part 1 (text/plain, inline)]

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.

[0001-Fix-parse-colon-path-with-UNC-directory-names.patch (text/plain, attachment)]

Message #8 received at 57353 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Copley <rcopley <at> gmail.com>
Cc: 57353 <at> debbugs.gnu.org
Subject: Re: bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
Date: Tue, 23 Aug 2022 16:21:17 +0300

> From: Richard Copley <rcopley <at> 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.

Message #11 received at 57353 <at> debbugs.gnu.org (full text, mbox):

From: Richard Copley <rcopley <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57353 <at> debbugs.gnu.org
Subject: Re: bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
Date: Wed, 24 Aug 2022 15:15:02 +0100

On Tue, 23 Aug 2022 at 14:21, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Richard Copley <rcopley <at> 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.

Message #14 received at 57353 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: rcopley <at> gmail.com
Cc: 57353 <at> debbugs.gnu.org
Subject: Re: bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
Date: Wed, 24 Aug 2022 17:15:53 +0300

> Cc: 57353 <at> debbugs.gnu.org
> Date: Tue, 23 Aug 2022 16:21:17 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Richard Copley <rcopley <at> 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)

Message #17 received at 57353 <at> debbugs.gnu.org (full text, mbox):

From: Richard Copley <rcopley <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57353 <at> debbugs.gnu.org
Subject: Re: bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
Date: Wed, 24 Aug 2022 15:23:56 +0100

On Wed, 24 Aug 2022 at 15:15, Eli Zaretskii <eliz <at> 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.

Message #22 received at 57353-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Copley <rcopley <at> gmail.com>
Cc: 57353-done <at> debbugs.gnu.org
Subject: Re: bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
Date: Wed, 24 Aug 2022 19:21:47 +0300

> From: Richard Copley <rcopley <at> gmail.com>
> Date: Wed, 24 Aug 2022 15:23:56 +0100
> Cc: 57353 <at> debbugs.gnu.org
> 
> On Wed, 24 Aug 2022 at 15:15, Eli Zaretskii <eliz <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.