[PATCH] git-version: Handle invalid arguments gracefully

DoneSubmitted by Jakub Kądziołka.
Details
2 participants
  • Jakub Kądziołka
  • Ludovic Courtès
Owner
unassigned
Severity
normal
J
J
Jakub Kądziołka wrote on 15 Apr 2020 17:18
Download
(address . guix-patches@gnu.org)
20200415151824.22988-1-kuba@kadziolka.net
Download
* 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?

[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.scm
index 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
L
L
Ludovic Courtès wrote on 17 Apr 2020 23:16
Download
(name . Jakub Kądziołka)(address . kuba@kadziolka.net)(address . 40643@debbugs.gnu.org)
87v9lx95j7.fsf@gnu.org
Download
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 rather
use ‘raise’ with a ‘&message’ condition (which additionally allows for
i18n) but it’s no big deal here.

Thanks,
Ludo’.
J
J
Jakub Kądziołka wrote on 22 Apr 2020 00:45
Download
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40643@debbugs.gnu.org)
20200421224545.ztr7beu3mqaufyo3@gravity
Download
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 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?

Regards,
Jakub Kądziołka
Download
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE5Xa/ss9usT31cTO54xWnWEYTFWQFAl6fd5MACgkQ4xWnWEYT
FWRU1BAAjPJ6kRmZHq80VzCFfpao+YGAcQoOo/jX4RrYl9SOwvMfTKTV1rf1jv/2
7QV4Z/q6jg7xPcBkuPL2M+BZB8P3ELlRnTQPZcPhojYEb7R4u+C+7r3otSb8L26V
sNEHXCjbUbgHdKMBgAU48oAHtT6cFgFOo5h7Lnkz6JwIUU1K65VOAuh7O30UI9bm
dfgNnIUh0RRT8jLSkcUqsCC90XdwEs/jxTihIzMwoFIWRBdWj17lgUtoQWSQjd00
pX2QidfKaYr7fxJgY7qiG/0hTFwLBkpduIRmFB48Btoc+eeEEWAvmcV4ShBz2pjE
1RQQsP3u73x0SOOAWKBjegA3xXmtbZwzVrmJgXYeyXFxukGMhYOIpSEQDQYQE45j
u6A6mE3NbzaPUJGtaA6xOBr4P7YOccg35C2ZsdtKHNopRDaLgcKnol8vaitADsNB
u8mO3F1V32TpGXovtifZz992INEzuym2zR/SrgAC3OS9DbKQpbVLqA78PxfIJDT0
QEuXF5oAdpDzvyLkZPpTxKd2hIECTM37r/FPl8TH5FFzLlpAUZFXJAeXFilqrbJD
im/Te1lQ0bTPpYmSIXBdcat3ZdBnjYAC5WzwYuSkhPl3w7iaj168MT243e8umH5X
HT1Q+/nZlfA2mkLadpA+rxRTxvO1PLbz0B2HAOxSSLG2upH7OA8=
=81vp
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 22 Apr 2020 17:10
Download
(name . Jakub Kądziołka)(address . kuba@kadziolka.net)(address . 40643@debbugs.gnu.org)
87pnbzwo85.fsf@gnu.org
Download
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 error
conditions.

HTH!

Ludo’.
J
J
Jakub Kądziołka wrote on 23 Apr 2020 15:32
Download
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40643-done@debbugs.gnu.org)
20200423133201.7cif3mwfdotazedt@gravity
Download
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
Download
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE5Xa/ss9usT31cTO54xWnWEYTFWQFAl6hmM0ACgkQ4xWnWEYT
FWQsBA//TB4DOC0ZBqNdfhdS1Z0T8U0AfgJP40NhPwNMoEIpt3CE+rPvuOY5weAq
PpJkQraWo0icPYLvvxqwT9s1b7AVqhrkMm+EVjhY3ggmBvl4dzT3NAS5M84qBQQ8
3xcPTClVo5sHFeo/6Q7cR3SevA/fNR764QKlQN++ie4bW+k48snDy+ZnB2qIzp9x
HpUFuWtN4ljV17Fw7sLSQIZ+vzBL/p/FL67dntvOOtqFn2lD5k1qZJ7xh7yHI8iw
TZnBXsTNAqGmpaR8xlZzyoXTgZt0PylzLPFODFK4oTK4+R3Dqb3mamTI6zk6G1wx
SjwVTUypqsIvFYpAHyeFfThFO2CxOHNIpfv/DaquH1Suiksf6+9098AUZFdoDvB4
zBugRWjOA/zt0iS7iRFtKKHV7Q3vsqirrcy6QvD9wwiYVIaClbK4Gz0qM5yag27+
XLkqSbO3D4Tps46ewvEQomVh/6yqCodyhZg3dxikUUAkN5s3t5rA0H3ta6RpW1Oc
5AfdbV5Mg/YXh8Ur9eBS11PRzGB54owxzClg3d7EsZ0uJwrZGWGKvNF6Y8vjoQb4
/hXDS5p5n6wkAN/TuftbjvHAKwmHetyT35UvIsZXNBjHSGfYQLCbbGcMDXEbO8sD
jXqek54w2vIgOvy4lZ1CKjJA0A5kygqNxNDi+G3t0y+AWPRZNLQ=
=3eo/
-----END PGP SIGNATURE-----


Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 40643@debbugs.gnu.org