Hello Clément,

apologies for such a long pause.  I tried to implement all we talked
about, but I kinda stuck.  What do you think about merging and not hold
it more for a small issue with project-list?  The patch is attached.

The test suite succeeds:
--8<---------------cut here---------------start------------->8---
./pre-inst-env env GUIX_PACKAGE_PATH= make check-system TESTS=cgit
--8<---------------cut here---------------end--------------->8---

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

[...]

> 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 we could stick with ordering fields and comments.

I'll add a note commentary about order at the head of the file.

>>> 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?

I think not only, because we have Cgit everywhere in the current
documentation.

>>>> +  (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?

Good idea, thank you!  I'll add a '(represents @code{scan-path})' to the
description of the 'repository-directory' field.

>>>> +  (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:
>
> a/b/foo.git
> c/bar.git
> baz.git
>
> 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.

Will be in a TODO list until I get more familiar with Guix or somebody
else add this.

>>> 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.

OK, thank you for a reference to Dovecot example.  I'll add this.

>> Also I need to mention that this patch probably will broke system
>> reconfigure and require update /etc/config.scm.
>
> Yes, of course :-)

OK.