bug#26911: 25.2; eshell "cd .." doesn't work correctly with TRAMP

[Paul Eggert]

I ran into the expand-file-name bug outside of Tramp, and fixed it by installing
the attached patch into Emacs master. I hope it fixes Bug#26911 too.

With luck it (or something like it) might even bear on Bug#34834 too, so I'll cc
this message there. (I don't use MS-Windows or Tramp so am not good at testing
in those environments.)

[Michael Albinus]

Paul Eggert <egg...@cs.ucla.edu> writes:

Hi Paul,

I confirm it is fixed for bug#26911, so I close this bug.

Whether it is fixed also for bug#34384 I cannot check due to the lack of
an MS Windows machine.

Best regards, Michael.

[Mattias Engdegård]

No doubt the change (14fb657ba82) is fine in isolation, but now if Emacs is started from $HOME/somedir and I do find-file, the minibuffer prompt is "/home/mattias/somedir/" instead of "~/somedir/" which does not seem to be an improvement.

Worse, if cwd is $HOME, the minibuffer prompt becomes "~" instead of "~/" which is inconvenient since that slash has to be typed explicitly.

If nobody else is observing the effect then I'm doing something wrong but I'm not sure what.

[Eli Zaretskii]

> From: Mattias Engdegård <matt...@acm.org>
> Date: Thu, 27 Aug 2020 20:31:59 +0200
> Cc: Michael Albinus <michael...@gmx.de>, 26...@debbugs.gnu.org,
> Eli Zaretskii <el...@gnu.org>,
> Yegor Timoshenko <yegorti...@gmail.com>

>
> If nobody else is observing the effect then I'm doing something wrong but I'm not sure what.

I see it as well, FWIW.

[Stephen Berman]

So do I.

Steve Berman

[Paul Eggert]

On 8/27/20 11:31 AM, Mattias Engdegård wrote:
> No doubt the change (14fb657ba82) is fine in isolation, but now if Emacs is started from $HOME/somedir and I do find-file, the minibuffer prompt is "/home/mattias/somedir/" instead of "~/somedir/" which does not seem to be an improvement.
>
> Worse, if cwd is $HOME, the minibuffer prompt becomes "~" instead of "~/" which is inconvenient since that slash has to be typed explicitly.

Thanks for reporting that. Sigh, too often when I fix one bug in
expand-file-name I introduce another. I installed the attached patch to fix this
bug (I and I hope it doesn't introduce yet another :-).

[Eli Zaretskii]

> Cc: Michael Albinus <michael...@gmx.de>, 26...@debbugs.gnu.org,
> Eli Zaretskii <el...@gnu.org>, Yegor Timoshenko <yegorti...@gmail.com>

> From: Paul Eggert <egg...@cs.ucla.edu>
> Date: Thu, 27 Aug 2020 14:53:52 -0700

>
> Thanks for reporting that. Sigh, too often when I fix one bug in
> expand-file-name I introduce another. I installed the attached patch to fix this
> bug (I and I hope it doesn't introduce yet another :-).

Two tests (including the new one) fail here on MS-Windows:

Test fileio-tests--HOME-trailing-slash backtrace:
signal(ert-test-failed (((should (equal (expand-file-name "~") (expa
ert-fail(((should (equal (expand-file-name "~") (expand-file-name ho
(if (unwind-protect (setq value-197 (apply fn-195 args-196)) (setq f
(let (form-description-199) (if (unwind-protect (setq value-197 (app
(let ((value-197 'ert-form-evaluation-aborted-198)) (let (form-descr
(let* ((fn-195 #'equal) (args-196 (condition-case err (let ((signal-
(let ((home (car --dolist-tail--))) (setenv "HOME" home) (let* ((fn-
(while --dolist-tail-- (let ((home (car --dolist-tail--))) (setenv "
(let ((--dolist-tail-- '("/a/b/c" "/a/b/c/"))) (while --dolist-tail-
(let ((old-home (getenv "HOME"))) (let ((--dolist-tail-- '("/a/b/c"
(closure (t) nil (let ((old-home (getenv "HOME"))) (let ((--dolist-t
ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
ert-run-test(#s(ert-test :name fileio-tests--HOME-trailing-slash :do
ert-run-or-rerun-test(#s(ert--stats :selector (not ...) :tests [...
ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
ert-run-tests-batch((not (tag :unstable)))
ert-run-tests-batch-and-exit((not (tag :unstable)))
eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)
command-line-1(("-L" ";." "-l" "ert" "-l" "src/fileio-tests.el" "--e
command-line()
normal-top-level()
Test fileio-tests--HOME-trailing-slash condition:
(ert-test-failed
((should
(equal
(expand-file-name "~")
(expand-file-name home)))
:form
(equal "d:/gnu/git/emacs/trunk/test/a/b/c/" "d:./a/b/c")
:value nil :explanation
(arrays-of-different-length 34 9 "d:/gnu/git/emacs/trunk/test/a/b/c/" "d:./a/b/c" first-mismatch-at 2)))
FAILED 1/12 fileio-tests--HOME-trailing-slash (0.000000 sec)

Test fileio-tests--expand-file-name-trailing-slash backtrace:
signal(ert-test-failed (((should (equal (expand-file-name fooslashal
ert-fail(((should (equal (expand-file-name fooslashalias "/") "/foo/
(if (unwind-protect (setq value-202 (apply fn-200 args-201)) (setq f
(let (form-description-204) (if (unwind-protect (setq value-202 (app
(let ((value-202 'ert-form-evaluation-aborted-203)) (let (form-descr
(let* ((fn-200 #'equal) (args-201 (condition-case err (let ((signal-
(let ((fooslashalias (car --dolist-tail--))) (let* ((fn-200 #'equal)
(while --dolist-tail-- (let ((fooslashalias (car --dolist-tail--)))
(let ((--dolist-tail-- '("foo/" "foo//" "foo/." "foo//." "foo///././
(closure (t) nil (let ((--dolist-tail-- '("foo/" "foo//" "foo/." "fo
ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
ert-run-test(#s(ert-test :name fileio-tests--expand-file-name-traili
ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
ert-run-tests-batch((not (tag :unstable)))
ert-run-tests-batch-and-exit((not (tag :unstable)))
eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)
command-line-1(("-L" ";." "-l" "ert" "-l" "src/fileio-tests.el" "--e
command-line()
normal-top-level()
Test fileio-tests--expand-file-name-trailing-slash condition:
(ert-test-failed
((should
(equal
(expand-file-name fooslashalias "/")
"/foo/"))
:form
(equal "d:/foo/" "/foo/")
:value nil :explanation
(arrays-of-different-length 7 5 "d:/foo/" "/foo/" first-mismatch-at 0)))

[Eli Zaretskii]

> Date: Fri, 28 Aug 2020 09:39:38 +0300
> From: Eli Zaretskii <el...@gnu.org>
> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com

>
> > Cc: Michael Albinus <michael...@gmx.de>, 26...@debbugs.gnu.org,
> > Eli Zaretskii <el...@gnu.org>, Yegor Timoshenko <yegorti...@gmail.com>
> > From: Paul Eggert <egg...@cs.ucla.edu>
> > Date: Thu, 27 Aug 2020 14:53:52 -0700
> >
> > Thanks for reporting that. Sigh, too often when I fix one bug in
> > expand-file-name I introduce another. I installed the attached patch to fix this
> > bug (I and I hope it doesn't introduce yet another :-).
>
> Two tests (including the new one) fail here on MS-Windows:

I've now fixed most of the failures, but one still remains, and it
definitely seems to be due to the latest changes in expand-file-name,
note the "c:." part below:

Test fileio-tests--expand-file-name-trailing-slash backtrace:
signal(ert-test-failed (((should (equal (expand-file-name (concat "c
ert-fail(((should (equal (expand-file-name (concat "c:/" fooslashali
(if (unwind-protect (setq value-207 (apply fn-205 args-206)) (setq f
(let (form-description-209) (if (unwind-protect (setq value-207 (app
(let ((value-207 'ert-form-evaluation-aborted-208)) (let (form-descr
(let* ((fn-205 #'equal) (args-206 (condition-case err (let ((signal-
(progn (let* ((fn-200 #'equal) (args-201 (condition-case err (let ((
(if (memq system-type '(windows-nt ms-dos)) (progn (let* ((fn-200 #'
(let ((fooslashalias (car --dolist-tail--))) (if (memq system-type '

(while --dolist-tail-- (let ((fooslashalias (car --dolist-tail--)))
(let ((--dolist-tail-- '("foo/" "foo//" "foo/." "foo//." "foo///././
(closure (t) nil (let ((--dolist-tail-- '("foo/" "foo//" "foo/." "fo
ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
ert-run-test(#s(ert-test :name fileio-tests--expand-file-name-traili
ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
ert-run-tests-batch((not (tag :unstable)))
ert-run-tests-batch-and-exit((not (tag :unstable)))
eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)
command-line-1(("-L" ";." "-l" "ert" "-l" "src/fileio-tests.el" "--e
command-line()
normal-top-level()
Test fileio-tests--expand-file-name-trailing-slash condition:
(ert-test-failed
((should
(equal

(expand-file-name ...)
"c:/foo/"))
:form
(equal "c:./foo/" "c:/foo/")
:value nil :explanation
(arrays-of-different-length 8 7 "c:./foo/" "c:/foo/" first-mismatch-at 2)))
FAILED 5/12 fileio-tests--expand-file-name-trailing-slash (0.000000 sec)

[Eli Zaretskii]

> Date: Fri, 28 Aug 2020 10:01:43 +0300

> From: Eli Zaretskii <el...@gnu.org>
> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
>

> I've now fixed most of the failures, but one still remains, and it
> definitely seems to be due to the latest changes in expand-file-name,
> note the "c:." part below:

This is one symptom of a more general (and much more serious) problem
with the modified expand-file-name:

(expand-file-name "d:/foo/bar/../baz") => "d:./foo/baz"

That period after the colon following the drive letter shouldn't be
there. As you may imagine, many Emacs commands are now broken because
of this. This must be fixed ASAP.

[Paul Eggert]

On 8/28/20 3:48 AM, Eli Zaretskii wrote:
> That period after the colon following the drive letter shouldn't be
> there. As you may imagine, many Emacs commands are now broken because
> of this. This must be fixed ASAP.

I installed the attached patch to revert the recent expand-file-changes in the
DOS_NT case, which should fix the problem you mentioned.

This part of fileio.c is hard to follow because of the #ifdef DOS_NT and #ifdef
WINDOWSNT and #ifdef MSDOS and whatnot. How about if we move the
MS-Windows-specific code to a different source file instead of having that
forest of ifdefs in fileio.c? As things stand, it's hard to maintain the
mainline GNU code, because the way everything's arranged the Microsoft-specific
stuff significantly obfuscates everything else.

[Eli Zaretskii]

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com

> From: Paul Eggert <egg...@cs.ucla.edu>
> Date: Fri, 28 Aug 2020 22:52:28 -0700

>
> > That period after the colon following the drive letter shouldn't be
> > there. As you may imagine, many Emacs commands are now broken because
> > of this. This must be fixed ASAP.
>
> I installed the attached patch to revert the recent expand-file-changes in the
> DOS_NT case, which should fix the problem you mentioned.

Thanks, it does. But it produces a different problem:

(expand-file-name "." "c:/foo/bar/") => "c:/foo/bar

(note the absence of the trailing slash).

> This part of fileio.c is hard to follow because of the #ifdef DOS_NT and #ifdef
> WINDOWSNT and #ifdef MSDOS and whatnot. How about if we move the
> MS-Windows-specific code to a different source file instead of having that
> forest of ifdefs in fileio.c? As things stand, it's hard to maintain the
> mainline GNU code, because the way everything's arranged the Microsoft-specific
> stuff significantly obfuscates everything else.

Sorry, I'm not interested in messing with expand-file-name, as the
gains are insignificant, if there are any, and the potential problems
that could cause are a legion. I actually think that your latest set
of changes there was a mistake (for the same reasons), but as long as
you are prepared to fix the fallout, I won't actively object.

It took us a lot of blood, sweat, and tears to get to the point where
we are: that expand-file-name works correctly for all supported
systems (including DOS/Windows) and also the remote use case. We all
know how one of the gazillion use cases of that function can be easily
broken by a seemingly innocent change in its complex code. So I think
we should leave that function alone, and any problems with file names
(if they indeed are significant) should be fixed elsewhere.

[Paul Eggert]

>> I installed the attached patch to revert the recent expand-file-changes in the
>> DOS_NT case, which should fix the problem you mentioned.
>
> Thanks, it does. But it produces a different problem:
>
> (expand-file-name "." "c:/foo/bar/") => "c:/foo/bar
>
> (note the absence of the trailing slash).

That's what Emacs 27 does on MS-Windows, no? So it's not a regression, and the
problem can be fixed at the convenience of whoever's interested in hacking on
the MS-Windows side of the code.

Another way to put it is that Bug#26911 is now fixed for GNU and POSIX, but not
for MS-Windows. My earlier changes attempted to fix it for all platforms, but
this had undesirable side-effects in MS-Windows so I withdrew the MS-Windows
part of the changes. I have therefore reopened Bug#26911 since I assume it's
still present on MS-Windows.

Are some of the new test cases failing on MS-Windows? Should I arrange for these
test cases to be expected to fail on MS-Windows? If so, please let me know which
ones are failing, so that I can do that.

> I'm not interested in messing with expand-file-name

That's understandable as expand-file-name is quite a mess internally. But if
you're not interested in any attempt to clean up the mess, I guess I should
refrain from giving it a shot.

[Michael Albinus]

Paul Eggert <egg...@cs.ucla.edu> writes:

Hi Paul,

> Are some of the new test cases failing on MS-Windows? Should I arrange
> for these test cases to be expected to fail on MS-Windows? If so,
> please let me know which ones are failing, so that I can do that.

Maybe tramp-test05-expand-file-name fails now on MS-Windows. If somebody
could check it? If have no MS Windows machine to test.

Best regards, Michael.

[Eli Zaretskii]

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
> From: Paul Eggert <egg...@cs.ucla.edu>

> Date: Sat, 29 Aug 2020 09:46:29 -0700

>
> >> I installed the attached patch to revert the recent expand-file-changes in the
> >> DOS_NT case, which should fix the problem you mentioned.
> >
> > Thanks, it does. But it produces a different problem:
> >
> > (expand-file-name "." "c:/foo/bar/") => "c:/foo/bar
> >
> > (note the absence of the trailing slash).
>
> That's what Emacs 27 does on MS-Windows, no? So it's not a regression, and the
> problem can be fixed at the convenience of whoever's interested in hacking on
> the MS-Windows side of the code.

It's a regression wrt behavior on Posix platforms: it fails one of the
tests in fileio-tests.el:

Test fileio-tests--HOME-trailing-slash condition:
(ert-test-failed
((should
(equal

(expand-file-name "~")
(expand-file-name home)))
:form

(equal "c:/a/b/c" "c:/a/b/c/")
:value nil :explanation
(arrays-of-different-length 8 9 "c:/a/b/c" "c:/a/b/c/" first-mismatch-at 8)))

FAILED 1/12 fileio-tests--HOME-trailing-slash (0.000000 sec)

> Another way to put it is that Bug#26911 is now fixed for GNU and POSIX, but not
> for MS-Windows. My earlier changes attempted to fix it for all platforms, but
> this had undesirable side-effects in MS-Windows so I withdrew the MS-Windows
> part of the changes. I have therefore reopened Bug#26911 since I assume it's
> still present on MS-Windows.

If that is the case, I prefer that we revert all the changes made
recently to fix bug#26911, and leave that bug open, until a fix is
available that works on all platforms.

> > I'm not interested in messing with expand-file-name
>
> That's understandable as expand-file-name is quite a mess internally. But if
> you're not interested in any attempt to clean up the mess, I guess I should
> refrain from giving it a shot.

Fine with me, then let's revert the recent changes and go back to what
we had before.

Thanks for trying to fix that.

[Eli Zaretskii]

> From: Michael Albinus <michael...@gmx.de>
> Cc: Eli Zaretskii <el...@gnu.org>, 26...@debbugs.gnu.org, matt...@acm.org,
> yegorti...@gmail.com
> Date: Sat, 29 Aug 2020 18:59:37 +0200

>
> Maybe tramp-test05-expand-file-name fails now on MS-Windows. If somebody
> could check it? If have no MS Windows machine to test.

I tried running "make tramp-tests", but all I get is this:

GEN lisp/net/tramp-tests.log
Not a Tramp file name: "NUL"

Any suggestions?

[Michael Albinus]

Eli Zaretskii <el...@gnu.org> writes:

Hi Eli,

>> Maybe tramp-test05-expand-file-name fails now on MS-Windows. If somebody
>> could check it? If have no MS Windows machine to test.
>
> I tried running "make tramp-tests", but all I get is this:
>
> GEN lisp/net/tramp-tests.log
> Not a Tramp file name: "NUL"
>
> Any suggestions?

Hmm, yes: on MS Windows, the mock trick doesn't work. I hoped, that at
least the tests which do not need a real connection do work.

Well, if you have a remote host reachable via putty, which has also a
/tmp directory, you could try

set REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp

prior the make call.

Thanks, and best regards, Michael.

[Eli Zaretskii]

> From: Michael Albinus <michael...@gmx.de>
> Cc: egg...@cs.ucla.edu, 26...@debbugs.gnu.org, matt...@acm.org,
> yegorti...@gmail.com
> Date: Sat, 29 Aug 2020 21:12:30 +0200

>
> > I tried running "make tramp-tests", but all I get is this:
> >
> > GEN lisp/net/tramp-tests.log
> > Not a Tramp file name: "NUL"
> >
> > Any suggestions?
>
> Hmm, yes: on MS Windows, the mock trick doesn't work. I hoped, that at
> least the tests which do not need a real connection do work.
>
> Well, if you have a remote host reachable via putty, which has also a
> /tmp directory, you could try
>
> set REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp
>
> prior the make call.

This variant seems to work:

REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp make lisp/net/tramp-tests

Most of the tests are "skipped", but the one you were interested in
isn't skipped, and indeed fails:

Test tramp-test05-expand-file-name condition:
(ert-test-failed
((should
(string-equal
(expand-file-name "/method:host:/path/.")
(if ... "/method:host:/path/" "/method:host:/path")))
:form
(string-equal "/method:host:/path" "/method:host:/path/")
:value nil))
FAILED 12/70 tramp-test05-expand-file-name (0.000000 sec)

HTH

[Paul Eggert]

On 8/29/20 11:26 AM, Eli Zaretskii wrote:

>> That's what Emacs 27 does on MS-Windows, no? So it's not a regression, and the
>> problem can be fixed at the convenience of whoever's interested in hacking on
>> the MS-Windows side of the code.
>
> It's a regression wrt behavior on Posix platforms: it fails one of the
> tests in fileio-tests.el:

That's not a regression in the usual sense of the word, since Emacs master on
MS-Windows is behaving the same way it did in Emacs 27. It's merely a bug that
has been fixed on most platforms but not on MS-Windows.

The test you mentioned is newly-added and already special-cases for MS-Windows,
and it's easy to special-case it just a bit more. I installed the attached
little patch as a workaround until we can get the bug fixed in the MS-Windows
support code.

> If that is the case, I prefer that we revert all the changes made
> recently to fix bug#26911, and leave that bug open, until a fix is
> available that works on all platforms.

Although we should continue to leave the bug open (since it's still present on
MS-Windows), reverting would be the tail wagging the dog. We should not
reintroduce a bug on GNU and similar platforms merely because we haven't yet
found the time to fix the bug on MS-Windows.

It surely would be better to fix the bug on MS-Windows. A good way to start
doing that is to refactor the code a bit to avoid the tricky #ifdefs it
currently uses, as these #ifdefs make bugs like this painful to fix. I can draft
a patch along those lines if you like. I realize you're dubious about
refactoring and so wouldn't install the patch without checking with you.

If you prefer fixing it a different way of course feel free to suggest
something. Since I don't use MS-Windows your expertise would be helpful.

[Michael Albinus]

Eli Zaretskii <el...@gnu.org> writes:

Hi Eli,

> This variant seems to work:
>
> REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp make lisp/net/tramp-tests
>
> Most of the tests are "skipped", but the one you were interested in
> isn't skipped, and indeed fails:
>
> Test tramp-test05-expand-file-name condition:
> (ert-test-failed
> ((should
> (string-equal
> (expand-file-name "/method:host:/path/.")
> (if ... "/method:host:/path/" "/method:host:/path")))
> :form
> (string-equal "/method:host:/path" "/method:host:/path/")
> :value nil))
> FAILED 12/70 tramp-test05-expand-file-name (0.000000 sec)

This is as expected, thanks.

I'm a little bit undecided whether I do special-case the test for
running on MS Windows, or whether I wait that it will be fixed. WDYT?

> HTH

Best regards, Michael.

[Paul Eggert]

On 8/30/20 7:09 AM, Eli Zaretskii wrote:

> you alluded to another use case, unrelated to
> remote file names, but didn't provide any details.
Here's an example. On RHEL 7.8 (file-symlink-p "/bin/.") returned "usr/bin"
which was a bug since "/bin/." is not (and cannot possibly be) a symbolic link.
This bug occurred because file-symlink-p calls expand-file-name which
incorrectly stripped trailing "/." from the file name before checking the file's
status. This sort of behavior broke code in startup.el that used file-attributes
(which had the same bug) to compare $PWD to the working directory's name, which
is how I ran into the bug again. (I vaguely recall running into this bug earlier
but lacked time/energy then to track it down and fix it.)

> Is that other use case really similar to the one
> which started this bug report

I expect they're related if we look at the mess inside file-attributes. They may
not appear to be similar to users who don't know how Emacs is implemented.

> I see no reason to require expand-file-name to preserve the
> trailing slash

It's required because trailing slash affects how file names are interpreted on
GNU and other POSIXish platforms. Emacs should not second-guess GNU and POSIX on
this: it should interpret file names like the underlying platforms do, as
anything else would be unnecessarily confusing.

> IMO the problem is immediately following the above snippet:
>
> /* Keep initial / only if this is the whole name. */
> if (o == target && IS_ANY_SEP (*o) && p[3] == 0)
> ++o;
>
> This is very easy to fix without affecting any other uses of the
> function: we should consider one other case in addition to "only if /
> is the whole name" -- the case where this fails to DTRT with remote
> directories.

Such a fix should be no problem for the GNU/POSIXish side, as that snippet is in
the DOS_NT code and any fixes there should affect only MS-Windows and DOS. I
don't know what a "remote directory" is in that context, though, so I can't give
specific advice.

> Its code is complex and full of subtle dark
> corners, many of which are not well covered by our test suite.

expand-file-name is more complex than it needs to be, and its dark corners would
be less dark if we cleaned it up a bit. In refactoring I would not attempt
elegance, only understandability. Right now the code is needlessly hard to
understand, and that makes it hard to fix - something I encountered while trying
to fix some of the abovementioned bugs.

[Eli Zaretskii]

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
> From: Paul Eggert <egg...@cs.ucla.edu>

> Date: Sun, 30 Aug 2020 14:39:28 -0700

>
> This bug occurred because file-symlink-p calls expand-file-name which
> incorrectly stripped trailing "/." from the file name before checking the file's
> status.

It is not expand-file-name's job to know about these subtleties.
expand-file-name deals only with the syntax of file names. It doesn't
know anything about the semantics of "." and ".." except that they can
be removed to bring the file name to a standard form. It doesn't know
whether a file is a directory or a symlink or something else; it
doesn't even care if the file exists. It isn't supposed to hit the
disk for its job.

It is therefore perfectly valid for it to remove the trailing "/."
without appending a slash. If file-symlink-p needs to handle such
file names specially, it should do it in its own code.

So this job should not be imposed on expand-file-name, and we should
remove the code added for that purpose.

> > I see no reason to require expand-file-name to preserve the
> > trailing slash
>
> It's required because trailing slash affects how file names are interpreted on
> GNU and other POSIXish platforms.

It is not the job of expand-file-name to interpret file names. Lisp
programs that need a directory's name to end in a slash should call
file-name-as-directory, this is why we have that function. If we
insist on appending the slash in all cases, then some code will
benefit, but other code will break (and will need to call
directory-file-name to avoid the breakage). There's no net win here,
so we should not do this, either.

expand-file-name is simply the wrong place for this kind of
functionality, even before we consider its complexity.

> > IMO the problem is immediately following the above snippet:
> >
> > /* Keep initial / only if this is the whole name. */
> > if (o == target && IS_ANY_SEP (*o) && p[3] == 0)
> > ++o;
> >
> > This is very easy to fix without affecting any other uses of the
> > function: we should consider one other case in addition to "only if /
> > is the whole name" -- the case where this fails to DTRT with remote
> > directories.
>
> Such a fix should be no problem for the GNU/POSIXish side, as that snippet is in
> the DOS_NT code and any fixes there should affect only MS-Windows and DOS. I
> don't know what a "remote directory" is in that context, though, so I can't give
> specific advice.

You are talking about the code after your changes, whereas I (and
Michael at the time he wrote that) were talking about the code before
your changes: then the above snippet affected all platforms.

> Right now the code is needlessly hard to understand, and that makes
> it hard to fix - something I encountered while trying to fix some of
> the abovementioned bugs.

It isn't hard for me to understand the current code, although it is
indeed complex (because the job it does is not trivial). But the
problem is not the complexity, the problem starts when we make changes
there which are either not strictly necessary, or affect more use
cases than the few we need to fix.

That function works, and works well. Let's not make it less
dependable than it is today.

Anyway, I think I understand all the issues now, so I will work on
fixing them soon in a way that will avoid unnecessary fallout.

Thanks.

[Paul Eggert]

On 8/31/20 7:58 AM, Eli Zaretskii wrote:

> expand-file-name deals only with the syntax of file names.

Yes, but it does so under constraints imposed by semantics. This is why
expand-file-name can't simply remove *all* slashes from file names (which would
be just a "syntax" thing, no? :-).

On GNU and other POSIXish systems, expand-file-names is entitled to do its
syntactic manipulations only because because of the POSIX rules that "." means
the working directory, leading "/" means the file name is absolute, trailing "/"
means the file name is that of a directory, and so forth.

expand-file-name can simplify "/./" to "/", even though it cannot always
simplify "/." to "" (and it cannot simply remove *all* slashes :-), because
expand-file-name's syntactic manipulations simplify the file name in a safe way
that does not change the file name's meaning. (This principle has one
well-documented exception for symbolic links that do not point to sibling
directories, but that does not overturn the principle elsewhere.)

> It is therefore perfectly valid for it to remove the trailing "/."
> without appending a slash.

Not at all. In many cases that would change the meaning of the file name, and
expand-file-name is not supposed to do that. On GNU and POSIXish platforms it is
valid to remove trailing "/." in some cases (e.g., "/foo//.") but it is
definitely not valid to do it in all cases.

> It is not the job of expand-file-name to interpret file names.

That depends on what one means by "interpret". It is the job of expand-file-name
to simplify file names under standard assumptions consistent with the underlying
platform's behavior. If a "simplification" would disagree with the behavior of
the underlying platform, that would cause needless confusion and
expand-file-name should not do that.

> Lisp programs that need a directory's name to end in a slash should call
> file-name-as-directory, this is why we have that function.

That is a separate point, and is not directly relevant to whether
expand-file-name should change a file name's meaning by removing slashes from it.

> If we insist on appending the slash in all cases

Nobody is insisting on that.

All I'm saying is that if the user has put a slash in a file name,
expand-file-name should not remove the slash if the removal would change the
file name's meaning. This is a simple principle that is easy to explain to
users. No other principle would make nearly as much sense.

>>> IMO the problem is immediately following the above snippet:
>>>
>>> /* Keep initial / only if this is the whole name. */
>>> if (o == target && IS_ANY_SEP (*o) && p[3] == 0)
>>> ++o;
>>>
>>> This is very easy to fix without affecting any other uses of the
>>> function: we should consider one other case in addition to "only if /
>>> is the whole name" -- the case where this fails to DTRT with remote
>>> directories.
>>
>> Such a fix should be no problem for the GNU/POSIXish side, as that snippet is in
>> the DOS_NT code and any fixes there should affect only MS-Windows and DOS. I
>> don't know what a "remote directory" is in that context, though, so I can't give
>> specific advice.
>
> You are talking about the code after your changes, whereas I (and
> Michael at the time he wrote that) were talking about the code before
> your changes: then the above snippet affected all platforms.

Feel free to alter the code to fix these bugs in a different way. However, I
expect any such fix will be simpler if starts with the current code (which fixes
the bugs on GNU and other POSIX hosts) instead of with the older code (which
does not).

[Eli Zaretskii]

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
> From: Paul Eggert <egg...@cs.ucla.edu>

> Date: Mon, 31 Aug 2020 11:15:35 -0700

>
> On 8/31/20 7:58 AM, Eli Zaretskii wrote:
>
> > expand-file-name deals only with the syntax of file names.
>
> Yes, but it does so under constraints imposed by semantics. This is why
> expand-file-name can't simply remove *all* slashes from file names (which would
> be just a "syntax" thing, no? :-).

No, because a valid syntax of an absolute file name is to start with a
slash.

> > It is therefore perfectly valid for it to remove the trailing "/."
> > without appending a slash.
>
> Not at all.

We disagree. So any further argument is fruitless, because I will not
change my mind on this. I'm pretty sure I'm right because this
function never did anything different from what I describe, until very
recently.

> In many cases that would change the meaning of the file name, and
> expand-file-name is not supposed to do that.

Once again, the meaning of a file name is out of scope of
expand-file-name's job.

> Feel free to alter the code to fix these bugs in a different way. However, I
> expect any such fix will be simpler if starts with the current code

I intend to fix the specific bugs that were reported, and will try
very hard not to alter any other behavior, but my baseline is how
expand-file-name behaved before your changes, not how it behaves now
(which is wrong).

[Paul Eggert]

On 8/31/20 11:56 AM, Eli Zaretskii wrote:
>>> expand-file-name deals only with the syntax of file names.
>> Yes, but it does so under constraints imposed by semantics. This is why

>> expand-file-name can't simply remove*all* slashes from file names (which would

>> be just a "syntax" thing, no? :-).
> No, because a valid syntax of an absolute file name is to start with a
> slash.

Ending with a slash is just as much syntax as starting with a slash is. The
meaning (absolute versus relative for starting slash, or directory versus file
for ending slash) is a consequence of the syntax in both cases. In neither case
should expand-file-name remove the slash, unless it can determine that removing
the slash does not change the meaning of the name (which it can do in some cases
but not in all).

> We disagree. So any further argument is fruitless

That's not a good way to resolve the disagreement. A better way is for me to see
what changes you make or plan to make to expand-file-name. If these changes
handle file names on GNU and POSIX platforms consistently with other GNU
applications, everything will be OK. It's possible we are simply
misunderstanding each other, after all.

[Eli Zaretskii]

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
> From: Paul Eggert <egg...@cs.ucla.edu>

> Date: Mon, 31 Aug 2020 16:36:41 -0700

>
> > We disagree. So any further argument is fruitless
>
> That's not a good way to resolve the disagreement.

It's not the best one, but I don't see how else we could resolve such
a basic disagreement.

[Eli Zaretskii]

> Date: Mon, 31 Aug 2020 17:58:17 +0300
> From: Eli Zaretskii <el...@gnu.org>

> Cc: 26...@debbugs.gnu.org, matt...@acm.org, michael...@gmx.de,
> yegorti...@gmail.com
>

> Anyway, I think I understand all the issues now, so I will work on
> fixing them soon in a way that will avoid unnecessary fallout.

Now done. I reverted most of the changes we've been discussing
lately, and instead installed a simpler change which fixes both this
bug and bug#34834, and doesn't affect any unrelated use cases.

Michael, I'd appreciate Tramp-related testing. I've ran the standard
tests from the test suite, but I'm sure you have more, including some
tests that are normally disabled.

I believe this bug and bug#34834 can now be closed.

Thanks.

[Michael Albinus]

Eli Zaretskii <el...@gnu.org> writes:

Hi Eli,

> Michael, I'd appreciate Tramp-related testing. I've ran the standard
> tests from the test suite, but I'm sure you have more, including some
> tests that are normally disabled.

Will do. I'm slow these days due to private issues, sorry.

> Thanks.

Best regards, Michael.