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