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

[Felix Dietrich]

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

bisect.sh
Description: Bisection Script

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.

0001-tramp-archive-Always-call-tramp-autoload-file-name-h.patch
Description: Patch to always call ‘tramp-autoload-file-name-handler’

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