bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky <npostavs@users.sourceforge.net>]

Philipp Stephani <p.stephani2@gmail.com> writes:

> If my git bisect is correct, it was
> commit a1bbc490155b61a634a6d0b165000ce35b93aa35 to fix Bug#29579. So
> by fixing one bug we introduced another one :(

Actually, looking at Bug#29579 again, it doesn't seem *that* bad, and as
far as I can tell, it has existed for a long time (still occurs back in
24.3).  So reverting that fix seems like a reasonable option too.  I can
confirm that doing so fixes this bug.

> Eli Zaretskii <eliz@gnu.org> schrieb am Di., 30. Jan. 2018 um
> 14:46 Uhr:
>    
>     I think to make up my mind I'd need a few words about each part of
>     the changes (with the exception of the test suite changes): why is
>     each of them needed, and what does it do to fix which part of the
>     original problem.

The file-name-non-special function handles all file-handler operations
for "/:" quoted files.  It has an alist inside, to decide which
arguments are filenames and so require the "/:" to be removed before
calling the real operation.  The alist was not complete, so for many
operations the "/:" would only be stripped from the first argument
(that's a fallback for operations not listed in the alist).

Up until the fix for Bug#29579 this didn't matter so much, because
file-name-non-special would bind file-name-handler-alist to nil, thus
preventing any further filenames from being passed to the handler
anyway.  So multi-filename-arg handlers were broken, but unobtrusively
so.  With the fix for Bug#29579, such handlers got stuck in infinite
recursion, as they kept getting called, not stripping the "/:" prefix,
thus getting called again, etc...

The proposed patch just fixes up this alist to finally list all the
arguments for all the handlers correctly:

@@ -7000,7 +7000,7 @@ file-name-non-special
            ;; Bug#25949.
 	   (if (memq operation
                      '(insert-directory process-file start-file-process
-                                        shell-command))
+                                        shell-command temporary-file-directory))
 	       (directory-file-name
 	        (expand-file-name
 		 (unhandled-file-name-directory default-directory)))
@@ -7024,15 +7024,23 @@ file-name-non-special
 			    ;; temporarily to unquoted filename.
 			    (verify-visited-file-modtime unquote-then-quote)
 			    ;; List the arguments which are filenames.
-			    (file-name-completion 1)
-			    (file-name-all-completions 1)
+			    (file-name-completion 0 1)
+			    (file-name-all-completions 0 1)
+                            (file-equal-p 0 1)
+                            (file-newer-than-file-p 0 1)
 			    (write-region 2 5)
 			    (rename-file 0 1)
 			    (copy-file 0 1)
 			    (copy-directory 0 1)
 			    (file-in-directory-p 0 1)
 			    (make-symbolic-link 0 1)
-			    (add-name-to-file 0 1))))
+			    (add-name-to-file 0 1)
+                            (make-auto-save-file-name buffer-file-name)
+                            (set-visited-file-modtime buffer-file-name)
+                            ;; These file-notify-* operations take a
+                            ;; descriptor.
+                            (file-notify-rm-watch . nil)
+                            (file-notify-valid-p . nil))))
 		   ;; For all other operations, treat the first argument only
 		   ;; as the file name.
 		   '(nil 0))))
@@ -7055,6 +7063,12 @@ file-name-non-special
     (pcase method
       (`identity (car arguments))
       (`add (file-name-quote (apply operation arguments)))
+      (`buffer-file-name
+       (let ((buffer-file-name
+              (if (string-match "\\`/:" buffer-file-name)
+                  (substring buffer-file-name (match-end 0))
+                buffer-file-name)))
+         (apply operation arguments)))
       (`insert-file-contents
        (let ((visit (nth 1 arguments)))
          (unwind-protect

>     I also wonder how come we've succeeded to break quoted file names
>     so fundamentally -- what change did that, and why did we make it
>     on the release branch?

IMO, the root cause is pretty clearly lack of adequate tests for this.
There are more than 60 file-handler operations; it's crazy to expect to
be able to make a correct change without an automated test that at least
exercises each one.