Re: [PATCH] Make git tag a builtin.

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 20 Jul 2007, Junio C Hamano wrote:

> Carlos Rica <jasampler@gmail.com> 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