[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#57407: [PATCH] Handle error of ’vc-registered’
From: |
Dmitry Gutov |
Subject: |
bug#57407: [PATCH] Handle error of ’vc-registered’ |
Date: |
Mon, 12 Sep 2022 04:08:23 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 |
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?