unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
	yegortimoshenko@gmail.com
Subject: bug#26911: 25.2; eshell "cd .." doesn't work correctly with TRAMP
Date: Mon, 31 Aug 2020 17:58:17 +0300	[thread overview]
Message-ID: <83sgc2x3p2.fsf@gnu.org> (raw)
In-Reply-To: <6acf8cfa-6071-e7b1-3055-04292634bb39@cs.ucla.edu> (message from Paul Eggert on Sun, 30 Aug 2020 14:39:28 -0700)

> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
>  yegortimoshenko@gmail.com
> From: Paul Eggert <eggert@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.





  reply	other threads:[~2020-08-31 14:58 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-13 16:15 bug#26911: 25.2; eshell "cd .." doesn't work correctly with TRAMP Yegor Timoshenko
2017-05-13 18:25 ` Michael Albinus
2017-05-13 18:30   ` Yegor Timoshenko
2017-05-15 15:53     ` Michael Albinus
2020-08-26 20:39 ` Paul Eggert
2020-08-27 11:46   ` Michael Albinus
2020-08-27 18:31 ` Mattias Engdegård
2020-08-27 18:38   ` Eli Zaretskii
2020-08-27 18:54     ` Stephen Berman
2020-08-27 21:53   ` Paul Eggert
2020-08-28  6:39     ` Eli Zaretskii
2020-08-28  7:01       ` Eli Zaretskii
2020-08-28 10:48         ` Eli Zaretskii
2020-08-29  5:52           ` Paul Eggert
2020-08-29  6:31             ` Eli Zaretskii
2020-08-29 16:46               ` Paul Eggert
2020-08-29 16:59                 ` Michael Albinus
2020-08-29 18:29                   ` Eli Zaretskii
2020-08-29 19:12                     ` Michael Albinus
2020-08-29 19:31                       ` Eli Zaretskii
2020-08-30  9:46                         ` Michael Albinus
2020-08-30 14:14                           ` Eli Zaretskii
2020-08-29 18:26                 ` Eli Zaretskii
2020-08-29 20:42                   ` Paul Eggert
2020-08-30 14:09                     ` Eli Zaretskii
2020-08-30 21:39                       ` Paul Eggert
2020-08-31 14:58                         ` Eli Zaretskii [this message]
2020-08-31 18:15                           ` Paul Eggert
2020-08-31 18:56                             ` Eli Zaretskii
2020-08-31 23:36                               ` Paul Eggert
2020-09-01  2:33                                 ` Eli Zaretskii
2020-09-03 17:27                           ` Eli Zaretskii
2020-09-03 17:42                             ` Michael Albinus
2020-09-04 11:55                             ` Michael Albinus
2020-09-04 12:25                               ` Eli Zaretskii
2020-09-04 13:53                                 ` Michael Albinus
2020-09-04 14:40                                   ` Eli Zaretskii
2020-09-04 15:20                                     ` Eli Zaretskii
2020-09-04 15:59                                       ` Michael Albinus
2020-09-04 17:42                                         ` Eli Zaretskii
2020-09-05  8:34                                           ` Michael Albinus
2020-09-05 11:18                                             ` Eli Zaretskii
2020-09-05 11:32                                               ` Eli Zaretskii
2020-09-05 15:57                                               ` Michael Albinus
2020-09-05 16:33                                                 ` Eli Zaretskii
2020-09-05 17:08                                                   ` Eli Zaretskii
2020-09-05 17:36                                                   ` Michael Albinus
2020-09-05 17:56                                                     ` Eli Zaretskii
2020-09-04 16:09                                     ` Michael Albinus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83sgc2x3p2.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=26911@debbugs.gnu.org \
    --cc=eggert@cs.ucla.edu \
    --cc=mattiase@acm.org \
    --cc=michael.albinus@gmx.de \
    --cc=yegortimoshenko@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).