[bug#50072] [PATCH WIP 0/4] Add upstream updater for git-fetch origins.

[zimoun]

Hi Maxime,

Thanks!  All LGTM and I have two naive remarks.


On Tue, 04 Jan 2022 at 20:06, Maxime Devos <maximedevos@telenet.be> wrote:

> diff --git a/guix/hash.scm b/guix/hash.scm

[...]

> +(define-module (guix hash)
> +  #:use-module (gcrypt hash)
> +  #:use-module (guix serialization)
> +  #:use-module (srfi srfi-1)
> +  #:use-module (srfi srfi-11)
> +  #:export (vcs-file?
> +            file-hash*))
> +
> +(define (vcs-file? file stat)
> +  "Returns true if FILE is a version control system file."
> +  (case (stat:type stat)
> +    ((directory)
> +     (member (basename file) '(".bzr" ".git" ".hg" ".svn" "CVS")))
> +    ((regular)
> +     ;; Git sub-modules have a '.git' file that is a regular text file.
> +     (string=? (basename file) ".git"))
> +    (else
> +     #f)))

1) Why ’vcs-file?’ requires to be exported?  Is it used elsewhere?


> +(define* (file-hash* file #:key
> +                     (algorithm (hash-algorithm sha256))
> +                     (recursive? 'auto)

2) ’auto’ is confusing…

> +                     (select? (negate vcs-file?)))
> +  "Compute the hash of FILE with ALGORITHM.  If RECURSIVE? is #true or 
> 'auto',
> +recurse into subdirectories of FILE, computing the combined hash (nar hash) 
> of

…here I understand that ’auto’ means #true…

> +Keep in mind that the hash of a regular file depends on RECURSIVE?:
> +if the recursive hash is desired, it must be set to #true.  Otherwise, it 
> must
> +be set to #false or 'auto'. In most situations, the non-recursive hash is 
> desired
> +for regular files."

…but there it is the contrary. :-)  To me, #true/#false or #t/#f are
meaningful, especially when…

> +  (if (or (eq? recursive? #true)
> +          (and (eq? recursive? 'auto)

…the symbol ’auto’ is only used here.  IIRC all the series. :-)


(I know Ricardo is for instance in favor of #true/#false compared to
#t/#f.  I have an opinion but I would like to avoid another
bikeshed. ;-))


Cheers,
simon