Clément Lassieur <clement@lassieur.org> writes:

> Hi Christopher, thank you for the update!

Thanks for taking another look. I've just send another set of revised
patches.

> Christopher Baines <mail@cbaines.net> writes:
>
> [...]
>
>> +(define gitolite-setup
>> +  (match-lambda
>> +    (($ <gitolite-configuration> package user group home
>> +                                 rc-file admin-pubkey)
>> +     #~(let ((user-info (getpwnam #$user)))
>> +         (use-modules (guix build utils))
>> +
>> +         (simple-format #t "guix: gitolite: installing ~A\n" #$rc-file)
>> +         (copy-file #$rc-file #$(string-append home "/.gitolite.rc"))
>> +
>> +         (let ((admin-pubkey #$admin-pubkey)
>
> What's the point of that 'let'?  Afterwards you reuse '$admin-pubkey'
> :-).

Ah yeah, I've fixed that now.

>> +               (pubkey-file #$(string-append home "/id_rsa.pub")))
>> +           (when admin-pubkey
>
> If we are 'gitolite-setup', that means 'admin-pubkey' is true, I think,
> so that 'when' is useless.

Indeed. I've removed the gitolite-setup function now, along with this
conditional.

>> +             ;; The key must be writable, so copy it from the store
>> +             (copy-file #$admin-pubkey pubkey-file)
>> +
>> +             (chmod pubkey-file #o500)
>> +             (chown pubkey-file
>> +                    (passwd:uid user-info)
>> +                    (passwd:gid user-info))
>> +
>> +             ;; Set the git configuration, to avoid gitolite trying to use
>> +             ;; the hostname command, as the network might not be up yet
>> +             (with-output-to-file #$(string-append home "/.gitconfig")
>> +               (lambda ()
>> +                 (display "[user]
>> +        name = GNU Guix
>> +        email = guix@localhost
>> +"))))
>> +           ;; Run Gitolite setup, as this updates the hooks and include the
>> +           ;; admin pubkey if specified. The admin pubkey is required for
>> +           ;; initial setup, and will replace the previous key if run after
>> +           ;; initial setup
>> +           (let ((pid (primitive-fork)))
>> +             (if (eq? pid 0)
>
> I have a slight preference for the previous 'match' expression you used
> before, because it's used elsewhere this way and it requires less code.

While I agree with both your points, I tried for quite a while last
weekend to get match to work, and couldn't. I couldn't even tell why it
suddenly wasn't. Unfortunately, Linux panicing when anything fails makes
debugging the system test a bit tricky.

>> +                 (begin
>
> I think that 'begin' is useless.

Yeah, I think I added that while trying to get match to work. I've
removed it now.

>> +                   ;; Exit with a non-zero status code if an exception is thrown.
>> +                   (dynamic-wind
>> +                     (const #t)
>> +                     (lambda ()
>> +                       (setenv "HOME" (passwd:dir user-info))
>> +                       (setenv "USER" #$user)
>> +                       (setgid (passwd:gid user-info))
>> +                       (setuid (passwd:uid user-info))
>> +                       (primitive-exit
>> +                        (apply system*
>> +                               #$(file-append package "/bin/gitolite")
>> +                               "setup"
>> +                               (if admin-pubkey
>> +                                   `("-pk" ,pubkey-file)
>> +                                   '()))))
>> +                     (lambda ()
>> +                       (primitive-exit 1))))
>> +                 (waitpid pid)))
>> +
>> +           (when (file-exists? pubkey-file)
>> +             (delete-file pubkey-file)))))))
>> +
>> +(define (gitolite-activation config)
>> +  (if (gitolite-configuration-admin-pubkey config)
>> +      (gitolite-setup config)
>> +      #~(display
>> +         "guix: Skipping gitolite setup as the admin-pubkey has not been provided\n")))
>
> I'm not fan of the idea that a user might:
>   1. setup an initial configuration with 'admin-pubkey',
>   2. change that configuration once the initial activation has been
>      done.
>
> What is the drawback to forcing the user to setup an 'admin-pubkey'?
> Maybe you think that doing the activation is annoying and it should only
> be done when necessary?  If that's the case, maybe what we need is an
> ad-hoc command instead of the activation, a bit like the
> 'certbot-command' of the Certbot service.

I wrote it this way as this is how I've been using Gitolite so far. On
Debian, I think debconf prompts you for the key when you install the
package, and runs gitolite setup.

I've actually read the gitolite setup script now, and its behaviour it
pretty reasonable if it's run frequently. As I understand it, it ensures
that the provided admin-pubkey exists in the keydir directory in the
gitolite-admin repository, and will commit to the repository if it
changes anything.

So, I think I've now changed both the service and the documentation to
describe adding the admin-pubkey always.

>> +          (test-eq "cloning the admin repository"
>> +            #t
>
> test-assert
>
>> +            (test-eq "pushing, and the associated hooks"
>> +              #t
>
> test-assert

I've changed these now :)

>> +              (invoke #$(file-append git "/bin/git") "push")))
>
> Could you confirm that if a hook fails, that test will fail?

Yep, I added this check when I realised that the hooks were broken.