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

[Oleg Pykhalov]

Hello Clément,

Thank you for review!

Clément Lassieur <address@hidden> writes:

[...]

>> +(define (serialize-repository-cgit-configuration x)
>> +  (serialize-configuration x repository-cgit-configuration-fields))
>
> '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.

--8<---------------cut here---------------start------------->8---
scheme@(gnu services cgit)> (serialize-configuration
                             (repository-cgit-configuration
                              (url "http://cgit.magnolia.local";)
                              (source-filter "la"))
                             repository-cgit-configuration-fields)
repo.enable-commit-graph=0
repo.enable-log-filecount=0
repo.enable-log-linecount=0
repo.enable-remote-branches=0
repo.enable-subject-links=0
repo.enable-html-serving=0
repo.hide=0
repo.ignore=0
repo.source-filter=la
repo.url=http://cgit.magnolia.local
scheme@(gnu services cgit)> 
It's been nice interacting with you!
Press C-c C-z to bring me back.
GNU Guile 2.2.2
Copyright (C) 1995-2017 Free Software Foundation, Inc.

Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
This program is free software, and you are welcome to redistribute it
under certain conditions; type `,show c' for details.

Enter `,help' for help.
scheme@(guile-user)> ,m (gnu services cgit)
;;; note: source file /home/natsu/src/guix-wip-cgit/gnu/services/cgit.scm
;;;       newer than compiled /home/natsu/src/guix-wip-cgit/gnu/services/cgit.go
;;; note: source file /home/natsu/src/guix-wip-cgit/gnu/services/cgit.scm
;;;       newer than compiled 
/home/natsu/.cache/guile/ccache/2.2-LE-8-3.A/home/natsu/src/guix-wip-cgit/gnu/services/cgit.scm.go
scheme@(gnu services cgit)> (serialize-configuration
                              (repository-cgit-configuration
                              (url "http://cgit.magnolia.local";)
                              (source-filter "la"))
                             repository-cgit-configuration-fields)
repo.enable-commit-graph=0
repo.enable-log-filecount=0
repo.enable-log-linecount=0
repo.enable-remote-branches=0
repo.enable-subject-links=0
repo.enable-html-serving=0
repo.hide=0
repo.ignore=0
repo.source-filter=la
repo.url=http://cgit.magnolia.local
--8<---------------cut here---------------end--------------->8---

> The same mechanism could be used for 'snapshots' and 'source-filter',
> according to the Archlinux link you provided.

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

--8<---------------cut here---------------start------------->8---
scheme@(gnu services cgit)> (serialize-configuration
                             (repository-cgit-configuration
                              (url "http://cgit.magnolia.local";)
                              (source-filter "la"))
                             repository-cgit-configuration-fields)
repo.url=http://cgit.magnolia.local
repo.enable-commit-graph=0
repo.enable-log-filecount=0
repo.enable-log-linecount=0
repo.enable-remote-branches=0
repo.enable-subject-links=0
repo.enable-html-serving=0
repo.hide=0
repo.ignore=0
repo.source-filter=la
--8<---------------cut here---------------end--------------->8---

>> +(define (serialize-module-link-path field-name val)
>> +  (if (null? val)
>> +      ""
>> +      (format #t "repo.~a.~a=~a\n"
>> +              (uglify-field-name field-name)
>> +              (car val) (cadr val))))
>
> I think 'match' is better than 'car' and 'cadr' because it names things.

Applied.

>> +(define (serialize-mimetype-alist field-name val)
>> +  (format #t "# Mimetypes\n~a"
>> +          (apply string-append
>> +                 (fold (lambda (x xs)
>> +                         (cons* (symbol->string (car x)) "=" (cadr x) "\n" 
>> xs))
>> +                       '() val))))
>
>
> Maybe you could use 'match' instead of 'car' and 'cadr'?  And I think
> 'x' and 'xs' are not very clear.  Also, isn't 'map' more natural than
> 'fold'?  I'd use something like this:
>
> (define (serialize-mimetype-alist-clem field-name val)
>   (format #t "# Mimetypes\n~a"
>           (string-join
>            (map (match-lambda
>                   ((extension mimetype)
>                    (format #f "~a=~a" (symbol->string extension) mimetype)))
>                 val) "\n")))
>
> Another advantage is that the order won't be inverted.

Applied.  Also I added forgoten "mimetype.".

>> +  (defbranch
>> +    (repo-string "")
>       ^--- Extra space here (because of 'def' being interpreted by your
>            text editor I reckon).

Applied.

>> +    "The name of the default branch for this repository. If no such branch
>                                Two spaces here :-) --------^

Applied.

>> +  (email-filter
>> +   (repo-string "")
>> +   "Override the default @code{email-filter.}")
>                                         }. ---^

Applied.

>> +  (enable-remote-branches?
>> +   (repo-boolean #f)
>> +   "Flag which, when set to @code{#t}, will make Cgit display remote
>> +branches in the summary and refs views.")
>
> 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

>> +(define-configuration cgit-configuration
>> +  (package
>> +    (package cgit)
>      ^--- There is one extra space here (because package is interpreted
>           by your text editor I reckon).

Applied.

>> +  (footer
>> +   (string "")
>> +   "The content of the file specified with this option will be included
>> +verbatim at the bottom of all pages (i.e. it replaces the standard
>> +\"generated by...\" message.")
>
> I think you forgot the closing parenthesis inside the comment.

Applied.

>> +  (max-stats
>> +   (string "")
>> +   "Maximum statistics period.  Valid values are @samp{week},@samp{month},
>                                               Space here :-) ---^
>> address@hidden and @samp{year.}")
>                          }. ----^

Done

>> +  (scan-hidden-path
>> +   (boolean #f)
>> +   "If set to @samp{#t} and repository-directory is enabled,
>> +repository-directory will recurse into directories whose name starts with a
>> +period.  Otherwise, repository-directory will stay away from such 
>> directories,
>> +considered as \"hidden\". Note that this does not apply to the \".git\"
>             Space here -----^

Applied.

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

>> +  (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).

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

> 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


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

0001-services-cgit-Add-more-configuration-fields.patch
Description: Text Data

Succeeded

    make check-system TESTS="cgit"


Thanks,
Oleg.