Hello Clément, Thank you for review! Clément Lassieur 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 :-) ---^ >> +@samp{quarter} 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.