[bug#63647,v3,2/3] guix: import: go: Catch Git error.
Commit Message
* guix/import/go.scm (git-checkout-hash): Catch Git error.
Reported-by: Felix Lechner <felix.lechner@lease-up.com>.
---
guix/import/go.scm | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
Comments
Simon Tournier <zimon.toutoune@gmail.com> skribis:
> * guix/import/go.scm (git-checkout-hash): Catch Git error.
[...]
> + (catch 'git-error
> + (lambda _
> + (let-values (((checkout commit _)
> + (parameterize ((%repository-cache-directory cache))
> + (update-cached-checkout url
> + #:ref
> + `(tag-or-commit . ,reference)))))
> + (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
> + (lambda (key error . rest)
> + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url)
> + (nix-base32-string->bytevector
> + "0000000000000000000000000000000000000000000000000000"))))
I’d rather let the exception through. How about adding
‘with-git-error-handling’ at the UI level, in (guix scripts import go)?
Thanks,
Ludo’.
Hi Ludo,
On Mon, 05 Jun 2023 at 14:45, Ludovic Courtès <ludo@gnu.org> wrote:
> Simon Tournier <zimon.toutoune@gmail.com> skribis:
>
>> * guix/import/go.scm (git-checkout-hash): Catch Git error.
>
> [...]
>
>> + (catch 'git-error
>> + (lambda _
>> + (let-values (((checkout commit _)
>> + (parameterize ((%repository-cache-directory cache))
>> + (update-cached-checkout url
>> + #:ref
>> + `(tag-or-commit . ,reference)))))
>> + (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
>> + (lambda (key error . rest)
>> + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url)
>> + (nix-base32-string->bytevector
>> + "0000000000000000000000000000000000000000000000000000"))))
>
> I’d rather let the exception through. How about adding
> ‘with-git-error-handling’ at the UI level, in (guix scripts import go)?
What do you mean by “let the exception through”? It seems better to be
non-blocking and thus catch the exception then raise a meaningful
warning; it’s required when running with the option recursive.
Well, maybe an improvement could be in the addition of some
’report-git-warning’ and/or ’with-git-error-handling*’, in (guix git);
hum, I do not know.
Last, considering that the module (guix import go) already contains 4 UI
messages (G_), trying to move this warning about Git to (guix scripts
import go) will add some complexity – re-raise the exception 2 or 3
times, IIUC – and thus it will not change much about the UI, IMHO. I
mean, such move should be for all the messages or nothing.
Cheers,
simon
Hello!
Simon Tournier <zimon.toutoune@gmail.com> skribis:
> On Mon, 05 Jun 2023 at 14:45, Ludovic Courtès <ludo@gnu.org> wrote:
>> Simon Tournier <zimon.toutoune@gmail.com> skribis:
>>
>>> * guix/import/go.scm (git-checkout-hash): Catch Git error.
>>
>> [...]
>>
>>> + (catch 'git-error
>>> + (lambda _
>>> + (let-values (((checkout commit _)
>>> + (parameterize ((%repository-cache-directory cache))
>>> + (update-cached-checkout url
>>> + #:ref
>>> + `(tag-or-commit . ,reference)))))
>>> + (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
>>> + (lambda (key error . rest)
>>> + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url)
>>> + (nix-base32-string->bytevector
>>> + "0000000000000000000000000000000000000000000000000000"))))
>>
>> I’d rather let the exception through. How about adding
>> ‘with-git-error-handling’ at the UI level, in (guix scripts import go)?
>
> What do you mean by “let the exception through”? It seems better to be
> non-blocking and thus catch the exception then raise a meaningful
> warning; it’s required when running with the option recursive.
I thought it would be more appropriate to error out as soon as we have a
Git problem, rather than print a warning and emit an incorrect hash.
I understand that, when using ‘--recursive’, this means aborting the
whole process without producing anything. But maybe that’s better that
producing an incorrect (origin …) form?
Now, I don’t use ‘guix import go -r’ so it’s possible that I don’t
understand the scenario being considered here!
Thanks,
Ludo’.
@@ -6,7 +6,7 @@
;;; Copyright © 2021-2022 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
-;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2021, 2023 Simon Tournier <zimon.toutoune@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -36,9 +36,11 @@ (define-module (guix import go)
#:use-module (guix memoization)
#:use-module (guix utils)
#:autoload (htmlprag) (html->sxml) ;from Guile-Lib
- #:autoload (guix base32) (bytevector->nix-base32-string)
+ #:autoload (guix base32) (bytevector->nix-base32-string nix-base32-string->bytevector)
#:autoload (guix build utils) (mkdir-p)
+ #:autoload (guix ui) (warning)
#:autoload (gcrypt hash) (hash-algorithm sha256)
+ #:autoload (git structs) (git-error-message)
#:use-module (ice-9 format)
#:use-module (ice-9 match)
#:use-module (ice-9 peg)
@@ -507,12 +509,18 @@ (define* (git-checkout-hash url reference algorithm)
;; subsequent "guix import" invocations.
(mkdir-p cache)
(chmod cache #o700)
- (let-values (((checkout commit _)
- (parameterize ((%repository-cache-directory cache))
- (update-cached-checkout url
- #:ref
- `(tag-or-commit . ,reference)))))
- (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
+ (catch 'git-error
+ (lambda _
+ (let-values (((checkout commit _)
+ (parameterize ((%repository-cache-directory cache))
+ (update-cached-checkout url
+ #:ref
+ `(tag-or-commit . ,reference)))))
+ (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
+ (lambda (key error . rest)
+ (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url)
+ (nix-base32-string->bytevector
+ "0000000000000000000000000000000000000000000000000000"))))
(define (vcs->origin vcs-type vcs-repo-url version)
"Generate the `origin' block of a package depending on what type of source