Re: [PATCH] Make git tag a builtin.

Hi,

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

Carlos Rica [off-list ref] writes:

quoted

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.

quoted

+		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.

quoted

+		/* 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.

quoted

+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