From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Simon Tournier Newsgroups: gmane.emacs.bugs Subject: bug#57407: [PATCH] Handle error of =?UTF-8?Q?=E2=80=99vc-registered=E2=80=99?= Date: Mon, 12 Sep 2022 14:18:52 +0200 Message-ID: <87czc0eqfn.fsf@gmail.com> References: <87lercwb0o.fsf@gmail.com> 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="29253"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) Cc: 57407@debbugs.gnu.org To: Dmitry Gutov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Sep 12 14:21:35 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 1oXiRT-0007Tj-KQ for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 12 Sep 2022 14:21:35 +0200 Original-Received: from localhost ([::1]:54448 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oXiRS-00060t-5z for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 12 Sep 2022 08:21:34 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:48864) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oXiQ0-0005z4-1Z for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2022 08:20:06 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55796) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oXiPx-0003qL-U5 for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2022 08:20:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oXiPx-0006lL-Q6 for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2022 08:20:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Simon Tournier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 12 Sep 2022 12:20: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.166298514925931 (code B ref 57407); Mon, 12 Sep 2022 12:20:01 +0000 Original-Received: (at 57407) by debbugs.gnu.org; 12 Sep 2022 12:19:09 +0000 Original-Received: from localhost ([127.0.0.1]:44495 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oXiP6-0006kA-La for submit@debbugs.gnu.org; Mon, 12 Sep 2022 08:19:09 -0400 Original-Received: from mail-wm1-f46.google.com ([209.85.128.46]:38463) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oXiP4-0006jE-37 for 57407@debbugs.gnu.org; Mon, 12 Sep 2022 08:19:06 -0400 Original-Received: by mail-wm1-f46.google.com with SMTP id n23-20020a7bc5d7000000b003a62f19b453so10851297wmk.3 for <57407@debbugs.gnu.org>; Mon, 12 Sep 2022 05:19:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from:from:to:cc:subject :date; bh=U6gYU43BvUkBJl238H248ckP8i1ZAKZyvioFNHj4HFM=; b=bUiSRttuiEbz/KxJXweqLpQo/3Gz1qJBHhXc+8bkFa+3TZvF3Af0X1jnuaCdPaJaOj iEmDN7TVBFuTR2yRYtr45DyGwJqLtFBVShoblTzD8rb16q33ywgK2RfcfPBTPqpBbQSY NUDFb5SfXbIp+iZWRI5p3vSoVHBkintifjxlozR0vlIu0bb9loAUIH5TinyU6caNQkQI UF+fKhQ/9VRPsylA4KOpqtPQhyhZrz19r6+WvnBN5q5V3yFw0UFKHEDsaoy8ctXC4yFC 0oX+Htxjp450F5TMy7M9hFIM0eUg68RYzi0FyzF10MwoUNdBjhW2u1hl3zD3+EkwnIu0 1nDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date; bh=U6gYU43BvUkBJl238H248ckP8i1ZAKZyvioFNHj4HFM=; b=c6dQFDG9FHKyPP8dXyNsok27NZatCUlz/XC4TklJMAIKe45zovpuj/lzobNJcetgqT W7lP70NzV0k+Iqffi0GvrbT7jud9YF5gjy4aQPZ/8tUoAzDm8Uet6Vbc5Cai6UrccqyE kg1kglHCyJiyF+jChqJ1d54NMvWwNgAnS96Orfn9GR02/5gL0biWe+eam6j9OZ9y8p3z ZRGr9CaZJ45L0cjPGTb78bujYMCOVULd4a0uQK05mtXsjob/6RpUyl6j9Pdo+v6BkDM7 S3UoZpaE4AezfwVyoPqLnj6ZiBG+FBHAml5BgNlt1mC9H7WteCnazhv0KoTX3LP11A+l c9+g== X-Gm-Message-State: ACgBeo3tD/r0MWIc6lpu673BJhvwfiFTcuKqADc1nGGA1hmIyApJeyi/ QVUK9OlT4brFeonDM+WiAziAkVIePBM= X-Google-Smtp-Source: AA6agR4W4FogeIKlQv1eVfAYIsB0YDKNkEg30mRP/PaYxzI+Nz8kxxv//77Lwg8Wdgh6IR2NAsyxBA== X-Received: by 2002:a05:600c:3544:b0:3b4:6278:8b60 with SMTP id i4-20020a05600c354400b003b462788b60mr8812513wmq.188.1662985139766; Mon, 12 Sep 2022 05:18:59 -0700 (PDT) Original-Received: from pfiuh07 ([193.48.40.241]) by smtp.gmail.com with ESMTPSA id bh16-20020a05600c3d1000b003a60ff7c082sm9959997wmb.15.2022.09.12.05.18.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Sep 2022 05:18:59 -0700 (PDT) In-Reply-To: (Dmitry Gutov's message of "Mon, 12 Sep 2022 04:08:23 +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" Xref: news.gmane.io gmane.emacs.bugs:242258 Archived-At: 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 "E= rror: > ..." messages replaced with "Warning: ..."? Somehow yes. More explicitly, replace =E2=80=9CError: =E2= =80=9D with =E2=80=9CWarning: =E2=80=9D. > 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. If the heuristic fails in vc-bzr-state-heuristic, then =E2=80=99vc-bzr-stat= e=E2=80=99 is called, which itself then calls =E2=80=99vc-bzr-status=E2=80=99. This stat= us 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 ca= ught, > 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 =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 catc= h. 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 fir= st argument to =E2=80=98condition-case=E2=80=99 whose error condition you want to = re-throw. Or I am missing a point. :-) > 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 ha= s, > 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. > 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-sta= te > 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 guard > 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? Fr= om 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 is n= ot modified by this patch and the handler happens in =E2=80=99vc-refresh-state=E2=80=99. 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 =E2=80=99vc-svn-working-revision=E2=80=99 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, =E2=80=99vc-svn-working-revision=E2=80=99 should also be guarded= with =E2=80=99condition-case=E2=80=99 and display an appropriate message, indeed. >> 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 'error'= 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 sp= ecific 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=3D (cadr err) "Searching for program") - ;; The most probable is the lack of the backend bi= nary. - (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 bina= ry. + (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 directo= ry" "git") --8<---------------cut here---------------end--------------->8--- the report is, --8<---------------cut here---------------start------------->8--- VC refresh error: (void-function _) --8<---------------cut here---------------end--------------->8--- Well, I do not know how to avoid the =E2=80=99pcase=E2=80=99 here to correc= tly 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. 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 th= is 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 wh= ich 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. :-) 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 co= uld 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) includ= ed. Let me know how to proceed for helping in the review process. All the best, simon