From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Dmitry Gutov Newsgroups: gmane.emacs.bugs Subject: bug#57407: [PATCH] Handle error of =?UTF-8?Q?=E2=80=99vc-registered=E2=80=99?= Date: Fri, 30 Sep 2022 03:55:50 +0300 Message-ID: <1529983c-f487-0e27-338e-9c537d2c7169@yandex.ru> References: <87lercwb0o.fsf@gmail.com> <87czc0eqfn.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4694"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Cc: 57407@debbugs.gnu.org To: Simon Tournier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Sep 30 02:56:11 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oe4K3-0000vO-9A for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 30 Sep 2022 02:56:11 +0200 Original-Received: from localhost ([::1]:46530 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oe4K2-0005qy-CR for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 29 Sep 2022 20:56:10 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:49726) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oe4Ju-0005qq-Jf for bug-gnu-emacs@gnu.org; Thu, 29 Sep 2022 20:56:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:40876) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oe4Ju-00046r-Al for bug-gnu-emacs@gnu.org; Thu, 29 Sep 2022 20:56:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oe4Jt-0006GY-UJ for bug-gnu-emacs@gnu.org; Thu, 29 Sep 2022 20:56:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Dmitry Gutov Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 30 Sep 2022 00:56:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 57407 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch moreinfo Original-Received: via spool by 57407-submit@debbugs.gnu.org id=B57407.166449936124079 (code B ref 57407); Fri, 30 Sep 2022 00:56:01 +0000 Original-Received: (at 57407) by debbugs.gnu.org; 30 Sep 2022 00:56:01 +0000 Original-Received: from localhost ([127.0.0.1]:39954 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oe4Js-0006GJ-2C for submit@debbugs.gnu.org; Thu, 29 Sep 2022 20:56:00 -0400 Original-Received: from mail-wr1-f54.google.com ([209.85.221.54]:36607) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oe4Jq-0006G2-1Z for 57407@debbugs.gnu.org; Thu, 29 Sep 2022 20:55:58 -0400 Original-Received: by mail-wr1-f54.google.com with SMTP id v28so4558108wrd.3 for <57407@debbugs.gnu.org>; Thu, 29 Sep 2022 17:55:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :sender:from:to:cc:subject:date; bh=buaFp05eHUoF0sfxjVt+MgmidPRV/p8AhZreQuPAGI0=; b=Mzdk396GASL6L3PlsVQ69+lLieSwMMx2h3UCORqf7+nvMaKIRuxPuDUY9sM0dy5mgZ K+49H+cYLfrq2wzs1TV5O401d3kpXX/lpaVU2w5BbccZoFxiXX8cE4euIHGhBBT0u+cz GrSls5TstvPG5jQA3SeFjrOHZCfuZvZjWlF6UqLH+0X02H92vdQvzPEc+fep2/Hdf/3i eodPOLn5kzeuJunMLdvO4tMrMFXAcfxbnmvKlR/Sqq2Yd4paR1jVAyeb119zG8ta6w0v oYoC/2Fe4uNML2HYU76eYbr5LoutdxhCzQykEVwugsXRzD7qXzVoMxkealmYKCaQQtq0 oZaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :sender:x-gm-message-state:from:to:cc:subject:date; bh=buaFp05eHUoF0sfxjVt+MgmidPRV/p8AhZreQuPAGI0=; b=hYWj68KhU1I/+r0pFfCyZ7/esQlG1bcEe+RgQI8Qi0JZwbeFrlqrwn2ItFhH05Nv6F TAs3R+AdEhMJOTI06YFtw/owSRUEryGVCVh2uh3Yl5KZrky5aRa2nkmrljvE51QFcLUx LzG4naTTqjEgK5e6ZdQeTRI31TyUG8O0LVgNlNyNljyq2OZm4az9NmygCqYBWt34A4XK 3qX7TwpseUOu/7my63ey8mY4IsYskwhHghS6Ze4CaClYubu6kuROE1HA2kjC1WmR1rQu aI2KYIYdSUVklUTLHD+wGzDS9f8EBoEpCXE9oBvDZI6EWkeu/JnLBajWTvTxUwhoBfBk ia/A== X-Gm-Message-State: ACrzQf3Nc69ReM+456LyAz9JgTGFe71vW04SIVfhbFRsH+63lsWj89ww 32Vyle4ta+/qpJqnz2cEll8= X-Google-Smtp-Source: AMsMyM7Dt2JA6BzY0sTebcxAwxPE+UjwJMmFhPFxjXHxmkBhAab8e14y53G1nrRjwN8uR5JkFdgxhg== X-Received: by 2002:adf:fc0a:0:b0:22c:daaf:b179 with SMTP id i10-20020adffc0a000000b0022cdaafb179mr3085156wrr.569.1664499352064; Thu, 29 Sep 2022 17:55:52 -0700 (PDT) Original-Received: from [192.168.0.6] ([46.251.119.176]) by smtp.googlemail.com with ESMTPSA id n186-20020a1ca4c3000000b003a8434530bbsm5472770wme.13.2022.09.29.17.55.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Sep 2022 17:55:51 -0700 (PDT) Content-Language: en-US In-Reply-To: <87czc0eqfn.fsf@gmail.com> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:243966 Archived-At: On 12.09.2022 15:18, Simon Tournier wrote: > Hi, > > Thanks for your comments. > > > On lun., 12 sept. 2022 at 04:08, Dmitry Gutov 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: ” with > “Warning: ”. 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--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--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.