[bug#51307] [PATCH 0/2] guix hash: eases conversion

[zimoun <zimon.toutoune@gmail.com>]

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