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

[Dmitry Gutov <dgutov@yandex.ru>]

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