| Message ID | 20211020165435.3358398-1-zimon.toutoune@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | guix hash: eases conversion | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > * guix/scripts/hash.scm (guix-hash): Allow several files. > [directory?]: New procedure. > [file-hash]: Catch system-error. > [hash-to-display]: New procedure. Nice! Could you update guix.texi and tests/guix-hash.texi? > - (with-error-handling > - (if (assoc-ref opts 'recursive?) > + (if (and (assoc-ref opts 'recursive?) > + (directory? file)) This change is not related to the main purpose of the patch. More importantly, note that ‘--recursive’ is not limited to directories: it preserves file properties (directory, executable, or regular), so it’s also useful for executable files for instance. It can also be used for regular files, even if it’s less useful. > + (define (hash-to-display thing) > + (match thing > + ((? file-exists? file) > + (fmt (file-hash file))) > + ("-" (with-error-handling > + (fmt (port-hash (assoc-ref opts 'hash-algorithm) > + (current-input-port))))) I’d swap the order of the two clauses and remove the call to ‘file-exists?’: if the file doesn’t exist, ‘file-hash’ will raise an error. > + (x > + (leave (G_ "wrong argument~%"))))) > + > + (for-each > + (lambda (arg) > + (format #t "~a~%" (hash-to-display arg))) Or just (compose display hash-to-display) ? Maybe s/hash-to-display/formatted-hash/. Could you send an updated patch? Thanks, Ludo’.
Hi Ludo, On Sat, 30 Oct 2021 at 16:46, Ludovic Courtès <ludo@gnu.org> wrote: > zimoun <zimon.toutoune@gmail.com> skribis: > >> * guix/scripts/hash.scm (guix-hash): Allow several files. >> [directory?]: New procedure. >> [file-hash]: Catch system-error. >> [hash-to-display]: New procedure. > > Nice! Could you update guix.texi and tests/guix-hash.texi? I will once we agree and reach consensus. ;-) >> - (with-error-handling >> - (if (assoc-ref opts 'recursive?) >> + (if (and (assoc-ref opts 'recursive?) >> + (directory? file)) > > This change is not related to the main purpose of the patch. > > More importantly, note that ‘--recursive’ is not limited to directories: > it preserves file properties (directory, executable, or regular), so > it’s also useful for executable files for instance. It can also be used > for regular files, even if it’s less useful. I understand. Could you provide concrete examples when it is not directory? In order to see if there is a pattern. >> + (define (hash-to-display thing) >> + (match thing >> + ((? file-exists? file) >> + (fmt (file-hash file))) >> + ("-" (with-error-handling >> + (fmt (port-hash (assoc-ref opts 'hash-algorithm) >> + (current-input-port))))) > > I’d swap the order of the two clauses and remove the call to > ‘file-exists?’: if the file doesn’t exist, ‘file-hash’ will raise an > error. I can swap order but not remove file-exists? Otherwise, the next clause… >> + (x >> + (leave (G_ "wrong argument~%"))))) will never happen. And some tests are failing. >> + (for-each >> + (lambda (arg) >> + (format #t "~a~%" (hash-to-display arg))) > > Or just (compose display hash-to-display) ? > > Maybe s/hash-to-display/formatted-hash/. Thanks. Indeed both are better. :-) > Could you send an updated patch? Yes, but before, from my understanding, we should agree on the goal of such patch. :-) See the two other replies. Cheers, simon
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > On Sat, 30 Oct 2021 at 16:46, Ludovic Courtès <ludo@gnu.org> wrote: >> zimoun <zimon.toutoune@gmail.com> skribis: >> >>> * guix/scripts/hash.scm (guix-hash): Allow several files. >>> [directory?]: New procedure. >>> [file-hash]: Catch system-error. >>> [hash-to-display]: New procedure. >> >> Nice! Could you update guix.texi and tests/guix-hash.texi? > > I will once we agree and reach consensus. ;-) Apparently there’s consensus on this specific patch. >> This change is not related to the main purpose of the patch. >> >> More importantly, note that ‘--recursive’ is not limited to directories: >> it preserves file properties (directory, executable, or regular), so >> it’s also useful for executable files for instance. It can also be used >> for regular files, even if it’s less useful. > > I understand. Could you provide concrete examples when it is not > directory? In order to see if there is a pattern. See the executables used in (gnu packages bootstrap): to preserve their executable bit, you need ‘--recursive’. Same for symlinks. Anyway, this patch is about allowing for multiple arguments (which we agree on), so I think we should discuss the ‘--recursive’ issue separately. Thanks! Ludo’.
diff --git a/guix/scripts/hash.scm b/guix/scripts/hash.scm index b8622373cc..f3363549d3 100644 --- a/guix/scripts/hash.scm +++ b/guix/scripts/hash.scm @@ -3,6 +3,7 @@ ;;; Copyright © 2013 Nikita Karetnikov <nikita@karetnikov.org> ;;; Copyright © 2016 Jan Nieuwenhuizen <janneke@gnu.org> ;;; Copyright © 2018 Tim Gesthuizen <tim.gesthuizen@yahoo.de> +;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -135,6 +136,11 @@ (define (vcs-file? file stat) (else #f))) + (define (directory? file) + (case (stat:type (stat file)) + ((directory) #t) + (else #f))) + (let* ((opts (parse-options)) (args (filter-map (match-lambda (('argument . value) @@ -149,27 +155,37 @@ (define (vcs-file? file stat) (define (file-hash file) ;; Compute the hash of FILE. ;; Catch and gracefully report possible '&nar-error' conditions. - (with-error-handling - (if (assoc-ref opts 'recursive?) + (if (and (assoc-ref opts 'recursive?) + (directory? file)) + (with-error-handling (let-values (((port get-hash) (open-hash-port (assoc-ref opts 'hash-algorithm)))) (write-file file port #:select? select?) (force-output port) - (get-hash)) - (match file - ("-" (port-hash (assoc-ref opts 'hash-algorithm) - (current-input-port))) - (_ (call-with-input-file file - (cute port-hash (assoc-ref opts 'hash-algorithm) - <>))))))) - - (match args - ((file) - (catch 'system-error - (lambda () - (format #t "~a~%" (fmt (file-hash file)))) - (lambda args - (leave (G_ "~a~%") - (strerror (system-error-errno args)))))) - (x - (leave (G_ "wrong number of arguments~%")))))) + (get-hash))) + (catch 'system-error + (lambda _ + (call-with-input-file file + (cute port-hash (assoc-ref opts 'hash-algorithm) + <>))) + (lambda args + (when (directory? file) + (display-hint (G_ "Try @option{--recursive}."))) + (leave (G_ "~a ~a~%") + file + (strerror (system-error-errno args))))))) + + (define (hash-to-display thing) + (match thing + ((? file-exists? file) + (fmt (file-hash file))) + ("-" (with-error-handling + (fmt (port-hash (assoc-ref opts 'hash-algorithm) + (current-input-port))))) + (x + (leave (G_ "wrong argument~%"))))) + + (for-each + (lambda (arg) + (format #t "~a~%" (hash-to-display arg))) + args)))