[bug#29820] [PATCH] services: cgit: Add more configuration fields.

[Clément Lassieur]

Oleg Pykhalov <address@hidden> writes:

>> 'url' needs to be the first setting specified for each repo.  I think
>> you could use something like this to make sure it is:
>>
>> (define (serialize-repository-cgit-configuration x)
>>   (define (rest? field)
>>     (not (eq? (configuration-field-name field) 'url)))
>>   (let ((url (repository-cgit-configuration-url x))
>>         (rest (filter rest? repository-cgit-configuration-fields)))
>>     (serialize-repo-string 'url url)
>>     (serialize-configuration x rest)))
>
> Output doesn't change.

[...]

> However, simple reordering fields in (define-configuration …) works.
> I reordered in attached patch.  Is it good enough?

It didn't work because you didn't call the
'serialize-repository-cgit-configuration' function.  With this snippet,
it will be called:

--8<---------------cut here---------------start------------->8---
(serialize-configuration
 (cgit-configuration
  (repositories
   (list
    (repository-cgit-configuration
     (url "http://cgit.magnolia.local";)
     (source-filter "la")))))
 cgit-configuration-fields)
--8<---------------cut here---------------end--------------->8---

I'm not sure which way is the best.  I tend to prefer a hard coded logic
because it will prevent bugs if later people forget about the order and
add fields at the wrong place.  If you stick with depending on the
fields order, then could you add very clear comments so that people
don't add fields at the wrong place?  WDYT?

>> I think the official project uses 'cgit' instead of 'Cgit' (there are
>> other occurrences where you use 'Cgit').
>
> Ludovic asked to capitalize cgit in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28283#14

But he was only talking about titles wasn't he?

>>> +  (repository-directory
>>> +   (repository-directory "/srv/git")
>>> +   "Name of the directory to scan for repositories.")
>>
>> I believe it would be clearer if it was named the same way cgit names
>> it: scan-path.
>
> Ludovic asked to rename it in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28283#14
>
> I don't know should we stay close to cgit naming conventions or not.
> Thoughts?

At least it should be possible to grep 'scan_path' in the documentation,
so that users can easily find what to use instead of 'scan-path'.  Could
you say 'scan_path' is the original name in the docstring?

>>> +  (project-list
>>> +   (list '())
>>> +   "A list of subdirectories inside of @code{repository-directory}, 
>>> relative
>>> +to it, that should loaded as Git repositories.")
>>
>> I forgot one thing: 'project-list' is a file, not a list of strings.  I
>> agree it's weird that cgit's documentation doesn't say it's a file.  I
>> see two solutions:
>
> Sorry, it's not clear for me.  As I understand from CGITRC(5) it's a
> list like:
>
>     project-list=/share/cgit/cgit.png /share/cgit/cgit.jpg
>
> relative to /srv/git (in our case).

CGITRC isn't clear.  It's really a file containing the list of git
directories.  For example:

/etc/cgit/project-list:
--8<---------------cut here---------------start------------->8---
a/b/foo.git
c/bar.git
baz.git
--8<---------------cut here---------------end--------------->8---

And

project-list=/etc/cgit/project-list

>> 1. Change the type to 'string', so that people can set a file name.
>>
>> 2. Use a list type that would transparently transform its values into a
>>    file in the store, with the generated cgitrc file pointing to it.
>>
>> The second solution is better because the user won't need to create the
>> file.
>
> I choose 1st for now, because 2nd I don't understand what need to be
> produced at the end.  Could you give me an example?

With 2nd, users would write a configuration like

(project-list '("a/b/foo.git"
                "c/bar.git"
                "baz.git"))

And 'guix system reconfigure' would create the file
/gnu/store/xxxxxxx-project-list containing those three lines.  The
generated cgitrc file would contain:

project-list=/gnu/store/xxxxxxx-project-list

You could use a type whose serializer would call the 'plain-file'
procedure.

>> Also, could you add a way to use an opaque configuration file?  It would
>> be helpful for users who don't have time to update their configuration,
>> or in case there are new cgit configurations that are not yet
>> implemented by the Scheme service.
>
> Sure.
>
> Is the following order is OK?
>
>     (serialize-configuration
>      (cgit-configuration
>       (extra-options (list "soo=do"))
>       (repositories (list
>                      (repository-cgit-configuration
>                       (module-link-path '("/super/cow" "moo"))
>                       (extra-options (list "goo=foo"))))))
>      cgit-configuration-fields)
>
>     …
>     repo.extra-options=goo=foo
>     extra-options=soo=do
>     # END OF FILE

I was more thinking about something like in the Dovecot service where
you can pass the whole file as a string.

> Also I need to mention that this patch probably will broke system
> reconfigure and require update /etc/config.scm.

Yes, of course :-)