From: Tay Ray Chuan <rctay89@gmail.com>
Date: Mon, 10 May 2010 12:00:42 GMT
Subject: Re: [WIP PATCH] Add skeleton remote helper for SVN
Message-ID: <AANLkTilzn2GLbxz3vLD4SoNIrYDU9_YzFfPI5Wv_WFs2@mail.gmail.com>
In-Reply-To: <1273491574-31870-2-git-send-email-artagnon@gmail.com>

Hi,

On Mon, May 10, 2010 at 7:39 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> diff --git a/Makefile b/Makefile
> index 4f7224a..06b969f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1658,7 +1658,7 @@ git.o git.spec \
>
>  TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
>  GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
> -       git.o http.o http-walker.o remote-curl.o
> +       git.o http.o http-walker.o remote-curl.o remote-svn.o
>  XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \
>        xdiff/xmerge.o xdiff/xpatience.o
>  OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS)

Johnathan (Cc'ed) had a recent patch
(<20100509035728.GA8198@progeny.tock>) that changed this area, and I
think it would be good to build on top of that.

> [snip]
> diff --git a/remote-svn.c b/remote-svn.c
> new file mode 100644
> index 0000000..dafbd6a
> --- /dev/null
> +++ b/remote-svn.c
> @@ -0,0 +1,202 @@
> [snip]
> +static int set_option(const char *name, const char *value)
> +{
> +       if (!strcmp(name, "verbosity")) {
> +               char *end;
> +               int v = strtol(value, &end, 10);
> +               if (value == end || *end)
> +                       return -1;
> +               options.verbosity = v;
> +               return 0;
> +       }
> +       else if (!strcmp(name, "progress")) {

We usually put the brace on the same line as the "else":

    if (...) {
      /* stuff */
    } else if (...) {
      /* stuff */
    }

(of course, tabs should be used - gmail doesn't allow me to do it here.)

> [snip]
> +static int export_handler(int nr_spec, char **specs)
> +{
> +       int err = 0;
> +
> +       // TODO: The real exporting
> +        // TODO: Write an importer for SVN

Here and elsewhere - no C++-style comments please.

> [snip]
> +static void parse_import(struct strbuf *buf)
> +{
> +       char **specs = NULL;
> +       int alloc_spec = 0, nr_spec = 0, i;
> +
> +       do {
> +               if (!prefixcmp(buf->buf, "import ")) {
> +                       ALLOC_GROW(specs, nr_spec + 1, alloc_spec);
> +                       specs[nr_spec++] = xstrdup(buf->buf + 5);
> +               }
> +               else
> +                       die("remote helper does not support %s", buf->buf);

See note on else-brace style.

-- 
Cheers,
Ray Chuan