Package: emacs;
Reported by: Simon Tournier <zimon.toutoune <at> gmail.com>
Date: Thu, 25 Aug 2022 16:21:02 UTC
Severity: normal
Tags: moreinfo
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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.