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

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

[Yegor Timoshenko]

Steps to reproduce:

M-x eshell
cd /user@remote.com:/
cd bin
cd ..
pwd

Expected output:

/user@remote.com:

Actual output:

/user@remote.com:/home/user

Reproducible in default Emacs 25.2 install:

In GNU Emacs 25.2.1 (x86_64-apple-darwin13.4.0, NS appkit-1265.21 Version 10.9.5 (Build 13F1911))
 of 2017-04-21 built on builder10-9.porkrind.org
Windowing system distributor 'Apple', version 10.3.1504
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

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

[Michael Albinus]

Yegor Timoshenko <yegortimoshenko@gmail.com> writes:

I could reproduce it. Seems to be like a problem in locate-file,
reproducible with

(let ((default-directory "/ssh::/bin"))
  (locate-file ".." '("./")))

I'll dig further.

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

[Yegor Timoshenko]

> I could reproduce it. Seems to be like a problem in locate-file,
> reproducible with
> 
> (let ((default-directory "/ssh::/bin"))
>  (locate-file ".." '("./")))
> 
> I'll dig further.

(cd "../") works (with a trailing slash).

Just to make it clear, as you've already pointed out, this is not specific to eshell:
cd and locate-file in files.el are also affected.

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

[Michael Albinus]

Yegor Timoshenko <yegortimoshenko@gmail.com> writes:

Looks to me like a bug in Fexpand_file_name, which does not handle file
name handlers properly. Try

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/ssh::/bin"))
  (expand-file-name ".." "./"))
--8<---------------cut here---------------end--------------->8---

In fileio.c, lines 1393-1394, the following loop

--8<---------------cut here---------------start------------->8---
	    while (o != target && (--o, !IS_DIRECTORY_SEP (*o)))
	      continue;
--8<---------------cut here---------------end--------------->8---

replaces "/ssh:host:/bin/.." by "/ssh:host:". But it should be
"/ssh:host:/". I'm not such familiar with that code to fix it.

Best regards, Michael.

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

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

[-- Attachment #2: 0001-Fix-expand-file-name-symlink-to-dir-bug.patch --]
[-- Type: text/x-patch, Size: 7842 bytes --]

From 14fb657ba82da346d36f05f88da26f1c5498b798 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 26 Aug 2020 13:25:35 -0700
Subject: [PATCH] Fix expand-file-name symlink-to-dir bug

Problem reported by Yegor Timoshenko (Bug#26911),
and I ran into it myself recently in normal-top-level.
* doc/lispref/files.texi (File Name Expansion), etc/NEWS: Mention this.
* src/fileio.c (Fexpand_file_name): Expand "/a/b/." to "/a/b/" not
"/a/b", to avoid misinterpreting a symlink "/a/b".  Similarly,
expand "/a/b/c/.." to "/a/b/" not "/a/b".
* test/lisp/net/tramp-tests.el (tramp-test05-expand-file-name):
Adjust to match new behavior.
(tramp-test05-expand-file-name-relative): This test now succeeds,
at least on Fedora 31.
* test/src/fileio-tests.el:
(fileio-tests--expand-file-name-trailing-slash)	New test.
---
 doc/lispref/files.texi       | 16 ++++++++++++++--
 etc/NEWS                     |  6 ++++++
 src/fileio.c                 | 37 +++++++++++++++++++++---------------
 test/lisp/net/tramp-tests.el | 11 ++++-------
 test/src/fileio-tests.el     | 11 +++++++++++
 5 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 92cbc2a1c9..090c54f8cd 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -2438,14 +2438,26 @@ File Name Expansion
 superroot above the root directory @file{/}.  On other filesystems,
 @file{/../} is interpreted exactly the same as @file{/}.
 
+If a filename must be that of a directory, its expansion must be too.
+For example, if a filename ends in @samp{/} or @samp{/.} or @samp{/..}
+then its expansion ends in @samp{/} so that it cannot be
+misinterpreted as the name of a symbolic link:
+
+@example
+@group
+(expand-file-name "/a///b//.")
+     @result{} "/a/b/"
+@end group
+@end example
+
 Expanding @file{.} or the empty string returns the default directory:
 
 @example
 @group
 (expand-file-name "." "/usr/spool/")
-     @result{} "/usr/spool"
+     @result{} "/usr/spool/"
 (expand-file-name "" "/usr/spool/")
-     @result{} "/usr/spool"
+     @result{} "/usr/spool/"
 @end group
 @end example
 
diff --git a/etc/NEWS b/etc/NEWS
index fae65a2227..14a75ca75d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1157,6 +1157,12 @@ region's (or buffer's) end.
 This function can be used by modes to add elements to the
 'choice' customization type of a variable.
 
++++
+** 'expand-file-name' no longer omits a trailing slash if the omission
+changes the filename's meaning.  E.g., (expand-file-name "/a/b/.") now
+returns "/a/b/" not "/a/b", which might be misinterpreted as the name
+of a symbolic link rather than of the directory it points to.
+
 +++
 ** New function 'file-modes-number-to-symbolic' to convert a numeric
 file mode specification into symbolic form.
diff --git a/src/fileio.c b/src/fileio.c
index 37072d9b6b..b70dff1c22 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -1065,7 +1065,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 #endif /* WINDOWSNT */
 #endif /* DOS_NT */
 
-  /* If nm is absolute, look for `/./' or `/../' or `//''sequences; if
+  /* If nm is absolute, look for "/./" or "/../" or "//" sequences; if
      none are found, we can probably return right away.  We will avoid
      allocating a new string if name is already fully expanded.  */
   if (
@@ -1398,7 +1398,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 
   if (newdir)
     {
-      if (nm[0] == 0 || IS_DIRECTORY_SEP (nm[0]))
+      if (IS_DIRECTORY_SEP (nm[0]))
 	{
 #ifdef DOS_NT
 	  /* If newdir is effectively "C:/", then the drive letter will have
@@ -1433,14 +1433,16 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 	  {
 	    *o++ = *p++;
 	  }
-	else if (p[1] == '.'
-		 && (IS_DIRECTORY_SEP (p[2])
-		     || p[2] == 0))
+	else if (p[1] == '.' && IS_DIRECTORY_SEP (p[2]))
 	  {
-	    /* If "/." is the entire filename, keep the "/".  Otherwise,
-	       just delete the whole "/.".  */
-	    if (o == target && p[2] == '\0')
-	      *o++ = *p;
+	    /* Replace "/./" with "/".  */
+	    p += 2;
+	  }
+	else if (p[1] == '.' && !p[2])
+	  {
+	    /* At the end of the file name, replace "/." with "/".
+	       The trailing "/" is for symlinks.  */
+	    *o++ = *p;
 	    p += 2;
 	  }
 	else if (p[1] == '.' && p[2] == '.'
@@ -1459,18 +1461,23 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 #ifdef WINDOWSNT
 	    char *prev_o = o;
 #endif
-	    while (o != target && (--o, !IS_DIRECTORY_SEP (*o)))
-	      continue;
+	    while (o != target)
+	      {
+		o--;
+		if (IS_DIRECTORY_SEP (*o))
+		  {
+		    /* Keep "/" at the end of the name, for symlinks.  */
+		    o += p[3] == 0;
+
+		    break;
+		  }
+	      }
 #ifdef WINDOWSNT
 	    /* Don't go below server level in UNC filenames.  */
 	    if (o == target + 1 && IS_DIRECTORY_SEP (*o)
 		&& IS_DIRECTORY_SEP (*target))
 	      o = prev_o;
-	    else
 #endif
-	    /* Keep initial / only if this is the whole name.  */
-	    if (o == target && IS_ANY_SEP (*o) && p[3] == 0)
-	      ++o;
 	    p += 3;
 	  }
 	else if (IS_DIRECTORY_SEP (p[1])
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 7e9ae33f84..aa00c07f79 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2131,16 +2131,16 @@ tramp-test05-expand-file-name
       (expand-file-name "/method:host:/path/../file") "/method:host:/file"))
     (should
      (string-equal
-      (expand-file-name "/method:host:/path/.") "/method:host:/path"))
+      (expand-file-name "/method:host:/path/.") "/method:host:/path/"))
     (should
      (string-equal
       (expand-file-name "/method:host:/path/..") "/method:host:/"))
     (should
      (string-equal
-      (expand-file-name "." "/method:host:/path/") "/method:host:/path"))
+      (expand-file-name "." "/method:host:/path/") "/method:host:/path/"))
     (should
      (string-equal
-      (expand-file-name "" "/method:host:/path/") "/method:host:/path"))
+      (expand-file-name "" "/method:host:/path/") "/method:host:/path/"))
     ;; Quoting local part.
     (should
      (string-equal
@@ -2155,12 +2155,9 @@ tramp-test05-expand-file-name
       "/method:host:/:/~/path/file"))))
 
 ;; The following test is inspired by Bug#26911 and Bug#34834.  They
-;; are rather bugs in `expand-file-name', and it fails for all Emacs
-;; versions.  Test added for later, when they are fixed.
+;; were bugs in `expand-file-name'.
 (ert-deftest tramp-test05-expand-file-name-relative ()
   "Check `expand-file-name'."
-  ;; Mark as failed until bug has been fixed.
-  :expected-result :failed
   (skip-unless (tramp--test-enabled))
 
   ;; These are the methods the test doesn't fail.
diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el
index 96b03a0137..1516590795 100644
--- a/test/src/fileio-tests.el
+++ b/test/src/fileio-tests.el
@@ -108,6 +108,17 @@ fileio-tests--relative-HOME
       (should (equal (expand-file-name "~/bar") "x:/foo/bar")))
     (setenv "HOME" old-home)))
 
+(ert-deftest fileio-tests--expand-file-name-trailing-slash ()
+  (dolist (fooslashalias '("foo/" "foo//" "foo/." "foo//." "foo///././."
+                           "foo/a/.."))
+    (should (equal (expand-file-name fooslashalias "/") "/foo/"))
+    (should (equal (expand-file-name (concat "/" fooslashalias)) "/foo/")))
+  (should (equal (expand-file-name "." "/usr/spool/") "/usr/spool/"))
+  (should (equal (expand-file-name "" "/usr/spool/") "/usr/spool/"))
+  ;; Trailing "B/C/.." means B must be a directory.
+  (should (equal (expand-file-name "/a/b/c/..") "/a/b/"))
+  (should (equal (expand-file-name "/a/b/c/../") "/a/b/")))
+
 (ert-deftest fileio-tests--insert-file-interrupt ()
   (let ((text "-*- coding: binary -*-\n\xc3\xc3help")
         f)
-- 
2.17.1

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

[Michael Albinus]

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

Hi Paul,

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

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.

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

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

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 27 Aug 2020 20:31:59 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 26911@debbugs.gnu.org,
>         Eli Zaretskii <eliz@gnu.org>,
>         Yegor Timoshenko <yegortimoshenko@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.

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

[Stephen Berman]

On Thu, 27 Aug 2020 21:38:10 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Mattias Engdegård <mattiase@acm.org>
>> Date: Thu, 27 Aug 2020 20:31:59 +0200
>> Cc: Michael Albinus <michael.albinus@gmx.de>, 26911@debbugs.gnu.org,
>>         Eli Zaretskii <eliz@gnu.org>,
>>         Yegor Timoshenko <yegortimoshenko@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.

So do I.

Steve Berman

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

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

[-- Attachment #2: 0001-Fix-recently-introduced-expand-file-name-bug.patch --]
[-- Type: text/x-patch, Size: 4982 bytes --]

From 0bbc84630f12e848e19c39dce01f3d14559bf70b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Aug 2020 14:46:52 -0700
Subject: [PATCH] Fix recently-introduced expand-file-name bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The bug was that (expand-file-name "~") returned something
like "/home/eggert/" instead of "/home/eggert".
Problem	reported by Mattias Engdegård (Bug#26911#27).
* src/fileio.c (Fexpand_file_name): When concatenating NEWDIR to
NM, instead of stripping trailing slashes from NEWDIR (which can
turn non-symlinks into symlinks), strip leading slashes from NM.
This also simplifies the code by removing no-longer-needed DOS_NT
special-casing.  Also, remove an unnecessary ‘target[length] = 0;’
as that byte will be overwritten by the next memcpy anyway.
* test/src/fileio-tests.el (fileio-tests--HOME-trailing-slash):
New test.
---
 src/fileio.c             | 38 +++++++++++++-------------------------
 test/src/fileio-tests.el |  8 ++++++++
 2 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index b70dff1c22..47e5e46a00 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -827,9 +827,9 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
   ptrdiff_t tlen;
 #ifdef DOS_NT
   int drive = 0;
-  bool collapse_newdir = true;
   bool is_escaped = 0;
 #endif /* DOS_NT */
+  bool collapse_newdir = true;
   ptrdiff_t length, nbytes;
   Lisp_Object handler, result, handled_name;
   bool multibyte;
@@ -1183,9 +1183,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 	      newdir = SSDATA (hdir);
 	      newdirlim = newdir + SBYTES (hdir);
 	    }
-#ifdef DOS_NT
 	  collapse_newdir = false;
-#endif
 	}
       else			/* ~user/filename */
 	{
@@ -1205,9 +1203,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 
 	      while (*++nm && !IS_DIRECTORY_SEP (*nm))
 		continue;
-#ifdef DOS_NT
 	      collapse_newdir = false;
-#endif
 	    }
 
 	  /* If we don't find a user of that name, leave the name
@@ -1374,12 +1370,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
     }
 #endif /* DOS_NT */
 
-  /* Ignore any slash at the end of newdir, unless newdir is
-     just "/" or "//".  */
   length = newdirlim - newdir;
-  while (length > 1 && IS_DIRECTORY_SEP (newdir[length - 1])
-	 && ! (length == 2 && IS_DIRECTORY_SEP (newdir[0])))
-    length--;
 
   /* Now concatenate the directory and name to new space in the stack frame.  */
   tlen = length + file_name_as_directory_slop + (nmlim - nm) + 1;
@@ -1398,25 +1389,22 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 
   if (newdir)
     {
-      if (IS_DIRECTORY_SEP (nm[0]))
+      if (!collapse_newdir)
 	{
-#ifdef DOS_NT
-	  /* If newdir is effectively "C:/", then the drive letter will have
-	     been stripped and newdir will be "/".  Concatenating with an
-	     absolute directory in nm produces "//", which will then be
-	     incorrectly treated as a network share.  Ignore newdir in
-	     this case (keeping the drive letter).  */
-	  if (!(drive && nm[0] && IS_DIRECTORY_SEP (newdir[0])
-		&& newdir[1] == '\0'))
-#endif
-	    {
-	      memcpy (target, newdir, length);
-	      target[length] = 0;
-	      nbytes = length;
-	    }
+	  /* With ~ or ~user, leave NEWDIR as-is to avoid transforming
+	     it from a symlink (or a regular file!) into a directory.  */
+	  memcpy (target, newdir, length);
+	  nbytes = length;
 	}
       else
 	nbytes = file_name_as_directory (target, newdir, length, multibyte);
+
+      /* If TARGET ends in a directory separator, omit leading
+	 directory separators from NM so that concatenating a TARGET "/"
+	 to an NM "/foo" does not result in the incorrect "//foo".  */
+      if (nbytes && IS_DIRECTORY_SEP (target[nbytes - 1]))
+	while (IS_DIRECTORY_SEP (nm[0]))
+	  nm++;
     }
 
   memcpy (target + nbytes, nm, nmlim - nm + 1);
diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el
index 1516590795..8b76912f5e 100644
--- a/test/src/fileio-tests.el
+++ b/test/src/fileio-tests.el
@@ -108,6 +108,14 @@ fileio-tests--relative-HOME
       (should (equal (expand-file-name "~/bar") "x:/foo/bar")))
     (setenv "HOME" old-home)))
 
+(ert-deftest fileio-tests--HOME-trailing-slash ()
+  "Test that expand-file-name of \"~\" respects trailing slash."
+  (let ((old-home (getenv "HOME")))
+    (dolist (home '("/a/b/c" "/a/b/c/"))
+      (setenv "HOME" home)
+      (should (equal (expand-file-name "~") (expand-file-name home))))
+    (setenv "HOME" old-home)))
+
 (ert-deftest fileio-tests--expand-file-name-trailing-slash ()
   (dolist (fooslashalias '("foo/" "foo//" "foo/." "foo//." "foo///././."
                            "foo/a/.."))
-- 
2.17.1

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

[Eli Zaretskii]

> Cc: Michael Albinus <michael.albinus@gmx.de>, 26911@debbugs.gnu.org,
>  Eli Zaretskii <eliz@gnu.org>, Yegor Timoshenko <yegortimoshenko@gmail.com>
> From: Paul Eggert <eggert@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)))

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

[Eli Zaretskii]

> Date: Fri, 28 Aug 2020 09:39:38 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
>  yegortimoshenko@gmail.com
> 
> > Cc: Michael Albinus <michael.albinus@gmx.de>, 26911@debbugs.gnu.org,
> >  Eli Zaretskii <eliz@gnu.org>, Yegor Timoshenko <yegortimoshenko@gmail.com>
> > From: Paul Eggert <eggert@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)

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

[Eli Zaretskii]

> Date: Fri, 28 Aug 2020 10:01:43 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
>  yegortimoshenko@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.

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

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.

[-- Attachment #2: 0001-Revert-recent-expand-file-name-changes-if-DOS_NT.patch --]
[-- Type: text/x-patch, Size: 4573 bytes --]

From 7d5807277ff614a337c7e4530bb8d0e0188c189b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 28 Aug 2020 22:37:29 -0700
Subject: [PATCH] Revert recent expand-file-name changes if DOS_NT

* src/fileio.c (Fexpand_file_name): Restore pre-August-26
behavior, if DOS_NT.  This should fix the recently-introduced
expand-file-name bugs on DOS_NT (Bug#26911).
---
 src/fileio.c | 66 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 9 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index 66010b6878..c91af36fdf 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -1372,6 +1372,14 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 
   length = newdirlim - newdir;
 
+#ifdef DOS_NT
+  /* Ignore any slash at the end of newdir, unless newdir is
+     just "/" or "//".  */
+  while (length > 1 && IS_DIRECTORY_SEP (newdir[length - 1])
+	 && ! (length == 2 && IS_DIRECTORY_SEP (newdir[0])))
+    length--;
+#endif
+
   /* Now concatenate the directory and name to new space in the stack frame.  */
   tlen = length + file_name_as_directory_slop + (nmlim - nm) + 1;
   eassert (tlen >= file_name_as_directory_slop + 1);
@@ -1388,22 +1396,40 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 
   if (newdir)
     {
-      if (!collapse_newdir)
+#ifndef DOS_NT
+      bool treat_as_absolute = !collapse_newdir;
+#else
+      bool treat_as_absolute = !nm[0] || IS_DIRECTORY_SEP (nm[0]);
+#endif
+      if (treat_as_absolute)
 	{
-	  /* With ~ or ~user, leave NEWDIR as-is to avoid transforming
-	     it from a symlink (or a regular file!) into a directory.  */
-	  memcpy (target, newdir, length);
-	  nbytes = length;
+#ifdef DOS_NT
+	  /* If newdir is effectively "C:/", then the drive letter will have
+	     been stripped and newdir will be "/".  Concatenating with an
+	     absolute directory in nm produces "//", which will then be
+	     incorrectly treated as a network share.  Ignore newdir in
+	     this case (keeping the drive letter).  */
+	  if (!(drive && nm[0] && IS_DIRECTORY_SEP (newdir[0])
+		&& newdir[1] == '\0'))
+#endif
+	    {
+	      /* With ~ or ~user, leave NEWDIR as-is to avoid transforming
+		 it from a symlink (or a regular file!) into a directory.  */
+	      memcpy (target, newdir, length);
+	      nbytes = length;
+	    }
 	}
       else
 	nbytes = file_name_as_directory (target, newdir, length, multibyte);
 
+#ifndef DOS_NT
       /* If TARGET ends in a directory separator, omit leading
 	 directory separators from NM so that concatenating a TARGET "/"
 	 to an NM "/foo" does not result in the incorrect "//foo".  */
       if (nbytes && IS_DIRECTORY_SEP (target[nbytes - 1]))
 	while (IS_DIRECTORY_SEP (nm[0]))
 	  nm++;
+#endif
     }
 
   memcpy (target + nbytes, nm, nmlim - nm + 1);
@@ -1420,6 +1446,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 	  {
 	    *o++ = *p++;
 	  }
+#ifndef DOS_NT
 	else if (p[1] == '.' && IS_DIRECTORY_SEP (p[2]))
 	  {
 	    /* Replace "/./" with "/".  */
@@ -1432,6 +1459,18 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 	    *o++ = *p;
 	    p += 2;
 	  }
+#else
+	else if (p[1] == '.'
+		 && (IS_DIRECTORY_SEP (p[2])
+		     || p[2] == 0))
+	  {
+	    /* If "/." is the entire filename, keep the "/".  Otherwise,
+	       just delete the whole "/.".  */
+	    if (o == target && p[2] == '\0')
+	      *o++ = *p;
+	    p += 2;
+	  }
+#endif
 	else if (p[1] == '.' && p[2] == '.'
 		 /* `/../' is the "superroot" on certain file systems.
 		    Turned off on DOS_NT systems because they have no
@@ -1445,9 +1484,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 #endif
 		 && (IS_DIRECTORY_SEP (p[3]) || p[3] == 0))
 	  {
-#ifdef WINDOWSNT
-	    char *prev_o = o;
-#endif
+#ifndef DOS_NT
 	    while (o != target)
 	      {
 		o--;
@@ -1459,11 +1496,22 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
 		    break;
 		  }
 	      }
-#ifdef WINDOWSNT
+#else
+# ifdef WINDOWSNT
+	    char *prev_o = o;
+# endif
+	    while (o != target && (--o, !IS_DIRECTORY_SEP (*o)))
+	      continue;
+# ifdef WINDOWSNT
 	    /* Don't go below server level in UNC filenames.  */
 	    if (o == target + 1 && IS_DIRECTORY_SEP (*o)
 		&& IS_DIRECTORY_SEP (*target))
 	      o = prev_o;
+	    else
+# endif
+	    /* Keep initial / only if this is the whole name.  */
+	    if (o == target && IS_ANY_SEP (*o) && p[3] == 0)
+	      ++o;
 #endif
 	    p += 3;
 	  }
-- 
2.17.1

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

[Eli Zaretskii]

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

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

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

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

[Michael Albinus]

Paul Eggert <eggert@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.

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

[Eli Zaretskii]

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

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   yegortimoshenko@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?

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: eggert@cs.ucla.edu,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   yegortimoshenko@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

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

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.

[-- Attachment #2: 0001-Mark-failing-fileio-test-on-MS-Windows.patch --]
[-- Type: text/x-patch, Size: 985 bytes --]

From e709c187fde76573ea3ec3a5f14e09b9ed59525f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 29 Aug 2020 13:34:06 -0700
Subject: [PATCH] Mark failing fileio test on MS-Windows

* test/src/fileio-tests.el (fileio-tests--HOME-trailing-slash):
Expect failure on MS-Windows.
---
 test/src/fileio-tests.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el
index 7baa4c7e2f..bedda83bbd 100644
--- a/test/src/fileio-tests.el
+++ b/test/src/fileio-tests.el
@@ -109,6 +109,9 @@ fileio-tests--relative-HOME
 
 (ert-deftest fileio-tests--HOME-trailing-slash ()
   "Test that expand-file-name of \"~\" respects trailing slash."
+  :expected-result (if (memq system-type '(windows-nt ms-dos))
+                       :failed
+                     :passed)
   (let ((process-environment (copy-sequence process-environment)))
     (dolist (home
              (if (memq system-type '(windows-nt ms-dos))
-- 
2.17.1

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

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

[Eli Zaretskii]

> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
>  yegortimoshenko@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 29 Aug 2020 13:42:23 -0700
> 
> 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.

It isn't right for us to make significant changes in expand-file-name,
let alone refactor it.  Its code is complex and full of subtle dark
corners, many of which are not well covered by our test suite.
Refactoring it into something more elegant is a large job -- for a
very small gain, since the function does its job 99.99% of the time.
We would be wasting our sparse resources if we do any serious changes
there, and will almost certainly introduce quite a few bugs on the
way.

It isn't worth it, definitely not in order to fix the couple of rare
bugs we are facing.

Instead, we should fix these particular bugs by localized changes
whose effect is limited to the specific situations we need to fix.

So with that in mind, let's please go back to the problems we have and
see how they can be fixed without unnecessary refactoring.

The original problem in this bug report was identified by Michael:

> In fileio.c, lines 1393-1394, the following loop
> 
> --8<---------------cut here---------------start------------->8---
> 	    while (o != target && (--o, !IS_DIRECTORY_SEP (*o)))
> 	      continue;
> --8<---------------cut here---------------end--------------->8---
> 
> replaces "/ssh:host:/bin/.." by "/ssh:host:". But it should be
> "/ssh:host:/".

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

In your log message you alluded to another use case, unrelated to
remote file names, but didn't provide any details.  Can you please
provide them now?  Is that other use case really similar to the one
which started this bug report (if not, no need to fix them both
together)?

The related bug#34834 is again about remote file names, but it's a
different situation.  AFAICT, it should also be easy to fix in a
localized manner.

Making such localized changes will allow us to be certain we don't
break any use cases we already support successfully.

Finally, I see no reason to require expand-file-name to preserve the
trailing slash -- we never required this until now, and AFAIK had no
problems with that.  If some specialized use case does need this, I'd
prefer to fix that only where and when it really matters.  For
example, if some Eshell command needs it, let's first consider fixing
that in Eshell.

In any case, the trailing slash issue is only tangentially related to
the bugs discussed here, so let's not mix these separate issues.

Thanks.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: eggert@cs.ucla.edu,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   yegortimoshenko@gmail.com
> Date: Sun, 30 Aug 2020 11:46:53 +0200
> 
> >   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?

IMO, special-casing a test is only a good idea if that is done for
reasons unrelated to the behavior being tested.  It makes no sense to
me to special-case a test in order to make failure look like success.
It's a slippery slope: once we go that way, before long we will have
to special-case real code, where the problematic behavior matters.

There's no real reason to special-case Windows in this case, because
its behavior doesn't really differ that much from Posix.  There's a
small difference in syntax, but the concepts are all the same.

We should fix this, not paper over it.

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

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

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

[Eli Zaretskii]

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

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

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

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

[Eli Zaretskii]

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

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

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

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

[Eli Zaretskii]

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

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

[Eli Zaretskii]

> Date: Mon, 31 Aug 2020 17:58:17 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, michael.albinus@gmx.de,
>  yegortimoshenko@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.

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

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

[Michael Albinus]

Eli Zaretskii <eliz@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.

My ansible script has finished. 75 runs of tramp-tests.el with different
configurations, no error in tramp-test05-expand-file-name or
tramp-test05-expand-file-name-relative.

However, I haven't run it on MS Windows.

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

Yes.

> Thanks.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: eggert@cs.ucla.edu,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   yegortimoshenko@gmail.com
> Date: Fri, 04 Sep 2020 13:55:24 +0200
> 
> > 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.
> 
> My ansible script has finished. 75 runs of tramp-tests.el with different
> configurations, no error in tramp-test05-expand-file-name or
> tramp-test05-expand-file-name-relative.

Thanks, that's good news.

> However, I haven't run it on MS Windows.

I did, but most of the tests are skipped.  Can you teach me how to
enable them on MS-Windows?  I can use PuTTY to connect to a remote
GNU/Linux machine where I have an account, if that is necessary.

Thanks.

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

Will do.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> However, I haven't run it on MS Windows.
>
> I did, but most of the tests are skipped.  Can you teach me how to
> enable them on MS-Windows?  I can use PuTTY to connect to a remote
> GNU/Linux machine where I have an account, if that is necessary.

Well, first check whether you can connect the remote host in
question. Try it interactively:

C-x C-f /plink:user@host:/tmp

("user" and "host" at your taste). The connection shall not require a
password. Alternatively, if you have created a PuTTy session with name
"session", you could use instead "/plinkx:session:/tmp".

Then, according to my MS Windows blurb, run in a CMD window:

--8<---------------cut here---------------start------------->8---
> set REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp
> %HOMEPATH%\Desktop\emacs-28.0.50\bin\emacs.exe -Q -batch -L X:\src\tramp\lisp -l X:\src\tramp\test\tramp-tests.el -f ert-run-tests-batch-and-exit
--8<---------------cut here---------------end--------------->8---

I have absolutely no build environment, just a downloaded Emacs tarfile
including binaries. Therefore, Tramp itself is mounted as share on the
X: drive.

In the test results, you will first see a line with the used remote
directory. tramp-test00-availability tells you whether the connection
can be established. It shall look like

--8<---------------cut here---------------start------------->8---
Remote directory: `/plink:user@host:/tmp'
   passed   1/70  tramp-test00-availability (0.053776 sec)
--8<---------------cut here---------------end--------------->8---

If this doesn't happen, or the first test returns with PASSED (capital
letters), something fails. Pls show me the output then.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: eggert@cs.ucla.edu,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   yegortimoshenko@gmail.com
> Date: Fri, 04 Sep 2020 15:53:09 +0200
> 
> > I did, but most of the tests are skipped.  Can you teach me how to
> > enable them on MS-Windows?  I can use PuTTY to connect to a remote
> > GNU/Linux machine where I have an account, if that is necessary.
> 
> Well, first check whether you can connect the remote host in
> question. Try it interactively:
> 
> C-x C-f /plink:user@host:/tmp
> 
> ("user" and "host" at your taste). The connection shall not require a
> password. Alternatively, if you have created a PuTTy session with name
> "session", you could use instead "/plinkx:session:/tmp".
> 
> Then, according to my MS Windows blurb, run in a CMD window:
> 
> --8<---------------cut here---------------start------------->8---
> > set REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp
> > %HOMEPATH%\Desktop\emacs-28.0.50\bin\emacs.exe -Q -batch -L X:\src\tramp\lisp -l X:\src\tramp\test\tramp-tests.el -f ert-run-tests-batch-and-exit
> --8<---------------cut here---------------end--------------->8---

OK, thanks.  This worked.  The results (see below) are mixed, but the
problems seem mostly to have something to do with the remote system,
not with Emacs.

The first 27 tests all passed.  Then things went south, with all the
rest but the last 5 tests failing.  The failures are pretty similar,
and can be divided into the following groups:

  . (file-error "Method `plink' should specify both encoding and decoding command or an scp program")

  . (file-error "Couldn't find command to check if file exists")

  .  (file-error "Tramp failed to connect.  If this happens repeatedly, try `M-x tramp-cleanup-this-connection'")

The last error always followed the penultimate one.  Usually, 3
"failed to connect" errors after each couldn't find command".

Several tests failed with other error messages:

  . Test tramp-test17-insert-directory backtrace:
     signal(wrong-type-argument ("listp sh:"))

  . Test tramp-test32-shell-command-dont-erase-buffer condition:
       (ert-test-failed
        ((should
	  (string-equal "barbaz
   "
		        (buffer-string)))
	 :form
	 (string-equal "barbaz
   " "bar")
	 :value nil))

  . Test tramp-test33-environment-variables condition:
       (ert-test-failed
        ((should
	  (string-equal
	   (format "%s,tramp:%s
   " emacs-version tramp-version)
	   (funcall this-shell-command-to-string "echo ${INSIDE_EMACS:-bla}")))
	 :form
	 (string-equal "28.0.50,tramp:2.5.0-pre
   " "")
	 :value nil))

Finally, the summary:

  Ran 71 tests, 32 results as expected, 28 unexpected, 11 skipped (2020-09-04 17:27:32+0300, 1357.765625 sec)

  28 unexpected results:
     FAILED  tramp-test17-insert-directory
     FAILED  tramp-test18-file-attributes
     FAILED  tramp-test19-directory-files-and-attributes
     FAILED  tramp-test20-file-modes
     FAILED  tramp-test21-file-links
     FAILED  tramp-test22-file-times
     FAILED  tramp-test23-visited-file-modtime
     FAILED  tramp-test26-file-name-completion
     FAILED  tramp-test27-load
     FAILED  tramp-test28-process-file
     FAILED  tramp-test29-start-file-process
     FAILED  tramp-test29-start-file-process-direct-async
     FAILED  tramp-test30-make-process
     FAILED  tramp-test30-make-process-direct-async
     FAILED  tramp-test31-interrupt-process
     FAILED  tramp-test32-shell-command
     FAILED  tramp-test32-shell-command-dont-erase-buffer
     FAILED  tramp-test33-environment-variables
     FAILED  tramp-test34-connection-local-variables
     FAILED  tramp-test34-explicit-shell-file-name
     FAILED  tramp-test35-exec-path
     FAILED  tramp-test35-remote-path
     FAILED  tramp-test36-vc-registered
     FAILED  tramp-test37-make-auto-save-file-name
     FAILED  tramp-test38-find-backup-file-name
     FAILED  tramp-test39-make-nearby-temp-file
     FAILED  tramp-test40-special-characters
     FAILED  tramp-test43-asynchronous-requests

  11 skipped results:
    SKIPPED  tramp-test24-file-acl
    SKIPPED  tramp-test25-file-selinux
    SKIPPED  tramp-test33-environment-variables-and-port-numbers
    SKIPPED  tramp-test40-special-characters-with-ls
    SKIPPED  tramp-test40-special-characters-with-perl
    SKIPPED  tramp-test40-special-characters-with-stat
    SKIPPED  tramp-test41-utf8
    SKIPPED  tramp-test41-utf8-with-ls
    SKIPPED  tramp-test41-utf8-with-perl
    SKIPPED  tramp-test41-utf8-with-stat
    SKIPPED  tramp-test42-file-system-info

What can I do about the failed tests?

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

[Eli Zaretskii]

On September 4, 2020 5:40:36 PM GMT+03:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: eggert@cs.ucla.edu,  26911@debbugs.gnu.org,  mattiase@acm.org,
>>   yegortimoshenko@gmail.com
>> Date: Fri, 04 Sep 2020 15:53:09 +0200
>> 
>> > I did, but most of the tests are skipped.  Can you teach me how to
>> > enable them on MS-Windows?  I can use PuTTY to connect to a remote
>> > GNU/Linux machine where I have an account, if that is necessary.
>> 
>> Well, first check whether you can connect the remote host in
>> question. Try it interactively:
>> 
>> C-x C-f /plink:user@host:/tmp
>> 
>> ("user" and "host" at your taste). The connection shall not require a
>> password. Alternatively, if you have created a PuTTy session with
>name
>> "session", you could use instead "/plinkx:session:/tmp".
>> 
>> Then, according to my MS Windows blurb, run in a CMD window:
>> 
>> --8<---------------cut here---------------start------------->8---
>> > set REMOTE_TEMPORARY_FILE_DIRECTORY=/plink:user@host:/tmp
>> > %HOMEPATH%\Desktop\emacs-28.0.50\bin\emacs.exe -Q -batch -L
>X:\src\tramp\lisp -l X:\src\tramp\test\tramp-tests.el -f
>ert-run-tests-batch-and-exit
>> --8<---------------cut here---------------end--------------->8---
>
>OK, thanks.  This worked.  The results (see below) are mixed, but the
>problems seem mostly to have something to do with the remote system,
>not with Emacs.
>
>The first 27 tests all passed.  Then things went south, with all the
>rest but the last 5 tests failing.  The failures are pretty similar,
>and can be divided into the following groups:
>
>. (file-error "Method `plink' should specify both encoding and decoding
>command or an scp program")
>
>  . (file-error "Couldn't find command to check if file exists")
>
>.  (file-error "Tramp failed to connect.  If this happens repeatedly,
>try `M-x tramp-cleanup-this-connection'")
>
>The last error always followed the penultimate one.  Usually, 3
>"failed to connect" errors after each couldn't find command".
>
>Several tests failed with other error messages:
>
>  . Test tramp-test17-insert-directory backtrace:
>     signal(wrong-type-argument ("listp sh:"))
>
>  . Test tramp-test32-shell-command-dont-erase-buffer condition:
>       (ert-test-failed
>        ((should
>	  (string-equal "barbaz
>   "
>		        (buffer-string)))
>	 :form
>	 (string-equal "barbaz
>   " "bar")
>	 :value nil))
>
>  . Test tramp-test33-environment-variables condition:
>       (ert-test-failed
>        ((should
>	  (string-equal
>	   (format "%s,tramp:%s
>   " emacs-version tramp-version)
>	   (funcall this-shell-command-to-string "echo
>${INSIDE_EMACS:-bla}")))
>	 :form
>	 (string-equal "28.0.50,tramp:2.5.0-pre
>   " "")
>	 :value nil))
>
>Finally, the summary:
>
>Ran 71 tests, 32 results as expected, 28 unexpected, 11 skipped
>(2020-09-04 17:27:32+0300, 1357.765625 sec)
>
>  28 unexpected results:
>     FAILED  tramp-test17-insert-directory
>     FAILED  tramp-test18-file-attributes
>     FAILED  tramp-test19-directory-files-and-attributes
>     FAILED  tramp-test20-file-modes
>     FAILED  tramp-test21-file-links
>     FAILED  tramp-test22-file-times
>     FAILED  tramp-test23-visited-file-modtime
>     FAILED  tramp-test26-file-name-completion
>     FAILED  tramp-test27-load
>     FAILED  tramp-test28-process-file
>     FAILED  tramp-test29-start-file-process
>     FAILED  tramp-test29-start-file-process-direct-async
>     FAILED  tramp-test30-make-process
>     FAILED  tramp-test30-make-process-direct-async
>     FAILED  tramp-test31-interrupt-process
>     FAILED  tramp-test32-shell-command
>     FAILED  tramp-test32-shell-command-dont-erase-buffer
>     FAILED  tramp-test33-environment-variables
>     FAILED  tramp-test34-connection-local-variables
>     FAILED  tramp-test34-explicit-shell-file-name
>     FAILED  tramp-test35-exec-path
>     FAILED  tramp-test35-remote-path
>     FAILED  tramp-test36-vc-registered
>     FAILED  tramp-test37-make-auto-save-file-name
>     FAILED  tramp-test38-find-backup-file-name
>     FAILED  tramp-test39-make-nearby-temp-file
>     FAILED  tramp-test40-special-characters
>     FAILED  tramp-test43-asynchronous-requests
>
>  11 skipped results:
>    SKIPPED  tramp-test24-file-acl
>    SKIPPED  tramp-test25-file-selinux
>    SKIPPED  tramp-test33-environment-variables-and-port-numbers
>    SKIPPED  tramp-test40-special-characters-with-ls
>    SKIPPED  tramp-test40-special-characters-with-perl
>    SKIPPED  tramp-test40-special-characters-with-stat
>    SKIPPED  tramp-test41-utf8
>    SKIPPED  tramp-test41-utf8-with-ls
>    SKIPPED  tramp-test41-utf8-with-perl
>    SKIPPED  tramp-test41-utf8-with-stat
>    SKIPPED  tramp-test42-file-system-info
>
>What can I do about the failed tests?

Hold on, these problems might be due to some quota I've exceeded on the remote system.  I'm unable to log in there, am trying to talk to the admins about that...

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>>     FAILED  tramp-test43-asynchronous-requests
>
> Hold on, these problems might be due to some quota I've exceeded on the remote system.  I'm unable to log in there, am trying to talk to the admins about that...

Will do. Just FTR, that test is tagged with :unstable, and it shouldn't
run yet outside my laptop. Since we don't use the Make machinery with
selectors, it isn't skipped on MS Windows.

Best regards, Michael.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> OK, thanks.  This worked.  The results (see below) are mixed, but the
> problems seem mostly to have something to do with the remote system,
> not with Emacs.

That's often the case; otherwise I would trap the failed tests locally.

> What can I do about the failed tests?

Well, there is the macro tramp--test-instrument-test-case. Usually, any
test which needs more analysis shall be wrapped by this macro, plus the
verbosity. Like

(tramp--test-instrument-test-case 6
... test code ...
)

The test code is usually the whole code of a ert-deftest body, except
the initial skip-unless forms. It will generate debug messages, which
could be analyzed. Unfortunately, it is often only me who can read the
result.

The verbosity is the value set for tramp-verbose. Often, it is
sufficient to set it to 6 (debug sent commands, and the responses). In
problematic cases it can be up to 10; the debug messages will be huge then.

An undocumented feature is a verbosity greater 10 (say 11). This
triggers also traces for all function calls of tramp-* functions via
trace.el. Usually, I ask for this only when I'm desperated ...

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: bug-gnu-emacs@gnu.org,  26911@debbugs.gnu.org, mattiase@acm.org,
>  eggert@cs.ucla.edu, yegortimoshenko@gmail.com
> Date: Fri, 04 Sep 2020 17:59:15 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi Eli,
> 
> >>     FAILED  tramp-test43-asynchronous-requests
> >
> > Hold on, these problems might be due to some quota I've exceeded on the remote system.  I'm unable to log in there, am trying to talk to the admins about that...

I'm talking to sysadmins to see how to overcome this problem, but in
case I cannot, how do I run the Tramp suite one test at a time? can
you show me an example command for that?

Thanks.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> I'm talking to sysadmins to see how to overcome this problem, but in
> case I cannot, how do I run the Tramp suite one test at a time? can
> you show me an example command for that?

# make tramp-tests SELECTOR='tramp-test05-expand-file-name'

See the test/README file for a discussion of SELECTOR.

> Thanks.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: bug-gnu-emacs@gnu.org,  26911@debbugs.gnu.org,  mattiase@acm.org,
>   eggert@cs.ucla.edu,  yegortimoshenko@gmail.com
> Date: Sat, 05 Sep 2020 10:34:39 +0200
> 
> > I'm talking to sysadmins to see how to overcome this problem, but in
> > case I cannot, how do I run the Tramp suite one test at a time? can
> > you show me an example command for that?
> 
> # make tramp-tests SELECTOR='tramp-test05-expand-file-name'

Thanks.  FTR, this translates into the following invocation from the
shell prompt:

  emacs -Q -batch -l test\lisp\net\tramp-tests.elc --eval "(ert-run-tests-batch-and-exit 'TEST-NAME)"

First, I think I know the reason for the problem I had yesterday to
run all the tests.  There's some problem in the Tramp tests that
causes almost each test that was run to leave 3 processes on the
remote system: 2 sshd's and 1 /bin/sh.  AFAICT, these are created by
the first connection made by each test.  Most tests create additional
connections, but their processes are all killed or exit when the test
completes, whereas this one connection is left behind.  And some test
leave behind more than one such triplet.  So after running enough
tests, the system is full of these triplets of zombie processes, and
on a resource-challenged system that could cause additional
connections to fail due to lack of resources to start another process.

Is it possible to make sure these processes are killed as part of each
test's cleanup?  For now, I ran the tests one by one, each time
killing the zombie processes manually on the remote system.  It took
some time...

Anyway, doing this cleanup manually allowed me to run all the tests
(skipping those which I knew to be "unstable"), and all but one of
them succeeded.  The one which failed is shown below together with the
failure description:

  Test tramp-test30-make-process condition:
      (ert-test-failed
       ((should
	 (string-match
	  (if ... "unknown signal
  \\'" "killed.*
  \\'")
	  (buffer-string)))
	:form
	(string-match "unknown signal
  \\'" "killed
  ")
	:value nil))
     FAILED  1/1  tramp-test30-make-process (39.250000 sec)

Just to be sure, I've ran this test twice, and each time it failed
with the same error.

I think this more or less concludes the testing of the fixes in these
two bugs, so I'm going to close them now.

Thanks.

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

[Eli Zaretskii]

> Date: Sat, 05 Sep 2020 14:18:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, eggert@cs.ucla.edu,
>  yegortimoshenko@gmail.com
> 
> I think this more or less concludes the testing of the fixes in these
> two bugs, so I'm going to close them now.

Done.

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

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> First, I think I know the reason for the problem I had yesterday to
> run all the tests.  There's some problem in the Tramp tests that
> causes almost each test that was run to leave 3 processes on the
> remote system: 2 sshd's and 1 /bin/sh.  AFAICT, these are created by
> the first connection made by each test.  Most tests create additional
> connections, but their processes are all killed or exit when the test
> completes, whereas this one connection is left behind.  And some test
> leave behind more than one such triplet.  So after running enough
> tests, the system is full of these triplets of zombie processes, and
> on a resource-challenged system that could cause additional
> connections to fail due to lack of resources to start another process.
>
> Is it possible to make sure these processes are killed as part of each
> test's cleanup?  For now, I ran the tests one by one, each time
> killing the zombie processes manually on the remote system.  It took
> some time...

Most of the tests start with (skip-unless (tramp--test-enabled)). This
defun calls tramp-cleanup-connection, which shall also kill all related
Tramp processes. Doesn't seem to work on MS Windows.

Hard to debug for me w/o such a machine. Could you write a bug report as
a reminder for me, that I investigate when I have such a machine?

> Anyway, doing this cleanup manually allowed me to run all the tests
> (skipping those which I knew to be "unstable"), and all but one of
> them succeeded.  The one which failed is shown below together with the
> failure description:
>
>   Test tramp-test30-make-process condition:
>       (ert-test-failed
>        ((should
> 	 (string-match
> 	  (if ... "unknown signal
>   \\'" "killed.*
>   \\'")
> 	  (buffer-string)))
> 	:form
> 	(string-match "unknown signal
>   \\'" "killed
>   ")
> 	:value nil))
>      FAILED  1/1  tramp-test30-make-process (39.250000 sec)
>
> Just to be sure, I've ran this test twice, and each time it failed
> with the same error.

Hahh! There is a special case in that test for MS-Windows:

--8<---------------cut here---------------start------------->8---
(should
 (string-match
  (if (eq system-type 'windows-nt)
      "unknown signal\n\\'" "killed.*\n\\'")
  (buffer-string))))
--8<---------------cut here---------------end--------------->8---

IIRC, somebody has reported this different return message. Or maybe I
have seen this on the MS Windows machine I've used for testing.

Maybe we shall simply allow both messages, because the exact wording
doesn't matter. What about the appended patch?

> Thanks.

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 828 bytes --]

*** /tmp/ediff7ZRJwF	2020-09-05 17:54:46.315839349 +0200
--- /home/albinus/src/tramp/test/tramp-tests.el	2020-09-05 17:53:23.687580606 +0200
***************
*** 4498,4508 ****
  	    ;; Read output.
  	    (with-timeout (10 (tramp--test-timeout-handler))
  	      (while (accept-process-output proc 0 nil t)))
! 	    (should
! 	     (string-match
! 	      (if (eq system-type 'windows-nt)
! 		  "unknown signal\n\\'" "killed.*\n\\'")
! 	      (buffer-string))))

  	;; Cleanup.
  	(ignore-errors (delete-process proc)))
--- 4498,4504 ----
  	    ;; Read output.
  	    (with-timeout (10 (tramp--test-timeout-handler))
  	      (while (accept-process-output proc 0 nil t)))
! 	    (should (string-match "unknown signal\\|killed" (buffer-string))))

  	;; Cleanup.
  	(ignore-errors (delete-process proc)))

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 26911@debbugs.gnu.org,  mattiase@acm.org,  eggert@cs.ucla.edu,
>   yegortimoshenko@gmail.com
> Date: Sat, 05 Sep 2020 17:57:56 +0200
> 
> > Is it possible to make sure these processes are killed as part of each
> > test's cleanup?  For now, I ran the tests one by one, each time
> > killing the zombie processes manually on the remote system.  It took
> > some time...
> 
> Most of the tests start with (skip-unless (tramp--test-enabled)). This
> defun calls tramp-cleanup-connection, which shall also kill all related
> Tramp processes. Doesn't seem to work on MS Windows.

AFAICS, tramp-cleanup-connection deletes the network connection
processes, but is that supposed to make sure the other side of the
connection exits cleanly?

> Hard to debug for me w/o such a machine. Could you write a bug report as
> a reminder for me, that I investigate when I have such a machine?

Will do.  And let me know if I can provide more details about this
matter, or help you debug this.

> >   Test tramp-test30-make-process condition:
> >       (ert-test-failed
> >        ((should
> > 	 (string-match
> > 	  (if ... "unknown signal
> >   \\'" "killed.*
> >   \\'")
> > 	  (buffer-string)))
> > 	:form
> > 	(string-match "unknown signal
> >   \\'" "killed
> >   ")
> > 	:value nil))
> >      FAILED  1/1  tramp-test30-make-process (39.250000 sec)
> >
> > Just to be sure, I've ran this test twice, and each time it failed
> > with the same error.
> 
> Hahh! There is a special case in that test for MS-Windows:
> 
> --8<---------------cut here---------------start------------->8---
> (should
>  (string-match
>   (if (eq system-type 'windows-nt)
>       "unknown signal\n\\'" "killed.*\n\\'")
>   (buffer-string))))
> --8<---------------cut here---------------end--------------->8---
> 
> IIRC, somebody has reported this different return message. Or maybe I
> have seen this on the MS Windows machine I've used for testing.
> 
> Maybe we shall simply allow both messages, because the exact wording
> doesn't matter. What about the appended patch?

It fixes the problem, thanks.

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

[Eli Zaretskii]

> Date: Sat, 05 Sep 2020 19:33:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 26911@debbugs.gnu.org, mattiase@acm.org, eggert@cs.ucla.edu,
>  yegortimoshenko@gmail.com
> 
> > Hard to debug for me w/o such a machine. Could you write a bug report as
> > a reminder for me, that I investigate when I have such a machine?
> 
> Will do.

Bug#43226.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Most of the tests start with (skip-unless (tramp--test-enabled)). This
>> defun calls tramp-cleanup-connection, which shall also kill all related
>> Tramp processes. Doesn't seem to work on MS Windows.
>
> AFAICS, tramp-cleanup-connection deletes the network connection
> processes, but is that supposed to make sure the other side of the
> connection exits cleanly?

It does at least when you're on GNU/Linux. What happens on MS Windows is
beyond my knowledge.

Perhaps I'll call process-send-eof prior killing the process?

>> Maybe we shall simply allow both messages, because the exact wording
>> doesn't matter. What about the appended patch?
>
> It fixes the problem, thanks.

Pushed to master.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 26911@debbugs.gnu.org,  mattiase@acm.org,  eggert@cs.ucla.edu,
>   yegortimoshenko@gmail.com
> Date: Sat, 05 Sep 2020 19:36:34 +0200
> 
> > AFAICS, tramp-cleanup-connection deletes the network connection
> > processes, but is that supposed to make sure the other side of the
> > connection exits cleanly?
> 
> It does at least when you're on GNU/Linux. What happens on MS Windows is
> beyond my knowledge.

Indeed, it's strange that the result should depend on the local
system, since the commands sent to the remote are the same in boths
cases, I'd imagine.

> Perhaps I'll call process-send-eof prior killing the process?

If you show me a patch, I can try it.  But let's continue discussing
this in the bug I filed, bug#43226.

Thanks.