all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
@ 2022-08-23 11:34 Richard Copley
  2022-08-23 13:21 ` Eli Zaretskii
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Copley @ 2022-08-23 11:34 UTC (permalink / raw)
  To: 57353

[-- 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
  2022-08-23 11:34 bug#57353: [PATCH] Fix parse-colon-path with UNC directory names Richard Copley
@ 2022-08-23 13:21 ` Eli Zaretskii
  2022-08-24 14:15   ` Richard Copley
  2022-08-24 14:15   ` Eli Zaretskii
  0 siblings, 2 replies; 6+ messages in thread
From: Eli Zaretskii @ 2022-08-23 13:21 UTC (permalink / raw)
  To: Richard Copley; +Cc: 57353

> 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.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
  2022-08-23 13:21 ` Eli Zaretskii
@ 2022-08-24 14:15   ` Richard Copley
  2022-08-24 14:15   ` Eli Zaretskii
  1 sibling, 0 replies; 6+ messages in thread
From: Richard Copley @ 2022-08-24 14:15 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 57353

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.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
  2022-08-23 13:21 ` Eli Zaretskii
  2022-08-24 14:15   ` Richard Copley
@ 2022-08-24 14:15   ` Eli Zaretskii
  2022-08-24 14:23     ` Richard Copley
  1 sibling, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2022-08-24 14:15 UTC (permalink / raw)
  To: rcopley; +Cc: 57353

> 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)





^ permalink raw reply related	[flat|nested] 6+ messages in thread

* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
  2022-08-24 14:15   ` Eli Zaretskii
@ 2022-08-24 14:23     ` Richard Copley
  2022-08-24 16:21       ` Eli Zaretskii
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Copley @ 2022-08-24 14:23 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 57353

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.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#57353: [PATCH] Fix parse-colon-path with UNC directory names
  2022-08-24 14:23     ` Richard Copley
@ 2022-08-24 16:21       ` Eli Zaretskii
  0 siblings, 0 replies; 6+ messages in thread
From: Eli Zaretskii @ 2022-08-24 16:21 UTC (permalink / raw)
  To: Richard Copley; +Cc: 57353-done

> 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.





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-08-24 16:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-23 11:34 bug#57353: [PATCH] Fix parse-colon-path with UNC directory names Richard Copley
2022-08-23 13:21 ` Eli Zaretskii
2022-08-24 14:15   ` Richard Copley
2022-08-24 14:15   ` Eli Zaretskii
2022-08-24 14:23     ` Richard Copley
2022-08-24 16:21       ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.