From: Franck Bui-Huu <vagabon.xyz@gmail.com>
Date: Sat, 09 Sep 2006 14:31:06 GMT
Subject: Re: [PATCH 1/4] Add git-archive
Message-ID: <cda58cb80609090731w7c66dcfbrbababb6c38d29bf6@mail.gmail.com>
In-Reply-To: <4501D0C5.702@lsrfire.ath.cx>

2006/9/8, Rene Scharfe <rene.scharfe@lsrfire.ath.cx>:
> Only a few trivial comments, as I managed to catch a cold somehow and
> can't think straight for longer than three seconds.
>
> >  .gitignore                    |    1
> >  Documentation/git-archive.txt |  100 ++++++++++++++++++
> >  Makefile                      |    3 -

[snip]

> > +
> > +     url = strdup(ar->remote);
>
> xstrdup()
>

ok, but need to rebase...

> > +     pid = git_connect(fd, url, buf);
> > +     if (pid < 0)
> > +             return pid;
> > +

[snip]

> > +     int extra_argc = 0;
> > +     const char *format = NULL; /* some default values */
>
> This comment does not convey any information.
>

OK, I'll remove it

> > +     const char *remote = NULL;
> > +     const char *base = "";

[snip]

> > +             }
> > +             if (arg[0] == '-') {
> > +                     extra_argv[extra_argc++] = arg;
>
> Overrun is not checked.
>

Indeed, I'll fix it.

> > +                     continue;
> > +             }
> > +             break;
> > +     }
> > +     if (list) {
> > +             if (!remote) {
> > +                     for (i = 0; i < ARRAY_SIZE(archivers); i++)
> > +                             printf("%s\n", archivers[i].name);
> > +                     exit(0);
> > +             }
> > +             die("--list and --remote are mutually exclusive");
> > +     }
>
> Not sure if we really need a list option.  I guess it only really
> makes sense if we have more than five formats.  I have no _strong_
> feelings against it, though. *shrug*
>

well it's almost free to add it, and no need any maintenance if we add
a new archiver backend, so I would say let it.

> > +     if (argc - i < 1) {
> > +             die("%s", archive_usage);
>
> usage()
>

ok

-- 
               Franck