From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.bugs Subject: bug#57407: [PATCH] Handle error of =?UTF-8?Q?=E2=80=99vc-registered=E2=80=99?= Date: Wed, 6 Sep 2023 15:48:45 -0700 Message-ID: References: <87lercwb0o.fsf@gmail.com> <87czc0eqfn.fsf@gmail.com> <1529983c-f487-0e27-338e-9c537d2c7169@yandex.ru> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39236"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 57407@debbugs.gnu.org, Simon Tournier To: Dmitry Gutov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Sep 07 00:50:23 2023 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 1qe1Lr-000A0n-6r for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 07 Sep 2023 00:50:23 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qe1Ld-0002Vo-6N; Wed, 06 Sep 2023 18:50:16 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qe1LV-0002VR-Aj for bug-gnu-emacs@gnu.org; Wed, 06 Sep 2023 18:50:01 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qe1LV-0003zk-12 for bug-gnu-emacs@gnu.org; Wed, 06 Sep 2023 18:50:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qe1LW-0001sG-8p for bug-gnu-emacs@gnu.org; Wed, 06 Sep 2023 18:50:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 06 Sep 2023 22:50:02 +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.16940405507140 (code B ref 57407); Wed, 06 Sep 2023 22:50:02 +0000 Original-Received: (at 57407) by debbugs.gnu.org; 6 Sep 2023 22:49:10 +0000 Original-Received: from localhost ([127.0.0.1]:38093 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qe1KS-0001qZ-Ly for submit@debbugs.gnu.org; Wed, 06 Sep 2023 18:49:10 -0400 Original-Received: from mail-lf1-x131.google.com ([2a00:1450:4864:20::131]:52522) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qe1KP-0001qG-Sa for 57407@debbugs.gnu.org; Wed, 06 Sep 2023 18:48:54 -0400 Original-Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-501bd6f7d11so483886e87.1 for <57407@debbugs.gnu.org>; Wed, 06 Sep 2023 15:48:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694040526; x=1694645326; darn=debbugs.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date :mime-version:references:in-reply-to:from:from:to:cc:subject:date :message-id:reply-to; bh=VJozwHPzxAB1y/rfZ48hsPEeRKQ3zAMlYKLMslvaMI0=; b=MFtamAozJNJWq9u/d3Wk57YICIu82twCcRU+j5v19Qe5Xue3ESg2ztXT09LWUgh1d2 A0WjE3O3Vk6Pn1GOQxIikCguX+9BBDiR750M9basdwwrf86GQ+TqDHWPAFco4UsDV+OB U8z6uhqLeRmQ+2VhYM7eO3+/GpKLPOOjI6DEAkH443UG2zbjTzs8TKeaIRORWuV5RnZH zGjZuL0zAx+iPE1wEzdOGRLw2ftFJIYnNqivNo7MMhlaI5jXoLEa87NMVd/+cbVPQzuO pz6f5qTLInlkOdIf1wQMJn7ILRz1gAEf0c4d/Q5iampi8HPS05o0YTkd/s+6URlaoGbv HwIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1694040526; x=1694645326; h=content-transfer-encoding:cc:to:subject:message-id:date :mime-version:references:in-reply-to:from:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=VJozwHPzxAB1y/rfZ48hsPEeRKQ3zAMlYKLMslvaMI0=; b=P5KvwJ6ztVkoMz/JEW7LBX55XI4CqhIeMdyji1NeHDohKG83Zf4xnir4P2kbjDJ/4l WJmPCsPsdg4JgShB50TEwWniLdaqs737T3+JMFWZ4W4VTS3MR+aUXjueBybK+lbpEjSn rwgI5iMLMP203LQkBrdczus8SoKEi4SJCajjlesBYjfd6d+f8LNbiASbzVpULxuNGkSq i8SR1ATDLQyZ3rRxv17MXV+Y3yxJHS23p4Cjw3qHdcMEF1OHRXUrDCoYPoGD7+owTGcl qknDzm6fcxwWlnjlYl/i61srwAZCbB8Of7z+wTX9ekfH+BonQBX+RUL0ojtwuZk0k0vK mRFQ== X-Gm-Message-State: AOJu0YzSqOvlpFnMzNmGlA++9M/u7TSxwYhWWaue4qLVqE3c9e6Rhy42 wmivYqPaIaO25acfMOe9wszE+G3s4t11LWOJQxs= X-Google-Smtp-Source: AGHT+IF36Savezya2yIJVcVb169YHBeVFrfj4GTUJ0ktWHe48GFW3FxS1wzMwx82DpihCDMbyHCjVFWK8g7zUAWsRJo= X-Received: by 2002:ac2:5326:0:b0:500:95f7:c416 with SMTP id f6-20020ac25326000000b0050095f7c416mr3063792lfh.7.1694040526193; Wed, 06 Sep 2023 15:48:46 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Wed, 6 Sep 2023 15:48:45 -0700 In-Reply-To: <1529983c-f487-0e27-338e-9c537d2c7169@yandex.ru> (Dmitry Gutov's message of "Fri, 30 Sep 2022 03:55:50 +0300") 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:269594 Archived-At: That was one year ago. Simon, did you have a chance to look into the issues that Dmitry mentioned below? Dmitry Gutov 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 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 =E2=80=9CError: = =E2=80=9D with >> =E2=80=9CWarning: =E2=80=9D. > > 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-comman= d), but > adding a condition-case with exactly one handler which simply re-throws d= oesn't > seem to change anything (except swallowing a part of the backtrace). > >> If the heuristic fails in vc-bzr-state-heuristic, then =E2=80=99vc-bzr-s= tate=E2=80=99 is >> called, which itself then calls =E2=80=99vc-bzr-status=E2=80=99. This s= tatus 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, th= en? >> Yes, you need to re-throw the error to propagate it. See >> =E2=80=99(elisp)Handling Errors=E2=80=99: >> Sometimes it is necessary to re-throw a signal caught by >> =E2=80=98condition-case=E2=80=99, for some outer-level handler to = catch. Here=E2=80=99s >> how to do that: >> (signal (car err) (cdr err)) >> where =E2=80=98err=E2=80=99 is the error description variable, the= first argument >> to =E2=80=98condition-case=E2=80=99 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 use= ful 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 w= ay 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 ju= st > 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 nev= er >>> 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 >> =E2=80=99vc--registered=E2=80=99 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-s= tate >>> wrapped its call in with-demoted-errors anyway. >> For what my opinion is worth, the commit >> 991e145450ec8b02865597bc80fd797e39e81f07 =E2=80=93 which replaces >> =E2=80=99ignore-errors=E2=80=99 with =E2=80=99with-demoted-errors=E2=80= =99 only for the Git backend =E2=80=93 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 gu= ard >>> with with-demoted-errors or condition-case. What do you think we should= do >>> with them? >> Could you be more specific about these =E2=80=9Cother callers=E2=80=9D? = From my >> understanding, =E2=80=99vc--registered=E2=80=99 is called by = =E2=80=99vc-registered=E2=80=99 >> and this patch handles this case; even =E2=80=99vc-registered=E2=80=99 i= s not modified >> by this patch and the handler happens in =E2=80=99vc-refresh-state=E2=80= =99. > > vc-registered and vc-dir-recompute-file-state call it. > > And vc-deduce-fileset-1 calls vc-registered. I suppose some or all of the= se > should catch errors now? > >> Moreover, >> --8<---------------cut here---------------start------------->8--- >> $ for backend in svn git hg ;do ag --elisp vc-${backend}-registered ;don= e >> 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 startu= p, 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 =E2=80=99vc-svn-working-revision=E2=80=99 u= sed 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, =E2=80=99vc-svn-working-revision=E2=80=99 should also be guar= ded with >> =E2=80=99condition-case=E2=80=99 and display an appropriate message, ind= eed. >> >>>> It is probably an abuse of =E2=80=99pcase=E2=80=99. Is =E2=80=99cond= =E2=80=99 better here? Last, >>>> I have not found in the documentation how to differentiate what it is >>>> raised depending on the error type, hence the =E2=80=99pcase=E2=80=99. >>> >>> I think you just need to write the specific error type instead of 'erro= r' in >>> the handler clause. >> Maybe I am missing a point about error handling and how they propagate. >> If I consider this change removing =E2=80=99pcase=E2=80=99 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 squeez= ed)) >> - (error >> - (pcase (car err) >> - ('file-missing >> - (if (string=3D (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=3D (cadr err) "Searching for program") >> + ;; The most probable is the lack of the backend b= inary. >> + (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 dire= ctory" "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 h= andler. > >> Well, I do not know how to avoid the =E2=80=99pcase=E2=80=99 here to cor= rectly propagate >> the errors. >> About the other =E2=80=99pcase=E2=80=99, 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 f= all >>> 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 =E2=80=99vc-bzr-state=E2=80=99 is at two places =E2=80=93= 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 =E2=80=99vc-bzr-state-heuristic=E2=80=99= could be split; >> as it is done with HG: vc-hg-state calling vc-hg-state-fast falling back >> to vc-hg-state-slow =E2=80=93 instead of having the fast (heuristic) inc= luded. > > Up to you. > >> Let me know how to proceed for helping in the review process. > > Let's address the raised points first. Thank you.