GNU bug report logs - #49918
28.0.50; cd function expands CDPATH incorrectly

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 49918 in the body.
You can then email your comments to 49918 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: Phil Hagelberg <phil <at> hagelb.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; cd function expands CDPATH incorrectly
Date: Fri, 06 Aug 2021 18:18:10 -0700

When using the function `cd' with a relative path and the $CDPATH
environment variable set, it will expand the value of $CDPATH once and
save off that expansion for future use. This causes all calls with
relative paths to fail when run from a different directory.

This reproduces the problem using eshell, but the bug affects any caller
of the `cd' function, not just eshell. However, it does not affect M-x
cd, which will always use absolute paths.

    CDPATH=.:$HOME/src emacs -Q
    M-x eshell
    cd /
    cd tmp
    No such directory found via CDPATH environment variable: tmp

This happens because of this snippet inside the `cd' function:

    (unless cd-path
      (setq cd-path (or (parse-colon-path (getenv "CDPATH"))
                        (list "./"))))

The behavior of the `parse-colon-path' function changed in 2020 in
commit 76098d39c992aa51f5bdb04fb39e40fc5eb409d5:

-    (mapcar (lambda (f)
-	      (if (equal "" f) nil
-		(substitute-in-file-name (file-name-as-directory f))))
-	    (split-string search-path path-separator))))
+    (let ((spath (substitute-env-vars search-path)))
+      (mapcar (lambda (f)
+                (if (equal "" f) nil
+                  (let ((dir (expand-file-name (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))))
+              (split-string spath path-separator)))))

Previous behavior did not expand relative paths like "." but the new
behavior does. That means that the above call which sets cd-path cannot
use this function as is. Either the old behavior of parse-colon-path
will need to be brought back, or `cd' needs to no longer call that function.

Witness on the current master branch: ($PWD here is ~/src/emacs)

    (parse-colon-path ".:/hey") : -> ("/home/phil/src/emacs/" "/hey/")

Whereas on 27, you get this:

    (parse-colon-path ".:/hey") : -> ("./" "/hey/")

The old behavior is safe to set for the value of `cd-path' but the new
behavior is not.


In GNU Emacs 28.0.50 (build 1, aarch64-unknown-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-08-06 built on reform
Repository revision: 0b049fe71d73f6885a3c81ea31829e3befc18933
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12099001
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-cairo --with-modules --without-compress-install
 --with-gnutls --without-gconf --without-xaw3d --without-gsettings
 --with-mailutils --with-native-compilation --with-json --with-harfbuzz
 --without-imagemagick --with-jpeg --with-png --with-rsvg --with-tiff
 --with-wide-int --with-xft --with-xml2 --with-xpm 'CFLAGS=-O3
 -mtune=native -march=native -fomit-frame-pointer' prefix=/usr/local'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Diff

Minor modes in effect:
  recentf-mode: t
  whitespace-mode: t
  global-company-mode: t
  company-mode: t
  shell-dirtrack-mode: t
  winner-mode: t
  show-paren-mode: t
  save-place-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
~/.emacs.d//phil/../custom hides /home/phil/src/emacs/lisp/custom

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs rfc822
mml mml-sec epa derived epg epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail hippie-exp
sh-script executable smerge-mode diff flyspell ispell mhtml-mode
css-mode smie eww xdg url-queue shr kinsoku svg xml browse-url puny
mm-url gnus nnheader gnus-util rmail rmail-loaddefs rfc2047 rfc2045
ietf-drums mail-utils mm-util mail-prsvr js cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs sgml-mode
facemenu dom scpaste htmlize url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util url-parse
auth-source password-cache json map url-vars mailcap cl-print pulse
color xref project eieio eieio-core eieio-loaddefs ielm pp tabify
vc-annotate imenu hi-lock misearch multi-isearch add-log log-view
pcvs-util vc vc-git diff-mode easy-mmode vc-dispatcher bug-reference
find-func shortdoc text-property-search em-tramp em-xtra help-fns
radix-tree recentf tree-widget wid-edit pcmpl-unix time-date paredit
whitespace idle-highlight-mode thingatpt hl-line company-dabbrev-code
company-dabbrev company-files company-capf company pcase em-unix em-term
term disp-table shell ehelp em-script em-prompt em-ls em-hist em-pred
em-glob em-dirs esh-var em-cmpl pcomplete comint ansi-color em-basic
em-banner em-alias esh-mode eshell esh-cmd esh-ext esh-opt esh-proc
esh-io esh-arg esh-module esh-groups esh-util winner ring smex comp
comp-cstr warnings subr-x rx cl-seq cl-macs cl-extra help-mode advice
my-autoload paren edmacro kmacro cl-loaddefs cl-lib saveplace ido seq
byte-opt gv bytecomp byte-compile cconv iso-transl tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win
x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting font-render-setting cairo move-toolbar gtk x-toolkit x
multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 348427 19451)
 (symbols 48 19549 1)
 (strings 32 73704 4571)
 (string-bytes 1 2519167)
 (vectors 16 37842)
 (vector-slots 8 705331 23605)
 (floats 8 380 691)
 (intervals 56 21516 4320)
 (buffers 992 29))

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Phil Hagelberg <phil <at> hagelb.org>
Cc: 49918 <at> debbugs.gnu.org
Subject: Re: bug#49918: 28.0.50; cd function expands CDPATH incorrectly
Date: Sat, 07 Aug 2021 12:51:22 +0200

Phil Hagelberg <phil <at> hagelb.org> writes:

> When using the function `cd' with a relative path and the $CDPATH
> environment variable set, it will expand the value of $CDPATH once and
> save off that expansion for future use. This causes all calls with
> relative paths to fail when run from a different directory.
>
> This reproduces the problem using eshell, but the bug affects any caller
> of the `cd' function, not just eshell. However, it does not affect M-x
> cd, which will always use absolute paths.
>
>     CDPATH=.:$HOME/src emacs -Q
>     M-x eshell
>     cd /
>     cd tmp
>     No such directory found via CDPATH environment variable: tmp
>
> This happens because of this snippet inside the `cd' function:
>
>     (unless cd-path
>       (setq cd-path (or (parse-colon-path (getenv "CDPATH"))
>                         (list "./"))))

Yup.  Thanks for the detailed analysis.

This should now be fixed in Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: phil <at> hagelb.org, 49918 <at> debbugs.gnu.org
Subject: Re: bug#49918: 28.0.50; cd function expands CDPATH incorrectly
Date: Sat, 07 Aug 2021 15:17:18 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Sat, 07 Aug 2021 12:51:22 +0200
> Cc: 49918 <at> debbugs.gnu.org
> 
> > This happens because of this snippet inside the `cd' function:
> >
> >     (unless cd-path
> >       (setq cd-path (or (parse-colon-path (getenv "CDPATH"))
> >                         (list "./"))))
> 
> Yup.  Thanks for the detailed analysis.
> 
> This should now be fixed in Emacs 28.

Why does the current implementation of parse-colon-path use
expand-file-name?  The comment says "to expand "~", but the original
implementation in Emacs 27 didn't do that.

Bug#21454 only wanted to avoid mis-interpreting duplicate slashes in
the input path, but there's no need to collapse them, so I don't see
how the call to expand-file-name is at all necessary, and could
potentially change behavior in unintended ways.  Am I missing
something?

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

From: Phil Hagelberg <phil <at> hagelb.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 49918 <at> debbugs.gnu.org
Subject: Re: bug#49918: 28.0.50; cd function expands CDPATH incorrectly
Date: Sat, 07 Aug 2021 08:50:37 -0700

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi <at> gnus.org>
>> Date: Sat, 07 Aug 2021 12:51:22 +0200
>> Cc: 49918 <at> debbugs.gnu.org
>> 
>> > This happens because of this snippet inside the `cd' function:
>> >
>> >     (unless cd-path
>> >       (setq cd-path (or (parse-colon-path (getenv "CDPATH"))
>> >                         (list "./"))))
>> 
>> Yup.  Thanks for the detailed analysis.
>> 
>> This should now be fixed in Emacs 28.

Thanks for the fix! I can confirm it does solve the problem.

> Why does the current implementation of parse-colon-path use
> expand-file-name?  The comment says "to expand "~", but the original
> implementation in Emacs 27 didn't do that.
>
> Bug#21454 only wanted to avoid mis-interpreting duplicate slashes in
> the input path, but there's no need to collapse them, so I don't see
> how the call to expand-file-name is at all necessary, and could
> potentially change behavior in unintended ways.  Am I missing
> something?

I have to agree I don't understand the logic of the patch that added the
`expand-file-name' call. It doesn't seem related to the bug it was
intended to fix. It's certainly not necessary from the context of
`parse-colon-path' being called from `cd' but I didn't take the time to
examine other callers to see if it would be relevant from those
contexts. But it's not unreasonable to assume that expansion should
happen downstream of the call to `parse-colon-path'.

-Phil

[signature.asc (application/pgp-signature, inline)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: phil <at> hagelb.org, 49918 <at> debbugs.gnu.org
Subject: Re: bug#49918: 28.0.50; cd function expands CDPATH incorrectly
Date: Sun, 08 Aug 2021 14:42:46 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> Why does the current implementation of parse-colon-path use
> expand-file-name?  The comment says "to expand "~", but the original
> implementation in Emacs 27 didn't do that.

I should probably have written (etc) after that comment...

> Bug#21454 only wanted to avoid mis-interpreting duplicate slashes in
> the input path, but there's no need to collapse them, so I don't see
> how the call to expand-file-name is at all necessary, and could
> potentially change behavior in unintended ways.  Am I missing
> something?

Tino's tests in files-tests-bug-21454 test explicitly for the duplicate
slash collapsing, so I assumed that was part of the point.  But I may
well be mistaken -- feel free to adjust this some more.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: phil <at> hagelb.org, 49918 <at> debbugs.gnu.org
Subject: Re: bug#49918: 28.0.50; cd function expands CDPATH incorrectly
Date: Sun, 08 Aug 2021 17:32:23 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: phil <at> hagelb.org,  49918 <at> debbugs.gnu.org
> Date: Sun, 08 Aug 2021 14:42:46 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Why does the current implementation of parse-colon-path use
> > expand-file-name?  The comment says "to expand "~", but the original
> > implementation in Emacs 27 didn't do that.
> 
> I should probably have written (etc) after that comment...
> 
> > Bug#21454 only wanted to avoid mis-interpreting duplicate slashes in
> > the input path, but there's no need to collapse them, so I don't see
> > how the call to expand-file-name is at all necessary, and could
> > potentially change behavior in unintended ways.  Am I missing
> > something?
> 
> Tino's tests in files-tests-bug-21454 test explicitly for the duplicate
> slash collapsing, so I assumed that was part of the point.  But I may
> well be mistaken -- feel free to adjust this some more.

Done.  I don't think we should collapse slashes except the leading
one, it isn't something this function should do, and is not in its
documentation.

Previous Next

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