[bug#69780] [PATCH 4/4] DRAFT news: Add entry for ‘guix git authenticate’ changes.

[Skyler Ferris via Guix-patches via <guix-patches@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 4252 bytes --]

Hello Ludo’,
This looks like a great way to improve protection for guix developers. Saving the input, which still initially comes from the developer, and using it automatically in the future means less overhead for developers and less chances for something to go wrong in the process. I'm trying it locally on my machine and there are a few things I noticed.

> +  (if (or (file-exists? pre-push-hook)
> +          (file-exists? fpost-checkout-hook))
> +      (begin
> +        (warning (G_ "not overriding pre-existing hooks '~a' and '~a'~%")
> +                 pre-push-hook post-checkout-hook)
> +        (display-hint (G_ "Consider running @command{guix git authenticate}
> +from your pre-push and update hooks so your repository is automatically
> +authenticated before you push or receive updates.")))

When the developer builds guix a pre-push hook is already installed, from etc/git/pre-push. This runs `make authenticate` itself but also runs `make check-channel-news`. I don't think we can just get rid of that file because then people would lose check-channel-news, but it seems useful to have this functionality built into the authenticate script so that other projects can use it. Unfortunately, unconditionally appending to the script could cause problems because the developer could have added their own hook which could have been written in any language. Perhaps we could update etc/git/pre-push to call the authenticate script in the new way and install any hooks that do not clobber (eg, if pre-push exists then we skip that hook but install the rest)?

> +  (define post-checkout-hook
> +    (in-vicinity directory "hooks/post-checkout"))

The post-checkout hook does not run when `git pull` is called. Instead, the post-merge hook is called. Note that post-merge does not receive the same set of arguments as post-checkout. I had success replacing "$newrev" with "$(git rev-parse HEAD)". We could leave out the value completely for post-merge because the script will use HEAD by default if no end is given. But maybe we don't want to rely on default behavior for a script that will not be automatically updated with the tool.

I can think of more ways that a developer could end up on a new commit. For example, running `git fetch` followed by `git reset --hard`. I'm not sure if it makes to support every possible case because that could get complicated very quickly. But git pull is the most common way to get updates (at least in my experience, which could have a sample bias) so I think it makes sense to at least support that.

> +while read local_ref local_oid remote_ref remote_oid
> +do
> +  guix git authenticate --end=\"$local_ref\"
> +done\n")

I believe this should be "$local_oid", not "$local_ref".

> +(define (configured-introduction repository)
> +  "Return two values: the commit and signer fingerprint (strings) as
> +configured in REPOSITORY.  Error out if one or both were missing."
> +  (let* ((config (repository-config repository))
> +         (commit (config-value config "guix.authentication.introduction-commit"))
> +         (signer (config-value config "guix.authentication.introduction-signer")))
> +    (unless (and commit signer)
> +      (leave (G_ "unknown introductory commit and signer~%")))
> +    (values commit signer)))

I am wondering how this would work if somebody is working with multiple branches, in particular if they do not all use the same introduction. Maybe that doesn't need to be addressed in this patch series but it might be easier to address it in the future if the branch name was included in the config file instead assuming that a specific introduction applies to every branch in a given checkout (for example, by using "guix.authentication.introduction-commit.branch-name"). This is probably more relevant to external users of the tool than to the guix repository itself. The logistics of using unrelated branches simultaneously seems complicated and not very helpful, especially when channels are such an appealing alternative. But it could be useful for other projects.

> +(define (configured? repository)
> +  "Return true if REPOSITORY already container introduction info in its
> +'config' file."

Typo: this should be "contains", not "container"

Regards,
Skyler

[-- Attachment #2: Type: text/html, Size: 5486 bytes --]