[PATCH] git-version: Handle invalid arguments gracefully

Jakub Kądziołka wrote on 15 Apr 2020 17:18

Recipients:(address . guix-patches@gnu.org)

Message-ID:20200415151824.22988-1-kuba@kadziolka.net

* guix/git-download.scm (git-version): Add a check for commit ID length.---If you're curious for the motivation, see [1]. This took a while todebug, so I'm hoping to ease this for the next person who inevitablystumbles upon this. Is a change like this okay?
[1]: http://logs.guix.gnu.org/guix/2020-04-14.log#162809
guix/git-download.scm | 9 +++++++++ 1 file changed, 9 insertions(+)

Toggle diff (36 lines)diff --git a/guix/git-download.scm b/guix/git-download.scmindex 1eae035fc4..40d81c72b9 100644--- a/guix/git-download.scm+++ b/guix/git-download.scm@@ -2,6 +2,7 @@ ;;; Copyright © 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2017 Mathieu Lirzin <mthl@gnu.org> ;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net> ;;; ;;; This file is part of GNU Guix. ;;;@@ -30,6 +31,7 @@   #:use-module (ice-9 match)   #:use-module (ice-9 vlist)   #:use-module (srfi srfi-1)+  #:use-module (srfi srfi-35)   #:export (git-reference             git-reference?             git-reference-url@@ -170,6 +172,13 @@ HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."  (define (git-version version revision commit)   "Return the version string for packages using git-download."+  ;; git-version is almost exclusively executed while modules are being loaded.+  ;; This makes any errors hide their backtrace. Avoid the mysterious error+  ;; "Value out of range 0 to N: 7" when the commit ID is too short, which+  ;; can happen, for example, when the user swapped the revision and commit+  ;; arguments by mistake.+  (when (< (string-length commit) 7)+    (error "git-version: commit ID unexpectedly short"))   (string-append version "-" revision "." (string-take commit 7)))  (define (git-file-name name version)-- 2.26.0

Ludovic Courtès wrote on 17 Apr 2020 23:16

Recipients:(name . Jakub Kądziołka)(address . kuba@kadziolka.net)(address . 40643@debbugs.gnu.org)

Message-ID:87v9lx95j7.fsf@gnu.org

Hi Jakub,
Jakub Kądziołka <kuba@kadziolka.net> skribis:

Toggle quote (6 lines)

> * guix/git-download.scm (git-version): Add a check for commit ID length.> ---> If you're curious for the motivation, see [1]. This took a while to> debug, so I'm hoping to ease this for the next person who inevitably> stumbles upon this. Is a change like this okay?

Yes, I think so.  The ‘error’ procedure is not great, we would ratheruse ‘raise’ with a ‘&message’ condition (which additionally allows fori18n) but it’s no big deal here.
Thanks,Ludo’.

Jakub Kądziołka wrote on 22 Apr 2020 00:45

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40643@debbugs.gnu.org)

Message-ID:20200421224545.ztr7beu3mqaufyo3@gravity

On Fri, Apr 17, 2020 at 11:16:44PM +0200, Ludovic Courtès wrote:

Toggle quote (14 lines)> Hi Jakub,> > Jakub Kądziołka <kuba@kadziolka.net> skribis:> > > * guix/git-download.scm (git-version): Add a check for commit ID length.> > ---> > If you're curious for the motivation, see [1]. This took a while to> > debug, so I'm hoping to ease this for the next person who inevitably> > stumbles upon this. Is a change like this okay?> > Yes, I think so.  The ‘error’ procedure is not great, we would rather> use ‘raise’ with a ‘&message’ condition (which additionally allows for> i18n) but it’s no big deal here.

I considered using raise instead, but I couldn't get it to workproperly. I was getting a "Wrong type (expecting exact integer)" errorinstead:
(raise (condition (&message (message "git-version: commit ID unexpectedly short"))))
Do you know why that might be?
Regards,Jakub Kądziołka

Ludovic Courtès wrote on 22 Apr 2020 17:10

Recipients:(name . Jakub Kądziołka)(address . kuba@kadziolka.net)(address . 40643@debbugs.gnu.org)

Message-ID:87pnbzwo85.fsf@gnu.org

Hi,
Jakub Kądziołka <kuba@kadziolka.net> skribis:

Toggle quote (25 lines)> On Fri, Apr 17, 2020 at 11:16:44PM +0200, Ludovic Courtès wrote:>> Hi Jakub,>> >> Jakub Kądziołka <kuba@kadziolka.net> skribis:>> >> > * guix/git-download.scm (git-version): Add a check for commit ID length.>> > --->> > If you're curious for the motivation, see [1]. This took a while to>> > debug, so I'm hoping to ease this for the next person who inevitably>> > stumbles upon this. Is a change like this okay?>> >> Yes, I think so.  The ‘error’ procedure is not great, we would rather>> use ‘raise’ with a ‘&message’ condition (which additionally allows for>> i18n) but it’s no big deal here.>> I considered using raise instead, but I couldn't get it to work> properly. I was getting a "Wrong type (expecting exact integer)" error> instead:>> (raise>   (condition (&message>     (message "git-version: commit ID unexpectedly short"))))>> Do you know why that might be?

It must be because you forgot to include (srfi srfi-34): there are two‘raise’ procedure, and in core Guile ‘raise’ is about signals, not errorconditions.
HTH!
Ludo’.

Jakub Kądziołka wrote on 23 Apr 2020 15:32

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40643-done@debbugs.gnu.org)

Message-ID:20200423133201.7cif3mwfdotazedt@gravity

On Wed, Apr 22, 2020 at 05:10:18PM +0200, Ludovic Courtès wrote:

Toggle quote (4 lines)

> It must be because you forgot to include (srfi srfi-34): there are two> ‘raise’ procedure, and in core Guile ‘raise’ is about signals, not error> conditions.

Thanks! That did it! I pushed an updated patch.
Regards,Jakub Kądziołka

Closed

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date