Re: [PATCH] Add (guix svn-download).

[Ludovic Courtès]

Sree Harsha Totakura <address@hidden> skribis:

> * guix/svn-download.scm, guix/build/svn.scm: New files.
> * Makefile.am (MODULES): Add them.

Looks good to me!

Some comments:

> +(define* (svn-fetch url revision directory
> +                    #:key (svn-command "svn"))
> +  "Fetch REVISION from URL into DIRECTORY.  REVISION must be a valid svn
> +revision.  Return #t on success, #f otherwise."

‘revision’ can/should be a number, no?  Please augment the docstring to
say that, and...

> +  (and (zero? (system* svn-command "checkout" "--non-interactive"
> +                       ;; Trust the server certificate.  This is OK as we
> +                       ;; verify the checksum later.  This can be removed 
> when
> +                       ;; ca-certificates package is added.
> +                       "--trust-server-cert" "-r" revision url directory))

... possibly use (number->string revision) here ↑.

> +;;; Commentary:
> +;;;
> +;;; An <origin> method that fetches a specific revision from a Subversion
> +;;; repository.  The repository URL and revision are specified with a
> +;;; <svn-reference> object.
> +;;;
> +;;; Code:
> +(define-record-type* <svn-reference>
> +  svn-reference make-svn-reference
> +  svn-reference?
> +  (url    svn-reference-url)
> +  (revision svn-reference-revision))

Please align things, add a comment saying whether ‘revision’ is a number
or string, and add a newline before ‘define-record-type*’.

Thanks!

Ludo’.