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