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.


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.


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

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 hope all this is helpful and going in the right direction for
improving the reported messages.  If not, let me know what could be
better.

Cheers,
simon


PS: If this patch makes sense for inclusion, then let me know and I will
complete the Copyright Assignment process.

Simon Tournier (1):
  Handle error of 'vc-registered'

 lisp/vc/vc-bzr.el        | 82 ++++++++++++++++++++--------------------
 lisp/vc/vc-dispatcher.el | 12 +++++-
 lisp/vc/vc-git.el        | 24 +++++++-----
 lisp/vc/vc-hg.el         | 13 +++----
 lisp/vc/vc-hooks.el      | 11 +++++-
 lisp/vc/vc-svn.el        |  5 +--
 6 files changed, 84 insertions(+), 63 deletions(-)


base-commit: 1007800a5994ac49b6bc9cd7528edb2d709d2031
-- 
2.36.0