GNU bug report logs - #57407
[PATCH] Handle error of ’vc-registered’

Previous Next

To reply to this bug, email your comments to 57407 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Handle error of ’vc-registered’
Date: Thu, 25 Aug 2022 18:20:07 +0200

[Message part 1 (text/plain, inline)]

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

[0001-Handle-error-of-vc-registered.patch (text/x-diff, attachment)]

Message #8 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 57407 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Sun, 04 Sep 2022 23:54:36 +0200

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> 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

Makes sense to me.  Perhaps Dmitry has some comments; added to the CCs.

Message #13 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 57407 <at> debbugs.gnu.org
Subject: Copyright Assignment done (was bug#57407: [PATCH] Handle error of
 ’vc-registered’)
Date: Thu, 08 Sep 2022 17:25:31 +0200

Hi,

On jeu., 25 août 2022 at 18:20, Simon Tournier <zimon.toutoune <at> gmail.com> wrote:

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

Just to let you know that now the Copyright Assignment process is
complete.


Cheers,
simon

Message #16 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Simon Tournier <zimon.toutoune <at> gmail.com>, 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Mon, 12 Sep 2022 04:08:23 +0300

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?

Message #19 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Mon, 12 Sep 2022 14:18:52 +0200

Hi,

Thanks for your comments.


On lun., 12 sept. 2022 at 04:08, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

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

Somehow yes.  More explicitly, replace “Error: <useless message>” with
“Warning: <more helpful>”.


> 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?

What do you mean?  You need to catch the error and propagates it.

If the heuristic fails in vc-bzr-state-heuristic, then ’vc-bzr-state’ is
called, which itself then calls ’vc-bzr-status’.  This status function
can signal an error and it requires to be raised again.


> 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?

Yes, you need to re-throw the error to propagate it.  See
’(elisp)Handling Errors’:

     Sometimes it is necessary to re-throw a signal caught by
     ‘condition-case’, for some outer-level handler to catch.  Here’s
     how to do that:

            (signal (car err) (cdr err))

     where ‘err’ is the error description variable, the first argument
     to ‘condition-case’ whose error condition you want to re-throw.

Or I am missing a point. :-)


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

I think it is what this patch is doing: handle possible errors by the
’vc-<backend>-registered’ callers.


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

For what my opinion is worth, the commit
991e145450ec8b02865597bc80fd797e39e81f07 – which replaces
’ignore-errors’ with ’with-demoted-errors’ only for the Git backend – is
a valuable idea but incorrectly implemented.  This patch is an attempt
to improve the situation.


> 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?

Could you be more specific about these “other callers”?  From my
understanding, ’vc-<backend>-registered’ is called by ’vc-registered’
and this patch handles this case; even ’vc-registered’ is not modified
by this patch and the handler happens in ’vc-refresh-state’.

Moreover,

--8<---------------cut here---------------start------------->8---
$ for backend in svn git hg ;do ag --elisp vc-${backend}-registered ;done
lisp/ldefs-boot.el
33455: (defun vc-svn-registered (f)
33462:      (vc-svn-registered f))))

lisp/vc/vc-svn.el
110:;; vc-svn-registered, but we want the value to be compiled at startup, not
133:;;;###autoload (defun vc-svn-registered (f)
140:;;;###autoload       (vc-svn-registered f))))
142:(defun vc-svn-registered (file)
242:  (vc-svn-registered file)
lisp/ldefs-boot.el
33399: (defun vc-git-registered (file)
33404:        (vc-git-registered file))))

lisp/vc/vc-git.el
244:;;;###autoload (defun vc-git-registered (file)
249:;;;###autoload         (vc-git-registered file))))
251:(defun vc-git-registered (file)
lisp/ldefs-boot.el
33410: (defun vc-hg-registered (file)
33415:        (vc-hg-registered file))))

lisp/vc/vc-hg.el
85:;; vc-hg-registered and vc-hg-state
198:;;;###autoload (defun vc-hg-registered (file)
203:;;;###autoload         (vc-hg-registered file))))
206:(defun vc-hg-registered (file)
--8<---------------cut here---------------end--------------->8---

means that another caller is ’vc-svn-working-revision’ used by,

--8<---------------cut here---------------start------------->8---
(defun vc-working-revision (file &optional backend)
  "Return the repository version from which FILE was checked out.
If FILE is not registered, this function always returns nil."
  (or (vc-file-getprop file 'vc-working-revision)
      (progn
        (setq backend (or backend (vc-backend file)))
        (when backend
          (vc-file-setprop file 'vc-working-revision
                           (vc-call-backend
                            backend 'working-revision file))))))
--8<---------------cut here---------------end--------------->8---

Therefore, ’vc-svn-working-revision’ should also be guarded with
’condition-case’ and display an appropriate message, indeed.


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

Maybe I am missing a point about error handling and how they propagate.
If I consider this change removing ’pcase’ and write the specific error
as you are suggesting,

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 778d1139fc..39ad27f2fd 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -361,15 +361,13 @@ vc-do-command
 	    (let ((buffer-undo-list t))
               (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)))))))
+                ((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))))))
 	    (when (and (not (eq t okstatus))
 		       (or (not (integerp status))
 			   (and okstatus (< okstatus status))))
--8<---------------cut here---------------end--------------->8---

then instead of,

--8<---------------cut here---------------start------------->8---
Warning: (vc-not-supported "Searching for program" "No such file or directory" "git")
--8<---------------cut here---------------end--------------->8---

the report is,

--8<---------------cut here---------------start------------->8---
VC refresh error: (void-function _)
--8<---------------cut here---------------end--------------->8---

Well, I do not know how to avoid the ’pcase’ here to correctly propagate
the errors.


About the other ’pcase’, this code,

--8<---------------cut here---------------start------------->8---
       ((setq backend (condition-case err
                          (vc-backend buffer-file-name)
                        ((vc-not-supported
                          (message "Warning: %S" err))
                         (_
                          (message "VC refresh error: %S" err)))))
--8<---------------cut here---------------end--------------->8---

does not raise the correct message.


Somehow, I probably miss how to correctly propagate errors but the patch
is, IMHO, the simplest way.


> 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?

Nothing has really changed.  The current pattern is:

--8<---------------cut here---------------start------------->8---
      (condition-case err
          (with-temp-buffer
            ...
            (if (not (looking-at "#bazaar dirstate flat format 3"))
                (vc-bzr-state file)     ; Some other unknown format?
              (let* ...)))
        ;; The dirstate file can't be read, or some other problem.
        (error
         (message "Falling back on \"slow\" status detection (%S)" err)
         (vc-bzr-state file)))
--8<---------------cut here---------------end--------------->8---

where it means ’vc-bzr-state’ is at two places – which is redundant.
Instead, the pattern,

--8<---------------cut here---------------start------------->8---
      (condition-case err
          (with-temp-buffer
            ...
            (if (not (looking-at "#bazaar dirstate flat format 3"))
                (signal 'error "VC: Bzr dirstate is not flat format 3")
              (let* ...)))
        ;; The dirstate file can't be read, or some other problem.
        (error
         (message "Falling back on \"slow\" status detection (%S)" err)
         (vc-bzr-state file)))
--8<---------------cut here---------------end--------------->8---

is better because it appears only at one location.  Now, knowing if this
test about the BZR format is relevant or not is another story. :-)

The patch is just tweaking how the errors are handled, not the logic
behind.  In another patch, this ’vc-bzr-state-heuristic’ could be split;
as it is done with HG: vc-hg-state calling vc-hg-state-fast falling back
to vc-hg-state-slow – instead of having the fast (heuristic) included.


Let me know how to proceed for helping in the review process.

All the best,
simon

Message #22 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 57407 <at> debbugs.gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Mon, 26 Sep 2022 18:58:41 +0200

Hi,

This is a friendly ping about patch #57407 [1].

1: <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57407>


I would like to know what is the next step and how I can help for
testing and/or the reviewing process about the patch.


On Thu, 25 Aug 2022 at 18:20, Simon Tournier <zimon.toutoune <at> gmail.com> wrote:

> 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



All the best,
simon

Message #25 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Juri Linkov <juri <at> linkov.net>, 57407 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Tue, 27 Sep 2022 13:39:11 +0200

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> This is a friendly ping about patch #57407 [1].
>
> 1: <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57407>
>
> I would like to know what is the next step and how I can help for
> testing and/or the reviewing process about the patch.

If Dmitry is busy at the moment, perhaps Juri has some comments here?
Added to the CCs.

Message #28 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Dmitry Gutov <dgutov <at> yandex.ru>, 57407 <at> debbugs.gnu.org,
 Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Tue, 27 Sep 2022 21:50:49 +0300

>> This is a friendly ping about patch #57407 [1].
>>
>> 1: <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=57407>
>>
>> I would like to know what is the next step and how I can help for
>> testing and/or the reviewing process about the patch.
>
> If Dmitry is busy at the moment, perhaps Juri has some comments here?
> Added to the CCs.

Sorry, I have no knowledge about this part of VC.
My relation with VC was only in adding missing features.

Message #31 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Fri, 30 Sep 2022 03:55:50 +0300

On 12.09.2022 15:18, Simon Tournier wrote:
> Hi,
> 
> Thanks for your comments.
> 
> 
> On lun., 12 sept. 2022 at 04:08, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> 
>> Do I take this right that the main purpose of the patch is to have the "Error:
>> ..." messages replaced with "Warning: ..."?
> 
> Somehow yes.  More explicitly, replace “Error: <useless message>” with
> “Warning: <more helpful>”.

Sounds good.

>> 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?
> 
> What do you mean?  You need to catch the error and propagates it.

Why not just let it bubble up? Why catch it at all here?

There is one condition-case that actually does something (in 
vc-do-command), but adding a condition-case with exactly one handler 
which simply re-throws doesn't seem to change anything (except 
swallowing a part of the backtrace).

> If the heuristic fails in vc-bzr-state-heuristic, then ’vc-bzr-state’ is
> called, which itself then calls ’vc-bzr-status’.  This status function
> can signal an error and it requires to be raised again.
> 
> 
>> 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?
> 
> Yes, you need to re-throw the error to propagate it.  See
> ’(elisp)Handling Errors’:
> 
>       Sometimes it is necessary to re-throw a signal caught by
>       ‘condition-case’, for some outer-level handler to catch.  Here’s
>       how to do that:
> 
>              (signal (car err) (cdr err))
> 
>       where ‘err’ is the error description variable, the first argument
>       to ‘condition-case’ whose error condition you want to re-throw.
> 
> Or I am missing a point. :-)

You indeed might end up re-signaling errors this way, but that's only 
useful if the condition-case form has some other purpose to begin with 
(aside from re-throwing). Like catching different kinds of errors, 
logging some one way and re-throwing others. Like you do in vc-do-command.

If you simply want to stop swallowing the errors in some form, you can 
just remove the condition-case form.

>> 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.
> 
> I think it is what this patch is doing: handle possible errors by the
> ’vc-<backend>-registered’ callers.

Have you looked at this comment in vc-svn-registered?

  ;; Some problem happened.  E.g. We can't find an `svn'
  ;; executable.  We used to only catch `file-error' but when
  ;; the process is run on a remote host via Tramp, the error
  ;; is only reported via the exit status which is turned into
  ;; an `error' by vc-do-command.

I wonder if that's still true.

>> 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.
> 
> For what my opinion is worth, the commit
> 991e145450ec8b02865597bc80fd797e39e81f07 – which replaces
> ’ignore-errors’ with ’with-demoted-errors’ only for the Git backend – is
> a valuable idea but incorrectly implemented.  This patch is an attempt
> to improve the situation.
> 
> 
>> 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?
> 
> Could you be more specific about these “other callers”?  From my
> understanding, ’vc-<backend>-registered’ is called by ’vc-registered’
> and this patch handles this case; even ’vc-registered’ is not modified
> by this patch and the handler happens in ’vc-refresh-state’.

vc-registered and vc-dir-recompute-file-state call it.

And vc-deduce-fileset-1 calls vc-registered. I suppose some or all of 
these should catch errors now?

> Moreover,
> 
> --8<---------------cut here---------------start------------->8---
> $ for backend in svn git hg ;do ag --elisp vc-${backend}-registered ;done
> lisp/ldefs-boot.el
> 33455: (defun vc-svn-registered (f)
> 33462:      (vc-svn-registered f))))
> 
> lisp/vc/vc-svn.el
> 110:;; vc-svn-registered, but we want the value to be compiled at startup, not
> 133:;;;###autoload (defun vc-svn-registered (f)
> 140:;;;###autoload       (vc-svn-registered f))))
> 142:(defun vc-svn-registered (file)
> 242:  (vc-svn-registered file)
> lisp/ldefs-boot.el
> 33399: (defun vc-git-registered (file)
> 33404:        (vc-git-registered file))))
> 
> lisp/vc/vc-git.el
> 244:;;;###autoload (defun vc-git-registered (file)
> 249:;;;###autoload         (vc-git-registered file))))
> 251:(defun vc-git-registered (file)
> lisp/ldefs-boot.el
> 33410: (defun vc-hg-registered (file)
> 33415:        (vc-hg-registered file))))
> 
> lisp/vc/vc-hg.el
> 85:;; vc-hg-registered and vc-hg-state
> 198:;;;###autoload (defun vc-hg-registered (file)
> 203:;;;###autoload         (vc-hg-registered file))))
> 206:(defun vc-hg-registered (file)
> --8<---------------cut here---------------end--------------->8---
> 
> means that another caller is ’vc-svn-working-revision’ used by,
> 
> --8<---------------cut here---------------start------------->8---
> (defun vc-working-revision (file &optional backend)
>    "Return the repository version from which FILE was checked out.
> If FILE is not registered, this function always returns nil."
>    (or (vc-file-getprop file 'vc-working-revision)
>        (progn
>          (setq backend (or backend (vc-backend file)))
>          (when backend
>            (vc-file-setprop file 'vc-working-revision
>                             (vc-call-backend
>                              backend 'working-revision file))))))
> --8<---------------cut here---------------end--------------->8---
> 
> Therefore, ’vc-svn-working-revision’ should also be guarded with
> ’condition-case’ and display an appropriate message, indeed.
> 
> 
>>> 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.
> 
> Maybe I am missing a point about error handling and how they propagate.
> If I consider this change removing ’pcase’ and write the specific error
> as you are suggesting,
> 
> --8<---------------cut here---------------start------------->8---
> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index 778d1139fc..39ad27f2fd 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -361,15 +361,13 @@ vc-do-command
>   	    (let ((buffer-undo-list t))
>                 (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)))))))
> +                ((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))))))
>   	    (when (and (not (eq t okstatus))
>   		       (or (not (integerp status))
>   			   (and okstatus (< okstatus status))))
> --8<---------------cut here---------------end--------------->8---
> 
> then instead of,
> 
> --8<---------------cut here---------------start------------->8---
> Warning: (vc-not-supported "Searching for program" "No such file or directory" "git")
> --8<---------------cut here---------------end--------------->8---
> 
> the report is,
> 
> --8<---------------cut here---------------start------------->8---
> VC refresh error: (void-function _)
> --8<---------------cut here---------------end--------------->8---

_ is syntax specific to pcase. condition-case uses 't' for the fallback 
handler.

> Well, I do not know how to avoid the ’pcase’ here to correctly propagate
> the errors.
> 
> 
> About the other ’pcase’, this code,
> 
> --8<---------------cut here---------------start------------->8---
>         ((setq backend (condition-case err
>                            (vc-backend buffer-file-name)
>                          ((vc-not-supported
>                            (message "Warning: %S" err))
>                           (_
>                            (message "VC refresh error: %S" err)))))
> --8<---------------cut here---------------end--------------->8---
> 
> does not raise the correct message.

Does this work for you?

((setq backend (condition-case err
                   (vc-backend buffer-file-name)
                 (vc-not-supported
                  (message "Warning: %S" err))
                 (t
                  (message "VC refresh error: %S" err))))

And you can update the previous (more complex) example similarly, 
without pcase.

> Somehow, I probably miss how to correctly propagate errors but the patch
> is, IMHO, the simplest way.
> 
> 
>> 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?
> 
> Nothing has really changed.  The current pattern is:
> 
> --8<---------------cut here---------------start------------->8---
>        (condition-case err
>            (with-temp-buffer
>              ...
>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>                  (vc-bzr-state file)     ; Some other unknown format?
>                (let* ...)))
>          ;; The dirstate file can't be read, or some other problem.
>          (error
>           (message "Falling back on \"slow\" status detection (%S)" err)
>           (vc-bzr-state file)))
> --8<---------------cut here---------------end--------------->8---
> 
> where it means ’vc-bzr-state’ is at two places – which is redundant.
> Instead, the pattern,
> 
> --8<---------------cut here---------------start------------->8---
>        (condition-case err
>            (with-temp-buffer
>              ...
>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>                  (signal 'error "VC: Bzr dirstate is not flat format 3")
>                (let* ...)))
>          ;; The dirstate file can't be read, or some other problem.
>          (error
>           (message "Falling back on \"slow\" status detection (%S)" err)
>           (vc-bzr-state file)))
> --8<---------------cut here---------------end--------------->8---
> 
> is better because it appears only at one location.  Now, knowing if this
> test about the BZR format is relevant or not is another story. :-)

Makes sense now, thanks.

> The patch is just tweaking how the errors are handled, not the logic
> behind.  In another patch, this ’vc-bzr-state-heuristic’ could be split;
> as it is done with HG: vc-hg-state calling vc-hg-state-fast falling back
> to vc-hg-state-slow – instead of having the fast (heuristic) included.

Up to you.

> Let me know how to proceed for helping in the review process.

Let's address the raised points first. Thank you.

Message #34 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 57407 <at> debbugs.gnu.org, Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Wed, 6 Sep 2023 15:48:45 -0700

That was one year ago.

Simon, did you have a chance to look into the issues that Dmitry
mentioned below?

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 12.09.2022 15:18, Simon Tournier wrote:
>> Hi,
>> Thanks for your comments.
>> On lun., 12 sept. 2022 at 04:08, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>>
>>> Do I take this right that the main purpose of the patch is to have the "Error:
>>> ..." messages replaced with "Warning: ..."?
>> Somehow yes.  More explicitly, replace “Error: <useless message>” with
>> “Warning: <more helpful>”.
>
> Sounds good.
>
>>> 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?
>> What do you mean?  You need to catch the error and propagates it.
>
> Why not just let it bubble up? Why catch it at all here?
>
> There is one condition-case that actually does something (in vc-do-command), but
> adding a condition-case with exactly one handler which simply re-throws doesn't
> seem to change anything (except swallowing a part of the backtrace).
>
>> If the heuristic fails in vc-bzr-state-heuristic, then ’vc-bzr-state’ is
>> called, which itself then calls ’vc-bzr-status’.  This status function
>> can signal an error and it requires to be raised again.
>>
>>> 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?
>> Yes, you need to re-throw the error to propagate it.  See
>> ’(elisp)Handling Errors’:
>>       Sometimes it is necessary to re-throw a signal caught by
>>       ‘condition-case’, for some outer-level handler to catch.  Here’s
>>       how to do that:
>>              (signal (car err) (cdr err))
>>       where ‘err’ is the error description variable, the first argument
>>       to ‘condition-case’ whose error condition you want to re-throw.
>> Or I am missing a point. :-)
>
> You indeed might end up re-signaling errors this way, but that's only useful if
> the condition-case form has some other purpose to begin with (aside from
> re-throwing). Like catching different kinds of errors, logging some one way and
> re-throwing others. Like you do in vc-do-command.
>
> If you simply want to stop swallowing the errors in some form, you can just
> remove the condition-case form.
>
>>> 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.
>> I think it is what this patch is doing: handle possible errors by the
>> ’vc-<backend>-registered’ callers.
>
> Have you looked at this comment in vc-svn-registered?
>
>   ;; Some problem happened.  E.g. We can't find an `svn'
>   ;; executable.  We used to only catch `file-error' but when
>   ;; the process is run on a remote host via Tramp, the error
>   ;; is only reported via the exit status which is turned into
>   ;; an `error' by vc-do-command.
>
> I wonder if that's still true.
>
>>> 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.
>> For what my opinion is worth, the commit
>> 991e145450ec8b02865597bc80fd797e39e81f07 – which replaces
>> ’ignore-errors’ with ’with-demoted-errors’ only for the Git backend – is
>> a valuable idea but incorrectly implemented.  This patch is an attempt
>> to improve the situation.
>>
>>> 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?
>> Could you be more specific about these “other callers”?  From my
>> understanding, ’vc-<backend>-registered’ is called by ’vc-registered’
>> and this patch handles this case; even ’vc-registered’ is not modified
>> by this patch and the handler happens in ’vc-refresh-state’.
>
> vc-registered and vc-dir-recompute-file-state call it.
>
> And vc-deduce-fileset-1 calls vc-registered. I suppose some or all of these
> should catch errors now?
>
>> Moreover,
>> --8<---------------cut here---------------start------------->8---
>> $ for backend in svn git hg ;do ag --elisp vc-${backend}-registered ;done
>> lisp/ldefs-boot.el
>> 33455: (defun vc-svn-registered (f)
>> 33462:      (vc-svn-registered f))))
>> lisp/vc/vc-svn.el
>> 110:;; vc-svn-registered, but we want the value to be compiled at startup, not
>> 133:;;;###autoload (defun vc-svn-registered (f)
>> 140:;;;###autoload       (vc-svn-registered f))))
>> 142:(defun vc-svn-registered (file)
>> 242:  (vc-svn-registered file)
>> lisp/ldefs-boot.el
>> 33399: (defun vc-git-registered (file)
>> 33404:        (vc-git-registered file))))
>> lisp/vc/vc-git.el
>> 244:;;;###autoload (defun vc-git-registered (file)
>> 249:;;;###autoload         (vc-git-registered file))))
>> 251:(defun vc-git-registered (file)
>> lisp/ldefs-boot.el
>> 33410: (defun vc-hg-registered (file)
>> 33415:        (vc-hg-registered file))))
>> lisp/vc/vc-hg.el
>> 85:;; vc-hg-registered and vc-hg-state
>> 198:;;;###autoload (defun vc-hg-registered (file)
>> 203:;;;###autoload         (vc-hg-registered file))))
>> 206:(defun vc-hg-registered (file)
>> --8<---------------cut here---------------end--------------->8---
>> means that another caller is ’vc-svn-working-revision’ used by,
>> --8<---------------cut here---------------start------------->8---
>> (defun vc-working-revision (file &optional backend)
>>    "Return the repository version from which FILE was checked out.
>> If FILE is not registered, this function always returns nil."
>>    (or (vc-file-getprop file 'vc-working-revision)
>>        (progn
>>          (setq backend (or backend (vc-backend file)))
>>          (when backend
>>            (vc-file-setprop file 'vc-working-revision
>>                             (vc-call-backend
>>                              backend 'working-revision file))))))
>> --8<---------------cut here---------------end--------------->8---
>> Therefore, ’vc-svn-working-revision’ should also be guarded with
>> ’condition-case’ and display an appropriate message, indeed.
>>
>>>> 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.
>> Maybe I am missing a point about error handling and how they propagate.
>> If I consider this change removing ’pcase’ and write the specific error
>> as you are suggesting,
>> --8<---------------cut here---------------start------------->8---
>> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
>> index 778d1139fc..39ad27f2fd 100644
>> --- a/lisp/vc/vc-dispatcher.el
>> +++ b/lisp/vc/vc-dispatcher.el
>> @@ -361,15 +361,13 @@ vc-do-command
>>   	    (let ((buffer-undo-list t))
>>                 (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)))))))
>> +                ((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))))))
>>   	    (when (and (not (eq t okstatus))
>>   		       (or (not (integerp status))
>>   			   (and okstatus (< okstatus status))))
>> --8<---------------cut here---------------end--------------->8---
>> then instead of,
>> --8<---------------cut here---------------start------------->8---
>> Warning: (vc-not-supported "Searching for program" "No such file or directory" "git")
>> --8<---------------cut here---------------end--------------->8---
>> the report is,
>> --8<---------------cut here---------------start------------->8---
>> VC refresh error: (void-function _)
>> --8<---------------cut here---------------end--------------->8---
>
> _ is syntax specific to pcase. condition-case uses 't' for the fallback handler.
>
>> Well, I do not know how to avoid the ’pcase’ here to correctly propagate
>> the errors.
>> About the other ’pcase’, this code,
>> --8<---------------cut here---------------start------------->8---
>>         ((setq backend (condition-case err
>>                            (vc-backend buffer-file-name)
>>                          ((vc-not-supported
>>                            (message "Warning: %S" err))
>>                           (_
>>                            (message "VC refresh error: %S" err)))))
>> --8<---------------cut here---------------end--------------->8---
>> does not raise the correct message.
>
> Does this work for you?
>
> ((setq backend (condition-case err
>                    (vc-backend buffer-file-name)
>                  (vc-not-supported
>                   (message "Warning: %S" err))
>                  (t
>                   (message "VC refresh error: %S" err))))
>
> And you can update the previous (more complex) example similarly, without pcase.
>
>> Somehow, I probably miss how to correctly propagate errors but the patch
>> is, IMHO, the simplest way.
>>
>>> 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?
>> Nothing has really changed.  The current pattern is:
>> --8<---------------cut here---------------start------------->8---
>>        (condition-case err
>>            (with-temp-buffer
>>              ...
>>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>>                  (vc-bzr-state file)     ; Some other unknown format?
>>                (let* ...)))
>>          ;; The dirstate file can't be read, or some other problem.
>>          (error
>>           (message "Falling back on \"slow\" status detection (%S)" err)
>>           (vc-bzr-state file)))
>> --8<---------------cut here---------------end--------------->8---
>> where it means ’vc-bzr-state’ is at two places – which is redundant.
>> Instead, the pattern,
>> --8<---------------cut here---------------start------------->8---
>>        (condition-case err
>>            (with-temp-buffer
>>              ...
>>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>>                  (signal 'error "VC: Bzr dirstate is not flat format 3")
>>                (let* ...)))
>>          ;; The dirstate file can't be read, or some other problem.
>>          (error
>>           (message "Falling back on \"slow\" status detection (%S)" err)
>>           (vc-bzr-state file)))
>> --8<---------------cut here---------------end--------------->8---
>> is better because it appears only at one location.  Now, knowing if this
>> test about the BZR format is relevant or not is another story. :-)
>
> Makes sense now, thanks.
>
>> The patch is just tweaking how the errors are handled, not the logic
>> behind.  In another patch, this ’vc-bzr-state-heuristic’ could be split;
>> as it is done with HG: vc-hg-state calling vc-hg-state-fast falling back
>> to vc-hg-state-slow – instead of having the fast (heuristic) included.
>
> Up to you.
>
>> Let me know how to proceed for helping in the review process.
>
> Let's address the raised points first. Thank you.

Message #39 received at 57407-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 57407-done <at> debbugs.gnu.org, Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Sun, 9 Jun 2024 17:01:36 -0400

Stefan Kangas <stefankangas <at> gmail.com> writes:

> That was one year ago.
>
> Simon, did you have a chance to look into the issues that Dmitry
> mentioned below?

More information was requested, but none was given within 9 months, so
I'm closing this bug.  If this is still an issue, please reply to this
email (use "Reply to all" in your email client) and we can reopen the
bug report.

Message #42 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: stefankangas <at> gmail.com
Cc: 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: closed (Re: bug#57407: [PATCH] Handle error of
 ’vc-registered’)
Date: Mon, 10 Jun 2024 16:03:06 +0200

Hi,

On Sun, 09 Jun 2024 at 22:18, help-debbugs <at> gnu.org (GNU bug Tracking System) wrote:

>> That was one year ago.
>>
>> Simon, did you have a chance to look into the issues that Dmitry
>> mentioned below?
>
> More information was requested, but none was given within 9 months, so
> I'm closing this bug.  If this is still an issue, please reply to this
> email (use "Reply to all" in your email client) and we can reopen the
> bug report.

The issue is not gone, AFAICT.  The cover letter provides a reproducer;
see below.

> From: Simon Tournier <zimon.toutoune <at> gmail.com>
> Subject: [PATCH] Handle error of ’vc-registered’
> To: bug-gnu-emacs <at> gnu.org
> Date: Thu, 25 Aug 2022 18:20:07 +0200
> Date: Thu, 25 Aug 2022 18:20:07 +0200 (1 year, 41 weeks, 2 days ago)
>
> 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:
>
> $  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
>
>
> 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.
>
> $ 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

Well, I am not following very closely the development of Emacs master,
so I cannot tell with high confidence if a workaround introduced
elsewhere fixes the issue.  However, from my quick look, the code that
triggers the spurious messages has not been changed.

Sorry to not have the time to send a v2; I am running out of time.
However, closing this report:

 1. Do not change that 991e145450ec8b02865597bc80fd797e39e81f07 is
    clearly incorrect.  It’s a regression

 2. My patch, while imperfect, fixes such regression.

Cheers,
simon

Message #45 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: closed (Re: bug#57407: [PATCH] Handle error of ’vc-registered’)
Date: Tue, 18 Jun 2024 12:04:23 -0700

reopen 57407
thanks

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

>>> That was one year ago.
>>>
>>> Simon, did you have a chance to look into the issues that Dmitry
>>> mentioned below?
>>
>> More information was requested, but none was given within 9 months, so
>> I'm closing this bug.  If this is still an issue, please reply to this
>> email (use "Reply to all" in your email client) and we can reopen the
>> bug report.
>
> The issue is not gone, AFAICT.  The cover letter provides a reproducer;
> see below.

OK, thanks, reopening the bug.

> Well, I am not following very closely the development of Emacs master,
> so I cannot tell with high confidence if a workaround introduced
> elsewhere fixes the issue.  However, from my quick look, the code that
> triggers the spurious messages has not been changed.
>
> Sorry to not have the time to send a v2; I am running out of time.
> However, closing this report:
>
>  1. Do not change that 991e145450ec8b02865597bc80fd797e39e81f07 is
>     clearly incorrect.  It’s a regression
>
>  2. My patch, while imperfect, fixes such regression.

Dmitry, what do you think of installing the proposed patch as is?

Does it introduce any other regressions?

Message #50 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: closed (Re: bug#57407: [PATCH] Handle error of ’vc-registered’)
Date: Tue, 18 Jun 2024 22:41:19 +0200

Hi Stefan,

Thank you for keeping an eye on the tracker.  It's very valuable in
order to maintain actionable fixes.

On Tue, 18 Jun 2024 at 21:04, Stefan Kangas <stefankangas <at> gmail.com> wrote:

> Dmitry, what do you think of installing the proposed patch as is?

The previous comments appear to me very good ones and they are real
improvements.  I do not promise to implement in timely manner but I
will do my best to dedicate some time before my summer holidays --
always the good time for rushing and cleaning the endless TODO list.
;-)

Cheers,
simon

Message #53 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: closed (Re: bug#57407: [PATCH] Handle error of ’vc-registered’)
Date: Tue, 18 Jun 2024 14:17:41 -0700

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> The previous comments appear to me very good ones and they are real
> improvements.  I do not promise to implement in timely manner but I
> will do my best to dedicate some time before my summer holidays --
> always the good time for rushing and cleaning the endless TODO list.
> ;-)

Sounds good.  Please let us know when you have a new patch.

Message #56 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 57407 <at> debbugs.gnu.org, Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: bug#57407: [PATCH] Handle error of ’vc-registered’
Date: Mon, 10 Feb 2025 23:26:39 -0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> That was one year ago.
>
> Simon, did you have a chance to look into the issues that Dmitry
> mentioned below?

Friendly ping.

> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> On 12.09.2022 15:18, Simon Tournier wrote:
>>> Hi,
>>> Thanks for your comments.
>>> On lun., 12 sept. 2022 at 04:08, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>>>
>>>> Do I take this right that the main purpose of the patch is to have the "Error:
>>>> ..." messages replaced with "Warning: ..."?
>>> Somehow yes.  More explicitly, replace “Error: <useless message>” with
>>> “Warning: <more helpful>”.
>>
>> Sounds good.
>>
>>>> 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?
>>> What do you mean?  You need to catch the error and propagates it.
>>
>> Why not just let it bubble up? Why catch it at all here?
>>
>> There is one condition-case that actually does something (in vc-do-command), but
>> adding a condition-case with exactly one handler which simply re-throws doesn't
>> seem to change anything (except swallowing a part of the backtrace).
>>
>>> If the heuristic fails in vc-bzr-state-heuristic, then ’vc-bzr-state’ is
>>> called, which itself then calls ’vc-bzr-status’.  This status function
>>> can signal an error and it requires to be raised again.
>>>
>>>> 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?
>>> Yes, you need to re-throw the error to propagate it.  See
>>> ’(elisp)Handling Errors’:
>>>       Sometimes it is necessary to re-throw a signal caught by
>>>       ‘condition-case’, for some outer-level handler to catch.  Here’s
>>>       how to do that:
>>>              (signal (car err) (cdr err))
>>>       where ‘err’ is the error description variable, the first argument
>>>       to ‘condition-case’ whose error condition you want to re-throw.
>>> Or I am missing a point. :-)
>>
>> You indeed might end up re-signaling errors this way, but that's only useful if
>> the condition-case form has some other purpose to begin with (aside from
>> re-throwing). Like catching different kinds of errors, logging some one way and
>> re-throwing others. Like you do in vc-do-command.
>>
>> If you simply want to stop swallowing the errors in some form, you can just
>> remove the condition-case form.
>>
>>>> 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.
>>> I think it is what this patch is doing: handle possible errors by the
>>> ’vc-<backend>-registered’ callers.
>>
>> Have you looked at this comment in vc-svn-registered?
>>
>>   ;; Some problem happened.  E.g. We can't find an `svn'
>>   ;; executable.  We used to only catch `file-error' but when
>>   ;; the process is run on a remote host via Tramp, the error
>>   ;; is only reported via the exit status which is turned into
>>   ;; an `error' by vc-do-command.
>>
>> I wonder if that's still true.
>>
>>>> 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.
>>> For what my opinion is worth, the commit
>>> 991e145450ec8b02865597bc80fd797e39e81f07 – which replaces
>>> ’ignore-errors’ with ’with-demoted-errors’ only for the Git backend – is
>>> a valuable idea but incorrectly implemented.  This patch is an attempt
>>> to improve the situation.
>>>
>>>> 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?
>>> Could you be more specific about these “other callers”?  From my
>>> understanding, ’vc-<backend>-registered’ is called by ’vc-registered’
>>> and this patch handles this case; even ’vc-registered’ is not modified
>>> by this patch and the handler happens in ’vc-refresh-state’.
>>
>> vc-registered and vc-dir-recompute-file-state call it.
>>
>> And vc-deduce-fileset-1 calls vc-registered. I suppose some or all of these
>> should catch errors now?
>>
>>> Moreover,
>>> --8<---------------cut here---------------start------------->8---
>>> $ for backend in svn git hg ;do ag --elisp vc-${backend}-registered ;done
>>> lisp/ldefs-boot.el
>>> 33455: (defun vc-svn-registered (f)
>>> 33462:      (vc-svn-registered f))))
>>> lisp/vc/vc-svn.el
>>> 110:;; vc-svn-registered, but we want the value to be compiled at startup, not
>>> 133:;;;###autoload (defun vc-svn-registered (f)
>>> 140:;;;###autoload       (vc-svn-registered f))))
>>> 142:(defun vc-svn-registered (file)
>>> 242:  (vc-svn-registered file)
>>> lisp/ldefs-boot.el
>>> 33399: (defun vc-git-registered (file)
>>> 33404:        (vc-git-registered file))))
>>> lisp/vc/vc-git.el
>>> 244:;;;###autoload (defun vc-git-registered (file)
>>> 249:;;;###autoload         (vc-git-registered file))))
>>> 251:(defun vc-git-registered (file)
>>> lisp/ldefs-boot.el
>>> 33410: (defun vc-hg-registered (file)
>>> 33415:        (vc-hg-registered file))))
>>> lisp/vc/vc-hg.el
>>> 85:;; vc-hg-registered and vc-hg-state
>>> 198:;;;###autoload (defun vc-hg-registered (file)
>>> 203:;;;###autoload         (vc-hg-registered file))))
>>> 206:(defun vc-hg-registered (file)
>>> --8<---------------cut here---------------end--------------->8---
>>> means that another caller is ’vc-svn-working-revision’ used by,
>>> --8<---------------cut here---------------start------------->8---
>>> (defun vc-working-revision (file &optional backend)
>>>    "Return the repository version from which FILE was checked out.
>>> If FILE is not registered, this function always returns nil."
>>>    (or (vc-file-getprop file 'vc-working-revision)
>>>        (progn
>>>          (setq backend (or backend (vc-backend file)))
>>>          (when backend
>>>            (vc-file-setprop file 'vc-working-revision
>>>                             (vc-call-backend
>>>                              backend 'working-revision file))))))
>>> --8<---------------cut here---------------end--------------->8---
>>> Therefore, ’vc-svn-working-revision’ should also be guarded with
>>> ’condition-case’ and display an appropriate message, indeed.
>>>
>>>>> 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.
>>> Maybe I am missing a point about error handling and how they propagate.
>>> If I consider this change removing ’pcase’ and write the specific error
>>> as you are suggesting,
>>> --8<---------------cut here---------------start------------->8---
>>> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
>>> index 778d1139fc..39ad27f2fd 100644
>>> --- a/lisp/vc/vc-dispatcher.el
>>> +++ b/lisp/vc/vc-dispatcher.el
>>> @@ -361,15 +361,13 @@ vc-do-command
>>>   	    (let ((buffer-undo-list t))
>>>                 (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)))))))
>>> +                ((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))))))
>>>   	    (when (and (not (eq t okstatus))
>>>   		       (or (not (integerp status))
>>>   			   (and okstatus (< okstatus status))))
>>> --8<---------------cut here---------------end--------------->8---
>>> then instead of,
>>> --8<---------------cut here---------------start------------->8---
>>> Warning: (vc-not-supported "Searching for program" "No such file or directory" "git")
>>> --8<---------------cut here---------------end--------------->8---
>>> the report is,
>>> --8<---------------cut here---------------start------------->8---
>>> VC refresh error: (void-function _)
>>> --8<---------------cut here---------------end--------------->8---
>>
>> _ is syntax specific to pcase. condition-case uses 't' for the fallback handler.
>>
>>> Well, I do not know how to avoid the ’pcase’ here to correctly propagate
>>> the errors.
>>> About the other ’pcase’, this code,
>>> --8<---------------cut here---------------start------------->8---
>>>         ((setq backend (condition-case err
>>>                            (vc-backend buffer-file-name)
>>>                          ((vc-not-supported
>>>                            (message "Warning: %S" err))
>>>                           (_
>>>                            (message "VC refresh error: %S" err)))))
>>> --8<---------------cut here---------------end--------------->8---
>>> does not raise the correct message.
>>
>> Does this work for you?
>>
>> ((setq backend (condition-case err
>>                    (vc-backend buffer-file-name)
>>                  (vc-not-supported
>>                   (message "Warning: %S" err))
>>                  (t
>>                   (message "VC refresh error: %S" err))))
>>
>> And you can update the previous (more complex) example similarly, without pcase.
>>
>>> Somehow, I probably miss how to correctly propagate errors but the patch
>>> is, IMHO, the simplest way.
>>>
>>>> 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?
>>> Nothing has really changed.  The current pattern is:
>>> --8<---------------cut here---------------start------------->8---
>>>        (condition-case err
>>>            (with-temp-buffer
>>>              ...
>>>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>>>                  (vc-bzr-state file)     ; Some other unknown format?
>>>                (let* ...)))
>>>          ;; The dirstate file can't be read, or some other problem.
>>>          (error
>>>           (message "Falling back on \"slow\" status detection (%S)" err)
>>>           (vc-bzr-state file)))
>>> --8<---------------cut here---------------end--------------->8---
>>> where it means ’vc-bzr-state’ is at two places – which is redundant.
>>> Instead, the pattern,
>>> --8<---------------cut here---------------start------------->8---
>>>        (condition-case err
>>>            (with-temp-buffer
>>>              ...
>>>              (if (not (looking-at "#bazaar dirstate flat format 3"))
>>>                  (signal 'error "VC: Bzr dirstate is not flat format 3")
>>>                (let* ...)))
>>>          ;; The dirstate file can't be read, or some other problem.
>>>          (error
>>>           (message "Falling back on \"slow\" status detection (%S)" err)
>>>           (vc-bzr-state file)))
>>> --8<---------------cut here---------------end--------------->8---
>>> is better because it appears only at one location.  Now, knowing if this
>>> test about the BZR format is relevant or not is another story. :-)
>>
>> Makes sense now, thanks.
>>
>>> The patch is just tweaking how the errors are handled, not the logic
>>> behind.  In another patch, this ’vc-bzr-state-heuristic’ could be split;
>>> as it is done with HG: vc-hg-state calling vc-hg-state-fast falling back
>>> to vc-hg-state-slow – instead of having the fast (heuristic) included.
>>
>> Up to you.
>>
>>> Let me know how to proceed for helping in the review process.
>>
>> Let's address the raised points first. Thank you.

Message #59 received at 57407 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 57407 <at> debbugs.gnu.org
Subject: Re: bug#57407: closed (Re: bug#57407: [PATCH] Handle error of ’vc-registered’)
Date: Sat, 1 Mar 2025 19:38:45 -0800

Hi Simon,

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Simon Tournier <zimon.toutoune <at> gmail.com> writes:
>
>> The previous comments appear to me very good ones and they are real
>> improvements.  I do not promise to implement in timely manner but I
>> will do my best to dedicate some time before my summer holidays --
>> always the good time for rushing and cleaning the endless TODO list.
>> ;-)
>
> Sounds good.  Please let us know when you have a new patch.

Were you able to make any progress here?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.