From mboxrd@z Thu Jan  1 00:00:00 1970
From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=)
Subject: Re: [PATCH] Add (guix svn-download).
Date: Wed, 26 Mar 2014 17:00:03 +0100
Message-ID: <87eh1p0x30.fsf@gnu.org>
References: <1395830163-27977-1-git-send-email-sreeharsha@totakura.in>
	<1395830163-27977-2-git-send-email-sreeharsha@totakura.in>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50689)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1WSqFR-0008AV-4s
	for guix-devel@gnu.org; Wed, 26 Mar 2014 12:00:18 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1WSqFK-0008M9-L8
	for guix-devel@gnu.org; Wed, 26 Mar 2014 12:00:13 -0400
Received: from hera.aquilenet.fr ([2a01:474::1]:48633)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <ludo@gnu.org>)
	id 1WSqFK-0008Jk-FA
	for guix-devel@gnu.org; Wed, 26 Mar 2014 12:00:06 -0400
In-Reply-To: <1395830163-27977-2-git-send-email-sreeharsha@totakura.in> (Sree
	Harsha Totakura's message of "Wed, 26 Mar 2014 11:36:03 +0100")
List-Id: "Development of GNU Guix and the GNU System distribution."
	<guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
To: Sree Harsha Totakura <sreeharsha@totakura.in>
Cc: guix-devel@gnu.org

Sree Harsha Totakura <sreeharsha@totakura.in> 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."

=E2=80=98revision=E2=80=99 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 remove=
d when
> +                       ;; ca-certificates package is added.
> +                       "--trust-server-cert" "-r" revision url directory=
))

... possibly use (number->string revision) here =E2=86=91.

> +;;; 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 =E2=80=98revision=E2=80=
=99 is a number
or string, and add a newline before =E2=80=98define-record-type*=E2=80=99.

Thanks!

Ludo=E2=80=99.