Re: Error with tramp-archive-autoload-file-name-handler

[Felix Dietrich <felix.dietrich@sperrhaken.name>]

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

Michael Albinus <michael.albinus@gmx.de> writes:

> Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
>
>>> Anyway, when `tramp-archive-autoload-file-name-handler' is present after
>>> tramp-archive-enabled has become nil, it is used and called but doesn't
>>> handle the request (i.e. returns nil).  AFAIU, that was what I have been
>>> seeing.
>>
>> Could the tramp error handler provide a more helpful error message

Oh, I meant “tramp file handler” here instead of “tramp error handler” –
although I had made wrong assumptions; therefore, this is irrelevant.

> Since this bug is fixed now, I don't believe it is worthful to invest
> more code there.

I am *not* convinced that the reason for this bug has been correctly
identified.  After spending some more time with it, I have come up with
the following test case with which I am now able to produce the error
reliably with revision a5841b196f [1] (that is your patch):

  #+begin_src emacs-lisp
	(let* ((emacs-repo "~/src/emacs/")
		   (foo.iso (concat (file-name-as-directory emacs-repo)
							"test/lisp/net/tramp-archive-resources/foo.iso/")))
	  (load "tramp-archive")
	  (let ((file-name-handler-alist
			 (list (cons (tramp-archive-autoload-file-name-regexp)
						 #'tramp-archive-autoload-file-name-handler)))
			(tramp-archive-enabled nil))
		(file-directory-p foo.iso))))
  #+end_src

With Emacs 27.1, however, I was not able to reproduce the issue.
Therefore, I used “git bisect” between the tag “emacs-27.1” and your
commit a5841b196f with the attached script to find the offending commit
introducing the issue: 4db69b32b8 (“Fix bug#48476”) [2].


[-- Attachment #2: Bisection Script --]
[-- Type: text/x-sh, Size: 2839 bytes --]

#!/bin/sh

# Usage: this_script [recompile [JOBS]]
#
# Without “recompile” this script only removes
# “lisp/net/tramp-archive.elc” to ensure that Emacs uses the
# “tramp-archive” version of the current revision; you have to have
# built Emacs yourself before running this script.
#
# With “recompile” the repository is cleaned, the build configured,
# and then Emacs build, before the test is run.  JOBS is bassed to the
# “-j” switch of “make”, that is: it “specifies the number of jobs to
# run simultaneously”.


# Emacs executable to use
emacs=src/emacs

skip_commit () { exit 125; }
# Can “git bisect” really be properly and reliably
# interupted this way?
interrupt_bisect () { exit 255; }
mark_good () { exit 0; }
mark_bad () { exit 1; }


if [ "$1" = "recompile" ]
then
    jobs=${2:+-j$2}
    {
      git clean -dxf \
      && ./autogen.sh \
      && ./configure --without-x --without-gnutls \
      && make $jobs src
    } || interrupt_bisect
else
    # Ensure that “tramp-archive” from the current revision is used.
    # The byte-compiled version is from the revision where the
    # bisection started and would be preferred by Emacs.
    rm -f lisp/net/tramp-archive.elc
fi

# Restore “build-aux/install-sh” to the version in the current HEAD.
# Otherwise, “git bisect” would abort after the script has finished
# (at least at some revisions) as the checkout of the next revision
# would fail because of changes to this file.
git checkout -- build-aux/install-sh



# \f
##################################################
# The actual Test.
##################################################

"$emacs" -Q --batch --eval \
"
(let ((ret 1))
  (unwind-protect
      (progn
        (load \"tramp-archive\")
        (let ((file-name-handler-alist
               (list (cons (tramp-archive-autoload-file-name-regexp)
                           #'tramp-archive-autoload-file-name-handler)))
              (tramp-archive-enabled nil))
          (file-directory-p
           \"~/emacs/test/lisp/net/tramp-archive-resources/foo.iso/\")
          ;; No error during ‘file-directory-p’
          (setq ret 0)))
    (kill-emacs ret)))
"

ret=$?
case $ret in
    0) mark_good
       ;;
    1) mark_bad
       ;;
    255) # For some revisions the ‘kill-emacs’ in the unwind-form does
         # not work as expected and Emacs instead exits with 255, or
         # maybe that is -1, (as it does on erros in batch mode).
         # Using ‘condition-case’ would proberly fix that (and make a
         # more robust test), but this is quicker and good enough.
        mark_bad
        ;;
    *)
        printf "Unexpected return code by Emacs: %i\n" $ret >&2
        interrupt_bisect
        ;;
esac

[-- Attachment #3: Type: text/plain, Size: 817 bytes --]



At a quick glance, this commit changed the implementation of
‘tramp-archive-autload-file-name-handler’ and might be missing an
else-form.  This causes the autoload handler to return nil when
‘tramp-archive-enabled’ is nil, and, at a slightly longer glance, for
some file name handlers this is wrong and an error.  The attached patch
changes ‘tramp-archive-autoload-file-name-handler’ to always call
‘tramp-autoload-file-name-handler’, setting ‘tramp-archive-autoload’
appropriately.  This may cause a regression, though, of bug #48476 [3]
(“Emacs hangs with 100% cpu if started within a current directory that
has a name ending with ".tar"”).  I have not tested that or spent time
on understanding that bug.  Maybe you still remember enough details of
the bug to judge this.


[-- Attachment #4: Patch to always call ‘tramp-autoload-file-name-handler’ --]
[-- Type: text/x-diff, Size: 2340 bytes --]

From 282fe5b2af1422236b213732d31a176fdea904ed Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Mon, 4 Apr 2022 18:45:17 +0200
Subject: [PATCH] =?UTF-8?q?tramp-archive:=20Always=20call=20=E2=80=98tramp?=
 =?UTF-8?q?-autoload-file-name-handler=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Otherwise, not calling ‘tramp-autoload-file-name-handler’ in
‘tramp-archive-autoload-file-name-handler’ when
‘tramp-archive-enabled’ is nil and
‘tramp-archive-autoload-file-name-handler’ is in the
‘file-name-handler-alist’ results in an error “Invalid handler in
‘file-name-handler-alist” once Emacs calls
‘tramp-archive-autoload-file-name-handler’ with a handler that does
not expect nil.  Always returning nil is also false in general.
---
 lisp/net/tramp-archive.el | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lisp/net/tramp-archive.el b/lisp/net/tramp-archive.el
index 890c8dbb75..e7ffe2d4f4 100644
--- a/lisp/net/tramp-archive.el
+++ b/lisp/net/tramp-archive.el
@@ -360,14 +360,13 @@ arguments to pass to the OPERATION."
 (progn (defun tramp-archive-autoload-file-name-handler (operation &rest args)
   "Load Tramp archive file name handler, and perform OPERATION."
   (defvar tramp-archive-autoload)
-  (when tramp-archive-enabled
-    ;; We cannot use `tramp-compat-temporary-file-directory' here due
-    ;; to autoload.  When installing Tramp's GNU ELPA package, there
-    ;; might be an older, incompatible version active.  We try to
-    ;; overload this.
-    (let ((default-directory temporary-file-directory)
-          (tramp-archive-autoload t))
-      (apply #'tramp-autoload-file-name-handler operation args)))))
+  (let (;; We cannot use `tramp-compat-temporary-file-directory' here due
+        ;; to autoload.  When installing Tramp's GNU ELPA package, there
+        ;; might be an older, incompatible version active.  We try to
+        ;; overload this.
+        (default-directory temporary-file-directory)
+        (tramp-archive-autoload tramp-archive-enabled))
+    (apply #'tramp-autoload-file-name-handler operation args))))
 
 (put #'tramp-archive-autoload-file-name-handler 'tramp-autoload t)
 
-- 
2.35.1


[-- Attachment #5: Type: text/plain, Size: 7197 bytes --]



At a much longer glance, in this particular case it is not actually
‘Ffile_directory_p’ (the C function in fileio.c) directly that has an
issue with nil, but, instead, the error is signalled in
‘Fexpand_file_name’, called indirectly by ‘Ffile_directory_p’:

    - Ffile_directory_p
      - expand_and_dir_to_file
        - Fexpand_file_name

‘expand_file_name’ expects a string as the return value from a magic
file name handler; nil, and everything else, is an error.

Now, why is it a problem to add
‘tramp-archive-autoload-file-name-handler’ to ‘file-name-handler-alist’
if ‘tramp-archive-file-name-handler’ is already there?  Why does the
following snipped still fail even though
‘tramp-archive-file-name-handler’ comes first in the handler alist?

   #+begin_src emacs-lisp
     (let* ((emacs-repo "~/src/emacs/")
            (foo.iso
             (concat (file-name-as-directory emacs-repo)
                     "test/lisp/net/tramp-archive-resources/foo.iso/")))
       (load "tramp-archive")
       (let ((tramp-archive-enabled nil)
             (file-name-handler-alist
              (list (cons tramp-archive-file-name-regexp
                          #'tramp-archive-file-name-handler)
                    (cons (tramp-archive-autoload-file-name-regexp)
                          #'tramp-archive-autoload-file-name-handler))))
         (file-directory-p foo.iso)))
   #+end_src

The answer is that the real file handler
‘tramp-archive-file-name-handler’ has its ‘operations’ property set,
which does not include ‘expand-file-name’; the autoload handler has not,
and so, when ‘file-directory-p’ is called, ‘find-file-name-handler’
picks for the ‘expand-file-name’ operation initiated by
‘file-directory-p’ the autoload handler, because the operations property
of ‘tramp-archive-file-name-handler’ indicates that
‘tramp-archive-file-name-handler’ is not applicable.  With
‘tramp-archive-enabled’ bound to nil, the autoload handler then returns
nil and, by that, causes ‘Fexpand_file_name’ to signal the error.  (As
my test above shows, having multiple entries is not significant, it is
the autoload handler that causes the error.)

The erroneous autoload handler:

  #+begin_src emacs-lisp
    (defun tramp-archive-autoload-file-name-handler (operation &rest args)
      "Load Tramp archive file name handler, and perform OPERATION."
      (defvar tramp-archive-autoload)
      (when tramp-archive-enabled
        ;; We cannot use `tramp-compat-temporary-file-directory' here due
        ;; to autoload.  When installing Tramp's GNU ELPA package, there
        ;; might be an older, incompatible version active.  We try to
        ;; overload this.
        (let ((default-directory temporary-file-directory)
              (tramp-archive-autoload t))
          (apply #'tramp-autoload-file-name-handler operation args))))
  #+end_src


I guess other Michael was on the right track:

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Anyway, when `tramp-archive-autoload-file-name-handler' is present after
> tramp-archive-enabled has become nil, it is used and called but doesn't
> handle the request (i.e. returns nil).  AFAIU, that was what I have been
> seeing.

Now, to explain this part and especially the “TWICE”:

> But evaluating this TWICE (the `add-to-list' form is from
> `tramp-register-archive-file-name-handler') does:
> 
>  #+begin_src emacs-lisp
> (add-to-list 'file-name-handler-alist
> 	         (cons (tramp-archive-autoload-file-name-regexp)
> 		       #'tramp-archive-autoload-file-name-handler))
> (file-directory-p
>  "/home/micha/software/emacs/test/lisp/net/tramp-archive-resources/foo.iso/")
>   |--> (error "Invalid handler in ‘file-name-handler-alist’")
> #+end_src
> 
> I think that adding `tramp-archive-autoload-file-name-handler'
> multiple times could be related.

For the first time:

When ‘tramp-archive-enabled’ is initially set to t:

  #+begin_src emacs-lisp
    ;;;###autoload
    (defvar tramp-archive-enabled (featurep 'dbusbind)
      "Non-nil when file archive support is available.")
  #+end_src

then ‘tramp-archive-autoload-file-name-handler’ will initialise tramp by
calling ‘tramp-autoload-file-name-handler’ which will first load
“tramp-archive”, because ‘tramp-archive-enabled’ is t; this can
potentially set ‘tramp-archive-enabled’ to nil:

  #+begin_src emacs-lisp
    ;; In tramp-archive.el,
    ;; right after the (defvar tramp-archive-enabled…
    (setq tramp-archive-enabled tramp-gvfs-enabled)
  #+end_src

  #+begin_src emacs-lisp
    (defconst tramp-gvfs-enabled
    (ignore-errors
      (and (featurep 'dbusbind)
       (autoload 'zeroconf-init "zeroconf")
       (tramp-compat-funcall 'dbus-get-unique-name :system)
       (tramp-compat-funcall 'dbus-get-unique-name :session)
       (or (tramp-process-running-p "gvfs-fuse-daemon")
           (tramp-process-running-p "gvfsd-fuse"))))
    "Non-nil when GVFS is available.")
  #+end_src

After loading “tramp-archive”, ‘tramp-autoload-file-name-handler’
continues with loading “tramp”; this will call, via the
‘tramp--startup-hook’ ‘tramp-register-file-name-handlers’.  This
function will, depending on the value of ‘tramp-archive-enabled’, also
add the ‘tramp-archive-file-name-handler’ to the
‘file-name-handler-alist’.

For the second time:

If ‘tramp-archive-enabled’ is nil now, and you add
‘tramp-archive-autoload-file-name-handler’ again to
‘file-name-handler-alist’, you arrive at the above described problem:
‘Fexpand_file_name’ called from ‘Ffile_directory_p’ will signal an error
because the autoload handler returns nil.

If ‘tramp-archive-enabled’ is t after the first time, the registration
process is simply done a second time; no error occurs.



Footnotes:
[1] commit a5841b196f12894df4c1bb073f28ddadb6faa3cf
    Author: Michael Albinus <michael.albinus@gmx.de>
    Date:   Mon Mar 28 12:02:23 2022 +0200
    Subject:  Do not register Tramp file name handlers twice

    * lisp/net/tramp.el (tramp-register-autoload-file-name-handlers):
    * lisp/net/tramp-archive.el (tramp-register-archive-file-name-handler):
    Check, whether the real file name handler is already registered.


[2] commit 4db69b32b835a833168982b0f11a43d7f62ba8b2
    Author: Michael Albinus <michael.albinus@gmx.de>
    Date:   Sat May 22 17:51:07 2021 +0200
    Subject: Fix bug#48476

    * lisp/net/tramp-archive.el (tramp-archive-autoload-file-name-handler):
    Add implementation.
    
    * lisp/net/tramp-integration.el (tramp-rename-files)
    (tramp-rename-these-files): Declare them.
    
    * lisp/net/tramp.el (tramp-autoload-file-name-handler):
    Load tramp-archive.el if needed.  (Bug#48476)
    
    * test/lisp/net/tramp-archive-tests.el (tramp-archive-test45-auto-load):
    Extend test.
    
    Use #' syntax for function symbols.


[3]  <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48476>

-- 
Felix Dietrich