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 #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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.