diff mbox series

[bug#51307,1/2] scripts: hash: Improve error handling.

Message ID 20211020165435.3358398-1-zimon.toutoune@gmail.com
State New
Headers show
Series guix hash: eases conversion | expand

Checks

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

Commit Message

zimoun Oct. 20, 2021, 4:54 p.m. UTC
* guix/scripts/hash.scm (guix-hash): Allow several files.
[directory?]: New procedure.
[file-hash]: Catch system-error.
[hash-to-display]: New procedure.
---
 guix/scripts/hash.scm | 56 +++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 20 deletions(-)

Comments

Ludovic Courtès Oct. 30, 2021, 2:46 p.m. UTC | #1
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’.
zimoun Oct. 30, 2021, 3:40 p.m. UTC | #2
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
Ludovic Courtès Oct. 31, 2021, 1:43 p.m. UTC | #3
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 mbox series

Patch

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)))