Re: [PATCH 1/4] Add git-archive
- From
- Franck Bui-Huu <vagabon.xyz@gmail.com>
- Date
- Sep 9, 2006, 14:31 UTC
- Message-ID
- <cda58cb80609090731w7c66dcfbrbababb6c38d29bf6@mail.gmail.com>
- In-Reply-To
- <4501D0C5.702@lsrfire.ath.cx>
2006/9/8, Rene Scharfe <rene.scharfe@lsrfire.ath.cx>:
Show 6 quoted lines
> 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]
Show 5 quoted lines
> > + > > + url = strdup(ar->remote); > > xstrdup() >
ok, but need to rebase...
> > + pid = git_connect(fd, url, buf); > > + if (pid < 0) > > + return pid; > > +
[snip]
Show 5 quoted lines
> > + 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]
Show 6 quoted lines
> > + }
> > + if (arg[0] == '-') {
> > + extra_argv[extra_argc++] = arg;
>
> Overrun is not checked.
>Indeed, I'll fix it.
Show 17 quoted lines
> > + 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.
Show 5 quoted lines
> > + if (argc - i < 1) {
> > + die("%s", archive_usage);
>
> usage()
>ok
--
Franck