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

[Stefan Kangas <stefankangas@gmail.com>]

That was one year ago.

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

Dmitry Gutov <dgutov@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@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.