Subject: Re: [bug#73833] [PATCH v2 1/5] guix: import: utils: Add function git->origin.

From: Ludovic Courtès <[email-masked]>
Date: Wed, 06 Nov 2024 16:16:18 +0100

Hi,

Nice patch series!

Nicolas Graves <[email-masked]> skribis:

> * guix/import/utils.scm: (git->origin): Add function.
>
> * guix/import/elpa.scm
> (download-git-repository): Remove function download-git-repository.
> (git-repository->origin): Remove function git-repository->origin.
> (ref): Add function ref.
> (melpa-recipe->origin): Use functions git->origin and ref.
>
> * guix/import/go.scm
> (git-checkout-hash): Remove function git-checkout-hash.
> (transform-version): Add function transform-version.
> (vcs->origin): Use functions git->origin and transform-version. Add
> optional argument transform-version.
>
> * tests/import/go.scm
> (go-module->guix-package): Adapt test case to changes in guix/import/go.scm.
>
> * guix/import/minetest.scm
> (download-git-repository): Remove function download-git-repository.
> (make-minetest-sexp): Use function git->origin.
>
> * tests/minetest.scm
> (make-package-sexp): Use function git->origin.
> (example-package): Adapt test-case to git->origin.
>
> * guix/import/composer.scm
> (make-php-sexp): Use function git->origin.
>
> Change-Id: Ied05a63bdd60fbafe26fbbb4e115ff6f0bb9db3c

[…]

> +(define (ref recipe)
> +  "Create REF from MELPA RECIPE."

More like: "Return a value suitable for the 'update-cached-checkout'
#:ref argument corresponding to RECIPE, a MELPA recipe alist."

> +;; This is done because the version field of the package, which the generated
> +;; quoted expression refers to, has been stripped of any 'v' prefixed.
> +(define (transform-version version)

Please add a docstring.

> -  ;; Use a custom cache to avoid cluttering the default one under
> -  ;; ~/.cache/guix, but choose one under /tmp so that it's persistent across
> -  ;; subsequent "guix import" invocations.
> -  (mkdir-p cache)
> -  (chmod cache #o700)
> -  (let-values (((checkout commit _)
> -                (parameterize ((%repository-cache-directory cache))
> -                  (update-cached-checkout url

Looks like this bit and its rationale in (guix import go) gets lost
here: ‘git->origin’ unconditionally uses ~/.cache, which means that
‘guix import go -r …’ would fill that directory.

Could we restore that behavior, probably as an option to ‘git->origin’?

> +control system is being used. Optionally use the function TRANSFORM-VERSION
> +which takes version as an input."

Two spaces after end-of-sentence period please.  :-)

“Call TRANSFORM-VERSION with VERSION as an argument to compute the
downstream version number.”

You can have #:key (transform-version identity) and call it
unconditionally (that is, it would always be a procedure, never #f).

> +(define* (git->origin repo-url ref #:key (ref->commit #f))
> +  "Returns a generated `origin' block of a package depending on the git source
> +control system, and the directory in the store where the package has been
> +downloaded, in case further processing is necessary.  REPO-URL or REF can be
> +null. REF->COMMIT can be a function or #t, in which case the commit matching
> +ref is used. If REF->COMMIT is not used, the value inside REF is used."

s/function/procedure/

> +  (let* ((version (and (pair? ref) (cdr ref)))
> +         (directory commit
> +                    (if version
> +                        (with-store store
> +                          (latest-repository-commit store repo-url
> +                                                    #:ref (if version ref '())))
> +                        (values #f #f)))
> +         (vcommit (match ref->commit
> +                    (#t    commit)
> +                    (#f    version)
> +                    ((? procedure?) (ref->commit version))
> +                    (_     #f))))

Weird semantics for ‘ref->commit’.  Could it not always be a procedure?

Also, s/vcommit/commit-string/ or something.  See the coding style
regarding identifiers (info "(guix) Formatting Code").

Likewise, could you remove uses of car/cdr in this patch set, at least
for new code (info "(guix) Data Types and Pattern Matching")?

Ludo’.