bug#57407: [PATCH] Handle error of ’vc-registered’

[Dmitry Gutov <dgutov@yandex.ru>]

Hi!

On 25.08.2022 19:20, Simon Tournier wrote:
> Hi,
> 
> Submission (Bug#18481) [0] merged on 2020-08-13 with commit
> 991e145450ec8b02865597bc80fd797e39e81f07 [1] aims to:
> 
> “Notify the user if we errors when querying for registered git files“
> 
> However, the replacement of ’ignore-errors’ by ’with-demoted-errors’
> introduces spurious messages.  This patch proposes to handle the errors
> in a way that:
> 
>   1. the user is still informed (avoid silent error)
>   2. improve the messages trying to be more accurate
>   3. do it for all the VC backends
> 
> 0: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=18481
> 1: https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=991e145450ec8b02865597bc80fd797e39e81f07
> 
> 
> 
> First, let compare the previous situation with the patched one.  If the
> user runs ’find-file’ in a Git repository without having installed the
> Git binary, then Emacs complains and the error is misleading.
> Reproducer:
> 
> --8<---------------cut here---------------start------------->8---
> $  which git
> which: no git in …
> $ mkdir -p /tmp/Git/.git
> $ emacs -q --batch --eval="(find-file \"/tmp/Git/foo\")"
> Error: (file-missing "Searching for program" "No such file or directory" "git")
> Package vc-mtn is deprecated
> --8<---------------cut here---------------end--------------->8---
> 
> Not having a working Git installation is not an error for opening one
> file belonging to a folder containing a ’.git’ subdirectory.  For
> instance, if an user processes many files reporting many messages, then
> it seems hard to locate the real error, if any.

Do I take this right that the main purpose of the patch is to have the 
"Error: ..." messages replaced with "Warning: ..."?

> Moreover, the messages are inconsistent depending on the VC backend;
> from nothing reported to a backtrace.
> 
> --8<---------------cut here---------------start------------->8---
> $ mkdir -p /tmp/Bzr/.bzr
> $ emacs -q --batch --eval="(find-file \"/tmp/Bzr/foo\")"
> Error: (file-missing "Searching for program" "No such file or directory" "bzr")
> Error: (file-missing "Searching for program" "No such file or directory" "bzr")
> 
> Error: file-missing ("Searching for program" "No such file or directory" "bzr")
> 
> [...]
> 
> Searching for program: No such file or directory, bzr
> --8<---------------cut here---------------end--------------->8---
> 
> Considering the patch, it would become:
> 
> --8<---------------cut here---------------start------------->8---
> $ emacs -q --batch --eval="(find-file \"/tmp/Git/foo\")"
> Warning: (vc-not-supported "Searching for program" "No such file or directory" "git")
> 
> $ emacs -q --batch --eval="(find-file \"/tmp/Bzr/foo\")"
> Falling back on "slow" status detection ((error . "VC: Bzr dirstate is not flat format 3"))
> Warning: (vc-not-supported "Searching for program" "No such file or directory" "bzr")
> --8<---------------cut here---------------end--------------->8---
> 
> and all the VC backends report similarly when something fails.

Some better consistency would be nice indeed.

> Second, I have tested various configurations using Guix (65cabb0) and
> also the Emacs test suite is passing.  However, note that a) I barely
> use VC so b) I am lacking imagination for testing scenarii where the
> bubble error could wrongly propagate and thus would provide an
> unexpected behavior.  Especially with remote as Tramp allows.
> 
> 
> Third, I do not know if it is the correct way for catching the errors.
> The core of the change is:
> 
> --8<---------------cut here---------------start------------->8---
> lisp/vc/vc-dispatcher.el (vc-do-command):
> 
>                (condition-case err
> 	          (setq status (apply #'process-file command nil t nil squeezed))
>                  (error
>                   (pcase (car err)
>                     ('file-missing
>                      (if (string= (cadr err) "Searching for program")
>                          ;; The most probable is the lack of the backend binary.
>                          (signal 'vc-not-supported (cdr err))
>                        (signal (car err) (cdr err))))
>                     (_
>                      (signal (car err) (cdr err))))))
> 
> lisp/vc/vc-hooks.el (vc-refresh-state):
> 
>                        (condition-case err
>                            (vc-backend buffer-file-name)
>                          (error
>                           (pcase (car err)
>                             ('vc-not-supported
>                              (message "Warning: %S" err))
>                             (_
>                              (message "VC refresh error: %S" err)))
>                           nil))
> --8<---------------cut here---------------end--------------->8---
> 
> and the rest of the change is just bubble error propagation from this
> ’vc-do-command’ to this ’vc-refresh-state’.

This general idea seems reasonable.

But you also add a bunch of re-signaling code like

+  (let ((state (condition-case err
+                   (vc-bzr-state-heuristic file)
+                 (error (signal (car err) (cdr err))))))

What is the idea behind this? Why not just keep the call?

And in vc-svn-registered, for example. You re-signal whatever error is 
caught, without any alternative. Do we need the condition-case there at 
all, then?

Furthermore, we'll have to examine the resulting effect on the behavior 
of various backend methods. Apparently, up until now vc-svn-registered 
never signaled an error (swallowed all of them). And now whatever 
callers it has, will need to handle possible errors.

It's probably fine, though. vc-backend is a more popular function, and 
it seems not much is changing for it, since, for some reason, 
vc-refresh-state wrapped its call in with-demoted-errors anyway.

But I think we have other callers of it in-tree, and not all of them 
guard with with-demoted-errors or condition-case. What do you think we 
should do with them?

> It is probably an abuse of ’pcase’.  Is ’cond’ better here?  Last,
> I have not found in the documentation how to differentiate what it is
> raised depending on the error type, hence the ’pcase’.

I think you just need to write the specific error type instead of 
'error' in the handler clause.

Regarding the rest of the patch, could you explain the change in 
vc-bzr-state-heuristic? Looking at the code, I figure the idea was to 
future-proof the parser against some future change in file format, to 
fall back to (slower) calling the 'bzr' program. Are we somehow certain 
now this is not needed?