Hi,
On Fri, 20 Jul 2007, Junio C Hamano wrote:
> Carlos Rica <jasampler@xxxxxxxxx> writes:
>
> > This replaces the script "git-tag.sh" with "builtin-tag.c".
>
> Thanks. Will queue in 'next', and perhaps with a few nit fixups
> to merge as the first thing after 1.5.3.
Nice!
> launch-editor() would need adjustment for GIT_EDITOR and core.editor
> which should be straightforward.
Yes. As a separate commit? After a couple of revisions, I lost track of
what I looked at, and what not... So it would make it easier for me to
have these changes after the big builtin-tag commit.
> > + sp = buf = read_sha1_file(sha1, &type, &size);
> > + if (!buf || !size)
> > + return 0;
>
> Theoretically, I can create millions of lightweight tags, all of
> them pointing at a zero-length blob object (or an empty tree
> object) and kill you with memory leak here ;-).
Yes ;-) Would we really want to say
if (!buf)
return 0;
if (!size) {
free(buf);
return 0;
}
here? IMHO that is overkill.
> > + /* skip header */
> > + while (sp + 1 < buf + size &&
> > + !(sp[0] == '\n' && sp[1] == '\n'))
> > + sp++;
> > + /* only take up to "lines" lines, and strip the signature */
> > + for (i = 0, sp += 2; i < filter->lines && sp < buf + size &&
> > + prefixcmp(sp, PGP_SIGNATURE "\n");
> > + i++) {
>
> Minor nit; I would have split this to four physical lines, like:
>
> for (i = 0, sp += 2;
> i < filter->lines && sp < buf + size &&
> prefixcmp(sp, PGP_SIGNATURE "\n");
> i++) {
>
> The places that semicolons appear are more significant gaps when
> reading the code.
I am to blame for that. That code was outlined by me.
> > +int cmd_tag(int argc, const char **argv, const char *prefix)
> > +{
> > ...
> > + if (!strcmp(arg, "-F")) {
> > + unsigned long len;
> > + int fd;
> > +
> > + annotate = 1;
> > + i++;
> > + if (i == argc)
> > + die("option -F needs an argument.");
> > +
> > + if (!strcmp(argv[i], "-"))
> > + fd = 0;
> > + else {
> > + fd = open(argv[i], O_RDONLY);
> > + if (fd < 0)
> > + die("could not open '%s': %s",
> > + argv[i], strerror(errno));
> > + }
> > + len = 1024;
> > + message = xmalloc(len);
>
> You cannot anticipate how many bytes the user will type (or
> pipe-in), but when you opened the file you could fstat() to see
> how many bytes you would need to hold the contents of that
> file. Even in stdin case fstat(fd) could tell you the size, but
> I am not sure how to tell if the st_size is reliable. But for
> the purposes of "git tag", 1k buffer that grows on demand is
> probably cheaper than a fstat() syscall.
For a one-shot program as git-tag, I agree, the current patch is
sufficient.
Ciao,
Dscho
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
