error: char103: premature end of data

error: char103: premature end of data

[Johannes Schindelin]

Hi,

I just tried to fetch from one of my repos which was perfectly usable (and 
fetchable) before, and got this error message:

	error: char103: premature end of data

WTF? Since when did we stop using non-cryptic error messages?

Needless to say, I am very unhappy with such a message, especially in a 
repo which worked perfectly, thank you.

I somehow suspect that this has something to do with the recent work on 
the tag objects. If so, NACK on that patch series from me.

Ciao,
Dscho

Re: error: char103: premature end of data

[Johan Herland]

On Friday 08 June 2007, Johannes Schindelin wrote:
> Hi,
> 
> I just tried to fetch from one of my repos which was perfectly usable (and 
> fetchable) before, and got this error message:
> 
> 	error: char103: premature end of data

Sorry about that. Do you have an idea of which tag object caused the 
failure? If so, could you send the output of "git-cat-file tag <name>" on 
it?

git-fsck on the repo should report the same error. If you run with -v 
(verbose) you should also get some hints as to which tag object causes 
this.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: error: char103: premature end of data

[Johannes Schindelin]

Hi,

On Thu, 7 Jun 2007, Johannes Schindelin wrote:

> I somehow suspect that this has something to do with the recent work on 
> the tag objects. If so, NACK on that patch series from me.

Okay. Instead of doing the work I have to do, which I cannot, because 
a vital part of git stopped working for me, I tracked down that it is 
indeed that monster commit v1.5.2.1-134-gc7de6eb AKA ':/Refactor git tag 
objects;'

I have to repeat my NACK on it. I did not have time to review it, when 
you sent it, so I tried to find what is wrong now (when I don't have 
time for it to begin with).

But it is really intrusive, introduces problems, is less than readable, 
and it is not at all clear what problems it solves, while it looks like it 
could be broken down to easily-reviewable fixes, cleanups, and changes, so 
please fix it while I recover from my troubles.

Ciao,
Dscho

Re: error: char103: premature end of data

[Johannes Schindelin]

Hi,

On Fri, 8 Jun 2007, Johan Herland wrote:

> On Friday 08 June 2007, Johannes Schindelin wrote:
>
> > I just tried to fetch from one of my repos which was perfectly usable 
> > (and fetchable) before, and got this error message:
> > 
> > 	error: char103: premature end of data
> 
> Sorry about that. Do you have an idea of which tag object caused the 
> failure? If so, could you send the output of "git-cat-file tag <name>" 
> on it?

Yes, I know what causes it. A tag with an empty message.

And I even know why it does that. It's easy. Look into git-tag.sh, and you 
will find that it does a git-stripspace on the final message. If that was 
empty, then the tag will just be the tag header.

> git-fsck on the repo should report the same error. If you run with -v 
> (verbose) you should also get some hints as to which tag object causes 
> this.

Yes, it finds the error. And crashes. And costs me time.

Why do we have to parse _everything_ in the tag to begin with? It's not 
like I will show the information of the darn thing when I just fetch it 
from repo A into repo B.

Ciao,
Dscho

Re: error: char103: premature end of data

[Johan Herland]

On Friday 08 June 2007, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 8 Jun 2007, Johan Herland wrote:
> 
> > On Friday 08 June 2007, Johannes Schindelin wrote:
> >
> > > I just tried to fetch from one of my repos which was perfectly usable 
> > > (and fetchable) before, and got this error message:
> > > 
> > > 	error: char103: premature end of data
> > 
> > Sorry about that. Do you have an idea of which tag object caused the 
> > failure? If so, could you send the output of "git-cat-file tag <name>" 
> > on it?
> 
> Yes, I know what causes it. A tag with an empty message.

Of course. Although if one uses git-tag -a to create an annotated tag object 
I have a hard time seeing why one wouldn't supply a message. But it's of 
course entirly valid not to supply a message.

> And I even know why it does that. It's easy. Look into git-tag.sh, and you 
> will find that it does a git-stripspace on the final message. If that was 
> empty, then the tag will just be the tag header.

You're right. I'll fix my code to handle this case. I'll also try to split
that first patch in my series into more manageable chunks.

> > git-fsck on the repo should report the same error. If you run with -v 
> > (verbose) you should also get some hints as to which tag object causes 
> > this.
> 
> Yes, it finds the error. And crashes. And costs me time.
> 
> Why do we have to parse _everything_ in the tag to begin with? It's not 
> like I will show the information of the darn thing when I just fetch it 
> from repo A into repo B.

We parse the tag completely in order to detect corruption/invalid tags as 
early as possible. If I'm pulling from a corrupt repo, I'd sure as hell 
want git to tell me when I first fetched, and not a couple of weeks later 
when I'd try to use the corrupt data, or call fsck, or whatever.

Again, I'm sorry for your problems.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: error: char103: premature end of data

[Johannes Schindelin]

Hi,

On Fri, 8 Jun 2007, Johan Herland wrote:

> If I'm pulling from a corrupt repo, I'd sure as hell want git to tell me 
> when I first fetched, and not a couple of weeks later when I'd try to 
> use the corrupt data, or call fsck, or whatever.

We don't check that the object names match the object contents on pull. 
With your argument, we should do that, too. And of course, get rid of 
git-fsck, because it is of no more use, then.

Ciao,
Dscho

[PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\n')

[Johan Herland]

Thanks to Johannes Schindelin <Johannes.Schindelin@gmx.de> for
discovering this.

Also add a testcase for this condition.

Signed-off-by: Johan Herland <johan@herland.net>
---

This patch should hopefully fix your problem.


...Johan

 t/t3800-mktag.sh |   15 +++++++++++++++
 tag.c            |    9 ++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index ac9008a..ac7cbbc 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -399,5 +399,20 @@ test_expect_success \
     'create valid tag #4' \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
+############################################################
+# 24. create valid tag #4 (with empty message)
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger a
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
 
 test_done
diff --git a/tag.c b/tag.c
index e371179..875145b 100644
--- a/tag.c
+++ b/tag.c
@@ -131,10 +131,6 @@ int parse_and_verify_tag_buffer(struct tag *item, const char *data, const unsign
 		header_end = memchr(tagger_line, '\n', end - tagger_line);
 		if (!header_end++)
 			return error("char" PD_FMT ": could not find \"\\n\" after \"tagger\"", tagger_line - data);
-		if (end - header_end < 1)
-			return error("char" PD_FMT ": premature end of data", header_end - data);
-		if (*header_end != '\n') /* header must end with "\n\n" */
-			return error("char" PD_FMT ": could not find blank line after header section", header_end - data);
 	}
 	else {
 		/* Treat tagger line as optional */
@@ -148,9 +144,8 @@ int parse_and_verify_tag_buffer(struct tag *item, const char *data, const unsign
 			header_end = tagger_line;
 	}
 
-	if (end - header_end < 1)
-		return error("char" PD_FMT ": premature end of data", header_end - data);
-	if (*header_end != '\n') /* header must end with "\n\n" */
+	/* header must end with "\n\n", but "\n" is acceptable if at EOF */
+	if (header_end < end - 1 && *header_end != '\n') /* not at EOF, and next char is not '\n' */
 		return error("char" PD_FMT ": could not find blank line after header section", header_end - data);
 
 	/*
-- 
1.5.2

Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\n')

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Thanks to Johannes Schindelin <Johannes.Schindelin@gmx.de> for
> discovering this.
>
> Also add a testcase for this condition.
>
> Signed-off-by: Johan Herland <johan@herland.net>

While this certainly is an improvement, I suspect that your
parse_tag() does a little too much.  In a format such as "tag"
object that does header + blank + body, it is customary to allow
header fields that your version does not understand (assuming
that such extention will go after the known fields is fine).

Which means that you should not be even saying "Ok, I've checked all
headers I know about---there should be a double LF to terminate it",
as you do not know if headers have ended.

Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\n')

[Johan Herland]

On Friday 08 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> 
> > Thanks to Johannes Schindelin <Johannes.Schindelin@gmx.de> for
> > discovering this.
> >
> > Also add a testcase for this condition.
> >
> > Signed-off-by: Johan Herland <johan@herland.net>
> 
> While this certainly is an improvement, I suspect that your
> parse_tag() does a little too much.  In a format such as "tag"
> object that does header + blank + body, it is customary to allow
> header fields that your version does not understand (assuming
> that such extention will go after the known fields is fine).
> 
> Which means that you should not be even saying "Ok, I've checked all
> headers I know about---there should be a double LF to terminate it",
> as you do not know if headers have ended.

Ok, I'm currently working on a patch series for Dscho and others where I 
split up the big patch ('[PATCH 1/6] Refactor git tag objects; make "tag" 
header optional; introduce new optional "keywords" header') into babysteps.

I can:

1. Provide a new patch series to totally replace the previous 6-part patch 
series (plus bugfixes). The new patch series will make smaller steps and 
end up (hopefully) in a better place, with less overzealous 
checking/parsing, and more "traditional" whitespacing.

OR

2. Provide the babystep-series ending up exactly where we are today (i.e. 
after the patch series, plus bug fixes). Then, provide patches on top of 
the existing series to get it into shape, both scope-wise (i.e. not trying 
to do too much) and whitespace-wise.

Which do you prefer?


...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\n')

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I can:
>
> 1. Provide a new patch series to totally replace the previous 6-part patch 
> series (plus bugfixes). The new patch series will make smaller steps and 
> end up (hopefully) in a better place, with less overzealous 
> checking/parsing, and more "traditional" whitespacing.
>
> OR
>
> 2. Provide the babystep-series ending up exactly where we are today (i.e. 
> after the patch series, plus bug fixes). Then, provide patches on top of 
> the existing series to get it into shape, both scope-wise (i.e. not trying 
> to do too much) and whitespace-wise.
>
> Which do you prefer?

I am not sure if there is any practical difference between the
two ;-).  But in either case, it appears that we should first
revert d9fa4a8 from 'next' and start from clean slate.  It
really seems that the patch series did upset too many people;
personally I found the first patch still was follow-able, but I
do agree that it should have been much smaller and not mixing
too many things into one).

So, let's do 1.

[PATCH 0/21] Refactor the tag object (take 2)

[Johan Herland]

This patch series implements part of the ground work for the 'notes'
feature discussed earlier in the thread "[PATCH 00/15] git-note: A
mechanism for providing free-form after-the-fact annotations on commits".

The following patches refactors the tag object by:
1. Unifying parsing and verification of tag objects (patches 1-9)
2. Do better and more thorough verification of tag objects (patches 10-13)
3. Making the "tagger" header mandatory as far as possible (patch 14)
4. Making the "tag" header optional (patch 15)
5. Introducing a new optional "keywords" header (patch 16)
6. Auxiliary changes supporting the above (patches 17-21)

This patch series replaces the earlier patch series of the same name
(plus the current bugfixes on top of that series). It's also much easier
on the eyes for those with 80 chars wide displays. Also, the selftest
suite should run successfully at any point in this patch series.

Here's the shortlog:

Johan Herland (21):
      Remove unnecessary code and comments on non-existing 8kB tag object restriction
      Return error messages when parsing fails.
      Refactoring to make verify_tag() and parse_tag_buffer() more similar
      Refactor verification of "tagger" line to be more similar to verification of "type" and "tagger" lines
      Make parse_tag_buffer_internal() handle item == NULL
      Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'
      Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()
      Switch from verify_tag() to parse_and_verify_tag_buffer() for verifying tag objects in git-mktag
      Remove unneeded code from mktag.c
      Free mktag's buffer before dying
      Rewrite error messages; fix up line lengths
      Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers
      Collect skipping of header field names and calculation of line lengths in one place
      Add proper parsing of "tagger" line, but only when thorough_verify is set
      Make tag names (i.e. the tag object's "tag" line) optional
      Introduce optional "keywords" on tag objects
      Update comments on tag objects in mktag.c
      git-fsck: Do thorough verification of tag objects
      Documentation/git-mktag: Document the changes in tag object structure
      git-mktag tests: Expand on mktag selftests according to the new tag object structure
      Add fsck_verify_ref_to_tag_object() to verify that refname matches name stored in tag object

 Documentation/git-mktag.txt |   41 +++++--
 builtin-fsck.c              |   41 +++++++
 builtin-log.c               |    2 +-
 mktag.c                     |  147 +++++-------------------
 t/t3800-mktag.sh            |  231 ++++++++++++++++++++++++++++++++++---
 tag.c                       |  268 +++++++++++++++++++++++++++++++++++--------
 tag.h                       |    5 +-
 7 files changed, 540 insertions(+), 195 deletions(-)


Have fun!

...Johan

[PATCH 01/21] Remove unnecessary code and comments on non-existing 8kB tag object restriction

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/mktag.c b/mktag.c
index 070bc96..b82e377 100644
--- a/mktag.c
+++ b/mktag.c
@@ -12,15 +12,8 @@
  * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
  * shortest possible type-line, and "tag .\n" at 6 bytes is the
  * shortest single-character-tag line.
- *
- * We also artificially limit the size of the full object to 8kB.
- * Just because I'm a lazy bastard, and if you can't fit a signature
- * in that size, you're doing something wrong.
  */
 
-/* Some random size */
-#define MAXSIZE (8192)
-
 /*
  * We refuse to tag something we can't verify. Just because.
  */
-- 
1.5.2

[PATCH 02/21] Return error messages when parsing fails.

[Johan Herland]

This patch brings the already similar tag.c:parse_tag_buffer() and
mktag.c:verify_tag() a little bit closer to eachother.

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/tag.c b/tag.c
index bbacd59..954ed8a 100644
--- a/tag.c
+++ b/tag.c
@@ -35,6 +35,12 @@ struct tag *lookup_tag(const unsigned char *sha1)
 
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
+#ifdef NO_C99_FORMAT
+#define PD_FMT "%d"
+#else
+#define PD_FMT "%td"
+#endif
+
 	int typelen, taglen;
 	unsigned char sha1[20];
 	const char *type_line, *tag_line, *sig_line;
@@ -45,28 +51,41 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
         item->object.parsed = 1;
 
 	if (size < 64)
-		return -1;
-	if (memcmp("object ", data, 7) || get_sha1_hex((char *) data + 7, sha1))
-		return -1;
+		return error("failed preliminary size check");
 
+	/* Verify object line */
+	if (memcmp(data, "object ", 7))
+		return error("char%d: does not start with \"object \"", 0);
+
+	if (get_sha1_hex((char *) data + 7, sha1))
+		return error("char%d: could not get SHA1 hash", 7);
+
+	/* Verify type line */
 	type_line = (char *) data + 48;
-	if (memcmp("\ntype ", type_line-1, 6))
-		return -1;
+	if (memcmp(type_line - 1, "\ntype ", 6))
+		return error("char%d: could not find \"\\ntype \"", 47);
 
+	/* Verify tag-line */
 	tag_line = strchr(type_line, '\n');
-	if (!tag_line || memcmp("tag ", ++tag_line, 4))
-		return -1;
+	if (!tag_line)
+		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - (char *) data);
+	tag_line++;
+	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
+		return error("char" PD_FMT ": no \"tag \" found", tag_line - (char *) data);
 
 	sig_line = strchr(tag_line, '\n');
 	if (!sig_line)
 		return -1;
 	sig_line++;
 
+	/* Get the actual type */
 	typelen = tag_line - type_line - strlen("type \n");
-	if (typelen >= 20)
-		return -1;
+	if (typelen >= sizeof(type))
+		return error("char" PD_FMT ": type too long", type_line+5 - (char *) data);
+
 	memcpy(type, type_line + 5, typelen);
 	type[typelen] = '\0';
+
 	taglen = sig_line - tag_line - strlen("tag \n");
 	item->tag = xmalloc(taglen + 1);
 	memcpy(item->tag, tag_line + 4, taglen);
@@ -92,6 +111,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 	}
 
 	return 0;
+
+#undef PD_FMT
 }
 
 int parse_tag(struct tag *item)
-- 
1.5.2

[PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar

[Johan Herland]

A fair amount of refactoring is included in this patch, none of which
should affect the actual behaviour of the code in any way.

Here are the changes done:

- Refactor out all the (char *) casting in parse_tag_buffer(). Create a
  wrapper function (parse_tag_buffer()) that casts _once_ and then calls
  parse_tag_buffer_internal() which does the real work

- Variable renaming in parse_tag_buffer_internal():
  - sig_line -> tagger_line
  - typelen  -> type_len
  - taglen   -> tag_len

- Variable renaming in verify_tag():
  - buffer  -> data
  - typelen -> type_len

- Remove unnecessary variable 'object' from verify_tag(). It has always
  the same value as 'data', so use 'data' directly instead

- Fix type of length variables (int -> unsigned long)

- Move null-termination of tag buffer out of verify_tag() and into main().

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   41 +++++++++++++++++++----------------------
 tag.c   |   50 +++++++++++++++++++++++++++-----------------------
 2 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/mktag.c b/mktag.c
index b82e377..0bc20c8 100644
--- a/mktag.c
+++ b/mktag.c
@@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 	return ret;
 }
 
+static int verify_tag(char *data, unsigned long size)
+{
 #ifdef NO_C99_FORMAT
 #define PD_FMT "%d"
 #else
 #define PD_FMT "%td"
 #endif
 
-static int verify_tag(char *buffer, unsigned long size)
-{
-	int typelen;
-	char type[20];
 	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line;
+	char type[20];
+	const char *type_line, *tag_line, *tagger_line;
+	unsigned long type_len;
 
 	if (size < 64)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
-	buffer[size] = 0;
-
 	/* Verify object line */
-	object = buffer;
-	if (memcmp(object, "object ", 7))
+	if (memcmp(data, "object ", 7))
 		return error("char%d: does not start with \"object \"", 0);
 
-	if (get_sha1_hex(object + 7, sha1))
+	if (get_sha1_hex(data + 7, sha1))
 		return error("char%d: could not get SHA1 hash", 7);
 
 	/* Verify type line */
-	type_line = object + 48;
+	type_line = data + 48;
 	if (memcmp(type_line - 1, "\ntype ", 6))
 		return error("char%d: could not find \"\\ntype \"", 47);
 
 	/* Verify tag-line */
 	tag_line = strchr(type_line, '\n');
 	if (!tag_line)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
+		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
 	tag_line++;
 	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
+		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
 
 	/* Get the actual type */
-	typelen = tag_line - type_line - strlen("type \n");
-	if (typelen >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
-
-	memcpy(type, type_line+5, typelen);
-	type[typelen] = 0;
+	type_len = tag_line - type_line - strlen("type \n");
+	if (type_len >= sizeof(type))
+		return error("char" PD_FMT ": type too long", type_line + 5 - data);
+	memcpy(type, type_line + 5, type_len);
+	type[type_len] = '\0';
 
 	/* Verify that the object matches */
 	if (verify_object(sha1, type))
@@ -91,23 +87,23 @@ static int verify_tag(char *buffer, unsigned long size)
 			break;
 		if (c > ' ')
 			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
+		return error("char" PD_FMT ": could not verify tag name", tag_line - data);
 	}
 
 	/* Verify the tagger line */
 	tagger_line = tag_line;
 
 	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
-		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
+		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
 
 	/* TODO: check for committer info + blank line? */
 	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
 
 	/* The actual stuff afterwards we don't care about.. */
 	return 0;
-}
 
 #undef PD_FMT
+}
 
 int main(int argc, char **argv)
 {
@@ -124,6 +120,7 @@ int main(int argc, char **argv)
 		free(buffer);
 		die("could not read from stdin");
 	}
+	buffer[size] = 0;
 
 	/* Verify it for some basic sanity: it needs to start with
 	   "object <sha1>\ntype\ntagger " */
diff --git a/tag.c b/tag.c
index 954ed8a..8d31603 100644
--- a/tag.c
+++ b/tag.c
@@ -33,7 +33,7 @@ struct tag *lookup_tag(const unsigned char *sha1)
         return (struct tag *) obj;
 }
 
-int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
+static int parse_tag_buffer_internal(struct tag *item, const char *data, const unsigned long size)
 {
 #ifdef NO_C99_FORMAT
 #define PD_FMT "%d"
@@ -41,14 +41,14 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 #define PD_FMT "%td"
 #endif
 
-	int typelen, taglen;
 	unsigned char sha1[20];
-	const char *type_line, *tag_line, *sig_line;
 	char type[20];
+	const char *type_line, *tag_line, *tagger_line;
+	unsigned long type_len, tag_len;
 
-        if (item->object.parsed)
-                return 0;
-        item->object.parsed = 1;
+	if (item->object.parsed)
+		return 0;
+	item->object.parsed = 1;
 
 	if (size < 64)
 		return error("failed preliminary size check");
@@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 	if (memcmp(data, "object ", 7))
 		return error("char%d: does not start with \"object \"", 0);
 
-	if (get_sha1_hex((char *) data + 7, sha1))
+	if (get_sha1_hex(data + 7, sha1))
 		return error("char%d: could not get SHA1 hash", 7);
 
 	/* Verify type line */
-	type_line = (char *) data + 48;
+	type_line = data + 48;
 	if (memcmp(type_line - 1, "\ntype ", 6))
 		return error("char%d: could not find \"\\ntype \"", 47);
 
 	/* Verify tag-line */
 	tag_line = strchr(type_line, '\n');
 	if (!tag_line)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - (char *) data);
+		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
 	tag_line++;
 	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - (char *) data);
+		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
 
-	sig_line = strchr(tag_line, '\n');
-	if (!sig_line)
+	tagger_line = strchr(tag_line, '\n');
+	if (!tagger_line)
 		return -1;
-	sig_line++;
+	tagger_line++;
 
 	/* Get the actual type */
-	typelen = tag_line - type_line - strlen("type \n");
-	if (typelen >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line+5 - (char *) data);
-
-	memcpy(type, type_line + 5, typelen);
-	type[typelen] = '\0';
+	type_len = tag_line - type_line - strlen("type \n");
+	if (type_len >= sizeof(type))
+		return error("char" PD_FMT ": type too long", type_line + 5 - data);
+	memcpy(type, type_line + 5, type_len);
+	type[type_len] = '\0';
 
-	taglen = sig_line - tag_line - strlen("tag \n");
-	item->tag = xmalloc(taglen + 1);
-	memcpy(item->tag, tag_line + 4, taglen);
-	item->tag[taglen] = '\0';
+	tag_len = tagger_line - tag_line - strlen("tag \n");
+	item->tag = xmalloc(tag_len + 1);
+	memcpy(item->tag, tag_line + 4, tag_len);
+	item->tag[tag_len] = '\0';
 
 	if (!strcmp(type, blob_type)) {
 		item->tagged = &lookup_blob(sha1)->object;
@@ -115,6 +114,11 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 #undef PD_FMT
 }
 
+int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
+{
+	return parse_tag_buffer_internal(item, (const char *) data, size);
+}
+
 int parse_tag(struct tag *item)
 {
 	enum object_type type;
-- 
1.5.2

[PATCH 04/21] Refactor verification of "tagger" line to be more similar to verification of "type" and "tagger" lines

[Johan Herland]

Also update selftests to reflect that verification of "tagger" now
happens _before_ verification of type name, object sha1 and tag name.

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c          |   16 ++++++++--------
 t/t3800-mktag.sh |    3 +++
 tag.c            |    6 +++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/mktag.c b/mktag.c
index 0bc20c8..4dbefab 100644
--- a/mktag.c
+++ b/mktag.c
@@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)
 
 	/* Verify tag-line */
 	tag_line = strchr(type_line, '\n');
-	if (!tag_line)
+	if (!tag_line++)
 		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
-	tag_line++;
 	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
 		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
 
+	/* Verify the tagger line */
+	tagger_line = strchr(tag_line, '\n');
+	if (!tagger_line++)
+		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
+	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
+
 	/* Get the actual type */
 	type_len = tag_line - type_line - strlen("type \n");
 	if (type_len >= sizeof(type))
@@ -90,12 +96,6 @@ static int verify_tag(char *data, unsigned long size)
 		return error("char" PD_FMT ": could not verify tag name", tag_line - data);
 	}
 
-	/* Verify the tagger line */
-	tagger_line = tag_line;
-
-	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
-		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
-
 	/* TODO: check for committer info + blank line? */
 	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
 
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 7c7e433..b4edb4d 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -133,6 +133,7 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type taggggggggggggggggggggggggggggggg
 tag mytag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
@@ -148,6 +149,7 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tagggg
 tag mytag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
@@ -163,6 +165,7 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag my	tag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
diff --git a/tag.c b/tag.c
index 8d31603..19c66cd 100644
--- a/tag.c
+++ b/tag.c
@@ -73,10 +73,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
 		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
 
+	/* Verify the tagger line */
 	tagger_line = strchr(tag_line, '\n');
-	if (!tagger_line)
-		return -1;
-	tagger_line++;
+	if (!tagger_line++)
+		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
 
 	/* Get the actual type */
 	type_len = tag_line - type_line - strlen("type \n");
-- 
1.5.2

[PATCH 05/21] Make parse_tag_buffer_internal() handle item == NULL

[Johan Herland]

This is in preparation for unifying verify_tag() and
parse_tag_buffer_internal().

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   54 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/tag.c b/tag.c
index 19c66cd..b134967 100644
--- a/tag.c
+++ b/tag.c
@@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 	const char *type_line, *tag_line, *tagger_line;
 	unsigned long type_len, tag_len;
 
-	if (item->object.parsed)
-		return 0;
-	item->object.parsed = 1;
+	if (item) {
+		if (item->object.parsed)
+			return 0;
+		item->object.parsed = 1;
+	}
 
 	if (size < 64)
 		return error("failed preliminary size check");
@@ -85,28 +87,30 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 	memcpy(type, type_line + 5, type_len);
 	type[type_len] = '\0';
 
-	tag_len = tagger_line - tag_line - strlen("tag \n");
-	item->tag = xmalloc(tag_len + 1);
-	memcpy(item->tag, tag_line + 4, tag_len);
-	item->tag[tag_len] = '\0';
-
-	if (!strcmp(type, blob_type)) {
-		item->tagged = &lookup_blob(sha1)->object;
-	} else if (!strcmp(type, tree_type)) {
-		item->tagged = &lookup_tree(sha1)->object;
-	} else if (!strcmp(type, commit_type)) {
-		item->tagged = &lookup_commit(sha1)->object;
-	} else if (!strcmp(type, tag_type)) {
-		item->tagged = &lookup_tag(sha1)->object;
-	} else {
-		error("Unknown type %s", type);
-		item->tagged = NULL;
-	}
-
-	if (item->tagged && track_object_refs) {
-		struct object_refs *refs = alloc_object_refs(1);
-		refs->ref[0] = item->tagged;
-		set_object_refs(&item->object, refs);
+	if (item) {
+		tag_len = tagger_line - tag_line - strlen("tag \n");
+		item->tag = xmalloc(tag_len + 1);
+		memcpy(item->tag, tag_line + 4, tag_len);
+		item->tag[tag_len] = '\0';
+
+		if (!strcmp(type, blob_type)) {
+			item->tagged = &lookup_blob(sha1)->object;
+		} else if (!strcmp(type, tree_type)) {
+			item->tagged = &lookup_tree(sha1)->object;
+		} else if (!strcmp(type, commit_type)) {
+			item->tagged = &lookup_commit(sha1)->object;
+		} else if (!strcmp(type, tag_type)) {
+			item->tagged = &lookup_tag(sha1)->object;
+		} else {
+			error("Unknown type %s", type);
+			item->tagged = NULL;
+		}
+
+		if (item->tagged && track_object_refs) {
+			struct object_refs *refs = alloc_object_refs(1);
+			refs->ref[0] = item->tagged;
+			set_object_refs(&item->object, refs);
+		}
 	}
 
 	return 0;
-- 
1.5.2

[PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/mktag.c b/mktag.c
index 4dbefab..2e70504 100644
--- a/mktag.c
+++ b/mktag.c
@@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)
 	memcpy(type, type_line + 5, type_len);
 	type[type_len] = '\0';
 
-	/* Verify that the object matches */
-	if (verify_object(sha1, type))
-		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
-
-	/* Verify the tag-name: we don't allow control characters or spaces in it */
-	tag_line += 4;
-	for (;;) {
-		unsigned char c = *tag_line++;
-		if (c == '\n')
-			break;
-		if (c > ' ')
-			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - data);
+	{
+		unsigned long i;
+
+		/* Verify that the object matches */
+		if (verify_object(sha1, type))
+			return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
+
+		/* Verify the tag-name: we don't allow control characters or spaces in it */
+		for (i = 4;;) {
+			unsigned char c = tag_line[i++];
+			if (c == '\n')
+				break;
+			if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
+		}
 	}
 
 	/* TODO: check for committer info + blank line? */
-- 
1.5.2

[PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()

[Johan Herland]

Rename parse_tag_buffer_internal() to parse_and_verify_tag_buffer() since
it now does tag object verification as well.

Add a new parameter 'thorough_verify' for turning on/off the extra code
to be run when verifying tag objects (as opposed to general parsing).

verify_tag() and parse_and_verify_tag_buffer() are now functionally
equivalent, provided that parse_and_verify_tag_buffer() is called with
item == NULL and thorough_verification != 0.

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tag.c b/tag.c
index b134967..3896e45 100644
--- a/tag.c
+++ b/tag.c
@@ -33,7 +33,26 @@ struct tag *lookup_tag(const unsigned char *sha1)
         return (struct tag *) obj;
 }
 
-static int parse_tag_buffer_internal(struct tag *item, const char *data, const unsigned long size)
+/*
+ * We refuse to tag something we can't verify. Just because.
+ */
+static int verify_object(unsigned char *sha1, const char *expected_type)
+{
+	int ret = -1;
+	enum object_type type;
+	unsigned long size;
+	void *buffer = read_sha1_file(sha1, &type, &size);
+
+	if (buffer) {
+		if (type == type_from_string(expected_type))
+			ret = check_sha1_signature(sha1, buffer, size, expected_type);
+		free(buffer);
+	}
+	return ret;
+}
+
+static int parse_and_verify_tag_buffer(struct tag *item,
+		const char *data, const unsigned long size, int thorough_verify)
 {
 #ifdef NO_C99_FORMAT
 #define PD_FMT "%d"
@@ -79,6 +98,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 	tagger_line = strchr(tag_line, '\n');
 	if (!tagger_line++)
 		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
+	if (thorough_verify) {
+		if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+			return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
+	}
 
 	/* Get the actual type */
 	type_len = tag_line - type_line - strlen("type \n");
@@ -87,6 +110,29 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 	memcpy(type, type_line + 5, type_len);
 	type[type_len] = '\0';
 
+	if (thorough_verify) {
+		unsigned long i;
+
+		/* Verify that the object matches */
+		if (verify_object(sha1, type))
+			return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
+
+		/* Verify the tag-name: we don't allow control characters or spaces in it */
+		for (i = 4;;) {
+			unsigned char c = tag_line[i++];
+			if (c == '\n')
+				break;
+			if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
+		}
+
+		/* Verify the tagger line */
+		/* TODO: check for committer/tagger info */
+
+		/* The actual stuff afterwards we don't care about.. */
+	}
+
 	if (item) {
 		tag_len = tagger_line - tag_line - strlen("tag \n");
 		item->tag = xmalloc(tag_len + 1);
@@ -120,7 +166,7 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
 
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
-	return parse_tag_buffer_internal(item, (const char *) data, size);
+	return parse_and_verify_tag_buffer(item, (const char *) data, size, 0);
 }
 
 int parse_tag(struct tag *item)
-- 
1.5.2

[PATCH 08/21] Switch from verify_tag() to parse_and_verify_tag_buffer() for verifying tag objects in git-mktag

[Johan Herland]

This involves exposing parse_and_verify_tag_buffer() in the tag API
(tag.h).

Also synchronize selftest with change in error message.

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c          |    5 ++---
 t/t3800-mktag.sh |    2 +-
 tag.c            |    2 +-
 tag.h            |    2 ++
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/mktag.c b/mktag.c
index 2e70504..5780f33 100644
--- a/mktag.c
+++ b/mktag.c
@@ -125,9 +125,8 @@ int main(int argc, char **argv)
 	}
 	buffer[size] = 0;
 
-	/* Verify it for some basic sanity: it needs to start with
-	   "object <sha1>\ntype\ntagger " */
-	if (verify_tag(buffer, size) < 0)
+	/* Verify tag object data */
+	if (parse_and_verify_tag_buffer(0, buffer, size, 1))
 		die("invalid tag signature file");
 
 	if (write_sha1_file(buffer, size, tag_type, result_sha1) < 0)
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index b4edb4d..0e87d2a 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -34,7 +34,7 @@ too short for a tag
 EOF
 
 cat >expect.pat <<EOF
-^error: .*size wrong.*$
+^error: .*size.*$
 EOF
 
 check_verify_failure 'Tag object length check'
diff --git a/tag.c b/tag.c
index 3896e45..e12b9fc 100644
--- a/tag.c
+++ b/tag.c
@@ -51,7 +51,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 	return ret;
 }
 
-static int parse_and_verify_tag_buffer(struct tag *item,
+int parse_and_verify_tag_buffer(struct tag *item,
 		const char *data, const unsigned long size, int thorough_verify)
 {
 #ifdef NO_C99_FORMAT
diff --git a/tag.h b/tag.h
index 7a0cb00..f341b7f 100644
--- a/tag.h
+++ b/tag.h
@@ -13,6 +13,8 @@ struct tag {
 };
 
 extern struct tag *lookup_tag(const unsigned char *sha1);
+extern int parse_and_verify_tag_buffer(struct tag *item,
+	const char *data, const unsigned long size, int thorough_verify);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
-- 
1.5.2

[PATCH 09/21] Remove unneeded code from mktag.c

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   94 ---------------------------------------------------------------
 1 files changed, 0 insertions(+), 94 deletions(-)

diff --git a/mktag.c b/mktag.c
index 5780f33..4f226ab 100644
--- a/mktag.c
+++ b/mktag.c
@@ -14,100 +14,6 @@
  * shortest single-character-tag line.
  */
 
-/*
- * We refuse to tag something we can't verify. Just because.
- */
-static int verify_object(unsigned char *sha1, const char *expected_type)
-{
-	int ret = -1;
-	enum object_type type;
-	unsigned long size;
-	void *buffer = read_sha1_file(sha1, &type, &size);
-
-	if (buffer) {
-		if (type == type_from_string(expected_type))
-			ret = check_sha1_signature(sha1, buffer, size, expected_type);
-		free(buffer);
-	}
-	return ret;
-}
-
-static int verify_tag(char *data, unsigned long size)
-{
-#ifdef NO_C99_FORMAT
-#define PD_FMT "%d"
-#else
-#define PD_FMT "%td"
-#endif
-
-	unsigned char sha1[20];
-	char type[20];
-	const char *type_line, *tag_line, *tagger_line;
-	unsigned long type_len;
-
-	if (size < 64)
-		return error("wanna fool me ? you obviously got the size wrong !");
-
-	/* Verify object line */
-	if (memcmp(data, "object ", 7))
-		return error("char%d: does not start with \"object \"", 0);
-
-	if (get_sha1_hex(data + 7, sha1))
-		return error("char%d: could not get SHA1 hash", 7);
-
-	/* Verify type line */
-	type_line = data + 48;
-	if (memcmp(type_line - 1, "\ntype ", 6))
-		return error("char%d: could not find \"\\ntype \"", 47);
-
-	/* Verify tag-line */
-	tag_line = strchr(type_line, '\n');
-	if (!tag_line++)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
-
-	/* Verify the tagger line */
-	tagger_line = strchr(tag_line, '\n');
-	if (!tagger_line++)
-		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
-	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
-		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
-
-	/* Get the actual type */
-	type_len = tag_line - type_line - strlen("type \n");
-	if (type_len >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line + 5 - data);
-	memcpy(type, type_line + 5, type_len);
-	type[type_len] = '\0';
-
-	{
-		unsigned long i;
-
-		/* Verify that the object matches */
-		if (verify_object(sha1, type))
-			return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
-
-		/* Verify the tag-name: we don't allow control characters or spaces in it */
-		for (i = 4;;) {
-			unsigned char c = tag_line[i++];
-			if (c == '\n')
-				break;
-			if (c > ' ')
-				continue;
-			return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
-		}
-	}
-
-	/* TODO: check for committer info + blank line? */
-	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
-
-	/* The actual stuff afterwards we don't care about.. */
-	return 0;
-
-#undef PD_FMT
-}
-
 int main(int argc, char **argv)
 {
 	unsigned long size = 4096;
-- 
1.5.2

[PATCH 10/21] Free mktag's buffer before dying

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/mktag.c b/mktag.c
index 4f226ab..31eadd8 100644
--- a/mktag.c
+++ b/mktag.c
@@ -21,7 +21,7 @@ int main(int argc, char **argv)
 	unsigned char result_sha1[20];
 
 	if (argc != 1)
-		usage("git-mktag < signaturefile");
+		usage("git-mktag < tag_data_file");
 
 	setup_git_directory();
 
@@ -32,14 +32,17 @@ int main(int argc, char **argv)
 	buffer[size] = 0;
 
 	/* Verify tag object data */
-	if (parse_and_verify_tag_buffer(0, buffer, size, 1))
-		die("invalid tag signature file");
+	if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
+		free(buffer);
+		die("invalid tag data file");
+	}
 
-	if (write_sha1_file(buffer, size, tag_type, result_sha1) < 0)
+	if (write_sha1_file(buffer, size, tag_type, result_sha1) < 0) {
+		free(buffer);
 		die("unable to write tag file");
+	}
 
 	free(buffer);
-
 	printf("%s\n", sha1_to_hex(result_sha1));
 	return 0;
 }
-- 
1.5.2

[PATCH 11/21] Rewrite error messages; fix up line lengths

[Johan Herland]

Also update selftests to reflect new error messages.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3800-mktag.sh |   22 +++++++++++-----------
 tag.c            |   44 +++++++++++++++++++++++++++++---------------
 2 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 0e87d2a..3bce5e0 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -49,7 +49,7 @@ tag mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: char0: .*"object "$
+^error: .*char 0.*"object ".*$
 EOF
 
 check_verify_failure '"object" line label check'
@@ -64,7 +64,7 @@ tag mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: char7: .*SHA1 hash$
+^error: .*char 7.*SHA1 hash.*$
 EOF
 
 check_verify_failure '"object" line SHA1 check'
@@ -79,7 +79,7 @@ tag mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: char47: .*"[\]ntype "$
+^error: .*char 47.*"[\]ntype ".*$
 EOF
 
 check_verify_failure '"type" line label check'
@@ -91,7 +91,7 @@ echo "object 779e9b33986b1c2670fff52c5067603117b3e895" >tag.sig
 printf "type tagsssssssssssssssssssssssssssssss" >>tag.sig
 
 cat >expect.pat <<EOF
-^error: char48: .*"[\]n"$
+^error: .*char 48.*"[\]n".*$
 EOF
 
 check_verify_failure '"type" line eol check'
@@ -106,7 +106,7 @@ xxx mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: char57: no "tag " found$
+^error: .*char 57.*"tag ".*$
 EOF
 
 check_verify_failure '"tag" line label check #1'
@@ -121,7 +121,7 @@ tag
 EOF
 
 cat >expect.pat <<EOF
-^error: char87: no "tag " found$
+^error: .*char 87.*"tag ".*$
 EOF
 
 check_verify_failure '"tag" line label check #2'
@@ -137,7 +137,7 @@ tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char53: type too long$
+^error: .*char 53.*Type too long.*$
 EOF
 
 check_verify_failure '"type" line type-name length check'
@@ -153,7 +153,7 @@ tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char7: could not verify object.*$
+^error: .*char 7.*Could not verify tagged object.*$
 EOF
 
 check_verify_failure 'verify object (SHA1/type) check'
@@ -169,7 +169,7 @@ tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char67: could not verify tag name$
+^error: .*char 67.*Could not verify tag name.*$
 EOF
 
 check_verify_failure 'verify tag-name check'
@@ -184,7 +184,7 @@ tag mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: char70: could not find "tagger"$
+^error: .*char 70.*Could not find "tagger ".*$
 EOF
 
 check_verify_failure '"tagger" line label check #1'
@@ -200,7 +200,7 @@ tagger
 EOF
 
 cat >expect.pat <<EOF
-^error: char70: could not find "tagger"$
+^error: .*char 70.*Could not find "tagger ".*$
 EOF
 
 check_verify_failure '"tagger" line label check #2'
diff --git a/tag.c b/tag.c
index e12b9fc..c7c75b3 100644
--- a/tag.c
+++ b/tag.c
@@ -72,41 +72,50 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	}
 
 	if (size < 64)
-		return error("failed preliminary size check");
+		return error("Tag object failed preliminary size check");
 
 	/* Verify object line */
 	if (memcmp(data, "object ", 7))
-		return error("char%d: does not start with \"object \"", 0);
+		return error("Tag object (@ char 0): "
+			"Does not start with \"object \"");
 
 	if (get_sha1_hex(data + 7, sha1))
-		return error("char%d: could not get SHA1 hash", 7);
+		return error("Tag object (@ char 7): Could not get SHA1 hash");
 
 	/* Verify type line */
 	type_line = data + 48;
 	if (memcmp(type_line - 1, "\ntype ", 6))
-		return error("char%d: could not find \"\\ntype \"", 47);
+		return error("Tag object (@ char 47): "
+			"Could not find \"\\ntype \"");
 
 	/* Verify tag-line */
 	tag_line = strchr(type_line, '\n');
-	if (!tag_line)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
-	tag_line++;
+	if (!tag_line++)
+		return error("Tag object (@ char " PD_FMT "): "
+			"Could not find \"\\n\" after \"type\"",
+			type_line - data);
 	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
+		return error("Tag object (@ char " PD_FMT "): "
+			"Could not find \"tag \"", tag_line - data);
 
 	/* Verify the tagger line */
 	tagger_line = strchr(tag_line, '\n');
 	if (!tagger_line++)
-		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
+		return error("Tag object (@ char " PD_FMT "): "
+			"Could not find \"\\n\" after \"tag\"",
+			tag_line - data);
 	if (thorough_verify) {
 		if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
-			return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - data);
+			return error("Tag object (@ char " PD_FMT "): "
+				"Could not find \"tagger \"",
+				tagger_line - data);
 	}
 
 	/* Get the actual type */
 	type_len = tag_line - type_line - strlen("type \n");
 	if (type_len >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line + 5 - data);
+		return error("Tag object (@ char " PD_FMT "): "
+			"Type too long", type_line + 5 - data);
 	memcpy(type, type_line + 5, type_len);
 	type[type_len] = '\0';
 
@@ -115,16 +124,20 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 		/* Verify that the object matches */
 		if (verify_object(sha1, type))
-			return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
+			return error("Tag object (@ char 7): "
+				"Could not verify tagged object %s",
+				sha1_to_hex(sha1));
 
-		/* Verify the tag-name: we don't allow control characters or spaces in it */
+		/* Verify tag name: disallow control characters or spaces */
 		for (i = 4;;) {
 			unsigned char c = tag_line[i++];
 			if (c == '\n')
 				break;
 			if (c > ' ')
 				continue;
-			return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
+			return error("Tag object (@ char " PD_FMT "): "
+				"Could not verify tag name",
+				tag_line + i - data);
 		}
 
 		/* Verify the tagger line */
@@ -148,7 +161,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		} else if (!strcmp(type, tag_type)) {
 			item->tagged = &lookup_tag(sha1)->object;
 		} else {
-			error("Unknown type %s", type);
+			error("Tag object (@ char " PD_FMT "): "
+				"Unknown type '%s'", type_line + 5 - data, type);
 			item->tagged = NULL;
 		}
 
-- 
1.5.2

[PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tag.c b/tag.c
index c7c75b3..ac76ec0 100644
--- a/tag.c
+++ b/tag.c
@@ -51,6 +51,13 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 	return ret;
 }
 
+/*
+ * Perform parsing and verification of tag object data.
+ *
+ * The 'item' parameter may be set to NULL if only verification is desired.
+ *
+ * The given data _must_ be null-terminated.
+ */
 int parse_and_verify_tag_buffer(struct tag *item,
 		const char *data, const unsigned long size, int thorough_verify)
 {
@@ -75,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		return error("Tag object failed preliminary size check");
 
 	/* Verify object line */
-	if (memcmp(data, "object ", 7))
+	if (prefixcmp(data, "object "))
 		return error("Tag object (@ char 0): "
 			"Does not start with \"object \"");
 
@@ -84,7 +91,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 	/* Verify type line */
 	type_line = data + 48;
-	if (memcmp(type_line - 1, "\ntype ", 6))
+	if (prefixcmp(type_line - 1, "\ntype "))
 		return error("Tag object (@ char 47): "
 			"Could not find \"\\ntype \"");
 
@@ -94,7 +101,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		return error("Tag object (@ char " PD_FMT "): "
 			"Could not find \"\\n\" after \"type\"",
 			type_line - data);
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
+	if (prefixcmp(tag_line, "tag ") || tag_line[4] == '\n')
 		return error("Tag object (@ char " PD_FMT "): "
 			"Could not find \"tag \"", tag_line - data);
 
@@ -105,7 +112,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 			"Could not find \"\\n\" after \"tag\"",
 			tag_line - data);
 	if (thorough_verify) {
-		if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+		if (prefixcmp(tagger_line, "tagger ") || (tagger_line[7] == '\n'))
 			return error("Tag object (@ char " PD_FMT "): "
 				"Could not find \"tagger \"",
 				tagger_line - data);
-- 
1.5.2

[PATCH 13/21] Collect skipping of header field names and calculation of line lengths in one place

[Johan Herland]

For each of the parsed lines we at some point skip past its initial
identifier ("type ", "tag ", etc.). We also at some point calculate the
length of the remaining line. This patch moves these calculations into
one place. This provides _one_ place for all header lines where their
respective pointers start pointing at the header value (instead of the
start of the line), and their lengths are calculated.

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/tag.c b/tag.c
index ac76ec0..9a6924f 100644
--- a/tag.c
+++ b/tag.c
@@ -118,12 +118,20 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				tagger_line - data);
 	}
 
+	/*
+	 * Advance header field pointers past their initial identifier.
+	 * Calculate lengths of header fields.
+	 */
+	type_line += strlen("type ");
+	type_len   = tag_line - type_line - 1;
+	tag_line  += strlen("tag ");
+	tag_len    = tagger_line - tag_line - 1;
+
 	/* Get the actual type */
-	type_len = tag_line - type_line - strlen("type \n");
 	if (type_len >= sizeof(type))
 		return error("Tag object (@ char " PD_FMT "): "
-			"Type too long", type_line + 5 - data);
-	memcpy(type, type_line + 5, type_len);
+			"Type too long", type_line - data);
+	memcpy(type, type_line, type_len);
 	type[type_len] = '\0';
 
 	if (thorough_verify) {
@@ -136,7 +144,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				sha1_to_hex(sha1));
 
 		/* Verify tag name: disallow control characters or spaces */
-		for (i = 4;;) {
+		for (i = 0;;) {
 			unsigned char c = tag_line[i++];
 			if (c == '\n')
 				break;
@@ -154,9 +162,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	}
 
 	if (item) {
-		tag_len = tagger_line - tag_line - strlen("tag \n");
 		item->tag = xmalloc(tag_len + 1);
-		memcpy(item->tag, tag_line + 4, tag_len);
+		memcpy(item->tag, tag_line, tag_len);
 		item->tag[tag_len] = '\0';
 
 		if (!strcmp(type, blob_type)) {
@@ -169,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 			item->tagged = &lookup_tag(sha1)->object;
 		} else {
 			error("Tag object (@ char " PD_FMT "): "
-				"Unknown type '%s'", type_line + 5 - data, type);
+				"Unknown type '%s'", type_line - data, type);
 			item->tagged = NULL;
 		}
 
-- 
1.5.2

[PATCH 14/21] Add proper parsing of "tagger" line, but only when thorough_verify is set

[Johan Herland]

Be explicit about the fact that the "tagger" line is now considered a
mandatory part of the tag object. There are however old tags (from before
July 2005) that don't have a "tagger" line. We therefore consider the
"tagger" line _optional_ when parsing tags without thorough_verify set.

In practice this means that verification of a missing "tagger" line will
only fail when adding or fsck-ing tags.

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   48 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/tag.c b/tag.c
index 9a6924f..c373c86 100644
--- a/tag.c
+++ b/tag.c
@@ -69,8 +69,9 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 	unsigned char sha1[20];
 	char type[20];
-	const char *type_line, *tag_line, *tagger_line;
-	unsigned long type_len, tag_len;
+	const char   *type_line, *tag_line, *tagger_line;
+	unsigned long type_len,   tag_len,   tagger_len;
+	const char *header_end;
 
 	if (item) {
 		if (item->object.parsed)
@@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	if (size < 64)
 		return error("Tag object failed preliminary size check");
 
-	/* Verify object line */
+	/* Verify mandatory object line */
 	if (prefixcmp(data, "object "))
 		return error("Tag object (@ char 0): "
 			"Does not start with \"object \"");
@@ -89,13 +90,13 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	if (get_sha1_hex(data + 7, sha1))
 		return error("Tag object (@ char 7): Could not get SHA1 hash");
 
-	/* Verify type line */
+	/* Verify mandatory type line */
 	type_line = data + 48;
 	if (prefixcmp(type_line - 1, "\ntype "))
 		return error("Tag object (@ char 47): "
 			"Could not find \"\\ntype \"");
 
-	/* Verify tag-line */
+	/* Verify mandatory tag line */
 	tag_line = strchr(type_line, '\n');
 	if (!tag_line++)
 		return error("Tag object (@ char " PD_FMT "): "
@@ -105,27 +106,46 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		return error("Tag object (@ char " PD_FMT "): "
 			"Could not find \"tag \"", tag_line - data);
 
-	/* Verify the tagger line */
+	/*
+	 * Verify mandatory tagger line, but only when we're checking
+	 * thoroughly, i.e. on inserting a new tag, and on fsck.
+	 * There are existing tag objects without a tagger line (most
+	 * notably the "v0.99" tag in the main git repo), and we don't
+	 * want to fail parsing on these.
+	 */
 	tagger_line = strchr(tag_line, '\n');
 	if (!tagger_line++)
 		return error("Tag object (@ char " PD_FMT "): "
 			"Could not find \"\\n\" after \"tag\"",
 			tag_line - data);
-	if (thorough_verify) {
-		if (prefixcmp(tagger_line, "tagger ") || (tagger_line[7] == '\n'))
+	if (prefixcmp(tagger_line, "tagger ")) { /* no tagger given */
+		if (thorough_verify)
 			return error("Tag object (@ char " PD_FMT "): "
 				"Could not find \"tagger \"",
 				tagger_line - data);
+		header_end = tagger_line;
+	}
+	else {                                   /* tagger given */
+		header_end = strchr(tagger_line, '\n');
+		if (!header_end++)
+			return error("Tag object (@ char " PD_FMT "): "
+				"Could not find \"\\n\" after \"tagger\"",
+				tagger_line - data);
 	}
 
 	/*
 	 * Advance header field pointers past their initial identifier.
-	 * Calculate lengths of header fields.
+	 * Calculate lengths of header fields (0 for fields that are not given).
 	 */
-	type_line += strlen("type ");
-	type_len   = tag_line - type_line - 1;
-	tag_line  += strlen("tag ");
-	tag_len    = tagger_line - tag_line - 1;
+	type_line     += strlen("type ");
+	type_len       = tag_line > type_line ?
+			(tag_line - type_line) - 1 : 0;
+	tag_line      += strlen("tag ");
+	tag_len        = tagger_line > tag_line ?
+			(tagger_line - tag_line) - 1 : 0;
+	tagger_line   += strlen("tagger ");
+	tagger_len     = header_end > tagger_line ?
+			(header_end - tagger_line) - 1 : 0;
 
 	/* Get the actual type */
 	if (type_len >= sizeof(type))
@@ -155,7 +175,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				tag_line + i - data);
 		}
 
-		/* Verify the tagger line */
+		/* Verify tagger line */
 		/* TODO: check for committer/tagger info */
 
 		/* The actual stuff afterwards we don't care about.. */
-- 
1.5.2

[PATCH 15/21] Make tag names (i.e. the tag object's "tag" line) optional

[Johan Herland]

The tag line is now optional. If not given in the tag object data, it
defaults to the empty string ("") in the parsed tag object.

The patch also adds a change to git-show; when asked to display a tag
object with no name (missing "tag" header), we will show the tag's sha1
instead of an empty string.

Finally the patch includes some tweaks to the selftests to make them work
with optional tag names.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-log.c    |    2 +-
 t/t3800-mktag.sh |    6 +++---
 tag.c            |   51 +++++++++++++++++++++++++++++----------------------
 tag.h            |    2 +-
 4 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 212cdfc..8a238c7 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -181,7 +181,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 			printf("%stag %s%s\n\n",
 					diff_get_color(rev.diffopt.color_diff,
 						DIFF_COMMIT),
-					t->tag,
+					*(t->tag) ? t->tag : name,
 					diff_get_color(rev.diffopt.color_diff,
 						DIFF_RESET));
 			ret = show_object(o->sha1, 1);
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 3bce5e0..3381239 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -106,7 +106,7 @@ xxx mytag
 EOF
 
 cat >expect.pat <<EOF
-^error: .*char 57.*"tag ".*$
+^error: .*char 57.*$
 EOF
 
 check_verify_failure '"tag" line label check #1'
@@ -121,7 +121,7 @@ tag
 EOF
 
 cat >expect.pat <<EOF
-^error: .*char 87.*"tag ".*$
+^error: .*char 87.*$
 EOF
 
 check_verify_failure '"tag" line label check #2'
@@ -169,7 +169,7 @@ tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: .*char 67.*Could not verify tag name.*$
+^error: .*char 66.*Could not verify tag name.*$
 EOF
 
 check_verify_failure 'verify tag-name check'
diff --git a/tag.c b/tag.c
index c373c86..fb678d7 100644
--- a/tag.c
+++ b/tag.c
@@ -96,15 +96,21 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		return error("Tag object (@ char 47): "
 			"Could not find \"\\ntype \"");
 
-	/* Verify mandatory tag line */
+	/* Verify optional tag line */
 	tag_line = strchr(type_line, '\n');
 	if (!tag_line++)
 		return error("Tag object (@ char " PD_FMT "): "
 			"Could not find \"\\n\" after \"type\"",
 			type_line - data);
-	if (prefixcmp(tag_line, "tag ") || tag_line[4] == '\n')
-		return error("Tag object (@ char " PD_FMT "): "
-			"Could not find \"tag \"", tag_line - data);
+	if (prefixcmp(tag_line, "tag ")) /* no tag name given */
+		tagger_line = tag_line;
+	else {                           /* tag name given */
+		tagger_line = strchr(tag_line, '\n');
+		if (!tagger_line++)
+			return error("Tag object (@ char " PD_FMT "): "
+				"Could not find \"\\n\" after \"tag\"",
+				tag_line - data);
+	}
 
 	/*
 	 * Verify mandatory tagger line, but only when we're checking
@@ -113,11 +119,6 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	 * notably the "v0.99" tag in the main git repo), and we don't
 	 * want to fail parsing on these.
 	 */
-	tagger_line = strchr(tag_line, '\n');
-	if (!tagger_line++)
-		return error("Tag object (@ char " PD_FMT "): "
-			"Could not find \"\\n\" after \"tag\"",
-			tag_line - data);
 	if (prefixcmp(tagger_line, "tagger ")) { /* no tagger given */
 		if (thorough_verify)
 			return error("Tag object (@ char " PD_FMT "): "
@@ -164,15 +165,15 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				sha1_to_hex(sha1));
 
 		/* Verify tag name: disallow control characters or spaces */
-		for (i = 0;;) {
-			unsigned char c = tag_line[i++];
-			if (c == '\n')
-				break;
-			if (c > ' ')
-				continue;
-			return error("Tag object (@ char " PD_FMT "): "
-				"Could not verify tag name",
-				tag_line + i - data);
+		if (tag_len) { /* tag name was given */
+			for (i = 0; i < tag_len; ++i) {
+				unsigned char c = tag_line[i];
+				if (c > ' ' && c != 0x7f)
+					continue;
+				return error("Tag object (@ char " PD_FMT "): "
+					"Could not verify tag name",
+					tag_line + i - data);
+			}
 		}
 
 		/* Verify tagger line */
@@ -181,10 +182,16 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		/* The actual stuff afterwards we don't care about.. */
 	}
 
-	if (item) {
-		item->tag = xmalloc(tag_len + 1);
-		memcpy(item->tag, tag_line, tag_len);
-		item->tag[tag_len] = '\0';
+	if (item) { /* Store parsed information into item */
+		if (tag_len) { /* optional tag name was given */
+			item->tag = xmalloc(tag_len + 1);
+			memcpy(item->tag, tag_line, tag_len);
+			item->tag[tag_len] = '\0';
+		}
+		else { /* optional tag name not given */
+			item->tag = xmalloc(1);
+			item->tag[0] = '\0';
+		}
 
 		if (!strcmp(type, blob_type)) {
 			item->tagged = &lookup_blob(sha1)->object;
diff --git a/tag.h b/tag.h
index f341b7f..2631911 100644
--- a/tag.h
+++ b/tag.h
@@ -8,7 +8,7 @@ extern const char *tag_type;
 struct tag {
 	struct object object;
 	struct object *tagged;
-	char *tag;
+	char *tag;       /* optional, may be empty ("") */
 	char *signature; /* not actually implemented */
 };
 
-- 
1.5.2

[PATCH 16/21] Introduce optional "keywords" on tag objects

[Johan Herland]

This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object. There are two "special"
keywords, however: "tag" and "note": When the "keywords" header is
missing, its default value is set to "tag" if a "tag" header is
present; else the default "keywords" value is set to "note". The
"keywords" header is meant to be used by porcelains for classifying
different types of tag objects. This classification may then be used to
filter tag objects in the presentation layer (e.g. by implementing
extra filter options to --decorate, etc.)

Signed-off-by: Johan Herland <johan@herland.net>
---
 tag.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 tag.h |    1 +
 2 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/tag.c b/tag.c
index fb678d7..1caec19 100644
--- a/tag.c
+++ b/tag.c
@@ -69,8 +69,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 	unsigned char sha1[20];
 	char type[20];
-	const char   *type_line, *tag_line, *tagger_line;
-	unsigned long type_len,   tag_len,   tagger_len;
+	const char   *type_line, *tag_line, *keywords_line, *tagger_line;
+	unsigned long type_len,   tag_len,   keywords_len,   tagger_len;
 	const char *header_end;
 
 	if (item) {
@@ -103,15 +103,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
 			"Could not find \"\\n\" after \"type\"",
 			type_line - data);
 	if (prefixcmp(tag_line, "tag ")) /* no tag name given */
-		tagger_line = tag_line;
+		keywords_line = tag_line;
 	else {                           /* tag name given */
-		tagger_line = strchr(tag_line, '\n');
-		if (!tagger_line++)
+		keywords_line = strchr(tag_line, '\n');
+		if (!keywords_line++)
 			return error("Tag object (@ char " PD_FMT "): "
 				"Could not find \"\\n\" after \"tag\"",
 				tag_line - data);
 	}
 
+	/* Verify optional keywords line */
+	if (prefixcmp(keywords_line, "keywords ")) /* no keywords given */
+		tagger_line = keywords_line;
+	else {                                     /* keywords given */
+		tagger_line = strchr(keywords_line, '\n');
+		if (!tagger_line++)
+			return error("Tag object (@ char " PD_FMT "): "
+				"Could not find \"\\n\" after \"keywords\"",
+				keywords_line - data);
+	}
+
 	/*
 	 * Verify mandatory tagger line, but only when we're checking
 	 * thoroughly, i.e. on inserting a new tag, and on fsck.
@@ -142,8 +153,11 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	type_len       = tag_line > type_line ?
 			(tag_line - type_line) - 1 : 0;
 	tag_line      += strlen("tag ");
-	tag_len        = tagger_line > tag_line ?
-			(tagger_line - tag_line) - 1 : 0;
+	tag_len        = keywords_line > tag_line ?
+			(keywords_line - tag_line) - 1 : 0;
+	keywords_line += strlen("keywords ");
+	keywords_len   = tagger_line > keywords_line ?
+			(tagger_line - keywords_line) - 1 : 0;
 	tagger_line   += strlen("tagger ");
 	tagger_len     = header_end > tagger_line ?
 			(header_end - tagger_line) - 1 : 0;
@@ -176,6 +190,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
 			}
 		}
 
+		/*
+		 * Verify keywords: disallow control characters, spaces,
+		 * or two subsequent commas
+		 */
+		if (keywords_len) { /* keywords line was given */
+			for (i = 0; i < keywords_len; ++i) {
+				unsigned char c = keywords_line[i];
+				if (c == ',' && keywords_line[i + 1] == ',')
+					/* consecutive commas */
+					return error("Tag object (@ char "
+						PD_FMT "): Found empty keyword",
+						keywords_line + i - data);
+				if (c > ' ' && c != 0x7f)
+					continue;
+				return error("Tag object (@ char " PD_FMT "): "
+					"Could not verify keywords",
+					keywords_line + i - data);
+			}
+		}
+
 		/* Verify tagger line */
 		/* TODO: check for committer/tagger info */
 
@@ -193,6 +227,18 @@ int parse_and_verify_tag_buffer(struct tag *item,
 			item->tag[0] = '\0';
 		}
 
+		if (keywords_len) { /* optional keywords string was given */
+			item->keywords = xmalloc(keywords_len + 1);
+			memcpy(item->keywords, keywords_line, keywords_len);
+			item->keywords[keywords_len] = '\0';
+		}
+		else { /* optional keywords string not given. Set default */
+			/* if tag name is set, use "tag"; else use "note" */
+			const char *default_kw = item->tag ? "tag" : "note";
+			item->keywords = xmalloc(strlen(default_kw) + 1);
+			memcpy(item->keywords, default_kw, strlen(default_kw) + 1);
+		}
+
 		if (!strcmp(type, blob_type)) {
 			item->tagged = &lookup_blob(sha1)->object;
 		} else if (!strcmp(type, tree_type)) {
diff --git a/tag.h b/tag.h
index 2631911..3b0008d 100644
--- a/tag.h
+++ b/tag.h
@@ -9,6 +9,7 @@ struct tag {
 	struct object object;
 	struct object *tagged;
 	char *tag;       /* optional, may be empty ("") */
+	char *keywords;  /* optional, defaults to tag ? "tag" : "note" */
 	char *signature; /* not actually implemented */
 };
 
-- 
1.5.2

[PATCH 17/21] Update comments on tag objects in mktag.c

[Johan Herland]

Also update minimum tag object length to the new minimum length after refactoring.

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   30 ++++++++++++++++++++++--------
 tag.c   |    2 +-
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/mktag.c b/mktag.c
index 31eadd8..af0cfa6 100644
--- a/mktag.c
+++ b/mktag.c
@@ -2,16 +2,30 @@
 #include "tag.h"
 
 /*
- * A signature file has a very simple fixed format: four lines
- * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
+ * Tag object data has the following format: two mandatory lines of
+ * "object <sha1>" + "type <typename>", plus two optional lines of
+ * "tag <tagname>" + "keywords <keywords>", plus a mandatory line of
  * "tagger <committer>", followed by a blank line, a free-form tag
- * message and a signature block that git itself doesn't care about,
- * but that can be verified with gpg or similar.
+ * message and an optional signature block that git itself doesn't
+ * care about, but that can be verified with gpg or similar.
  *
- * The first three lines are guaranteed to be at least 63 bytes:
- * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, and "tag .\n" at 6 bytes is the
- * shortest single-character-tag line.
+ * <sha1> represents the object pointed to by this tag, <typename> is
+ * the type of the object pointed to ("tag", "blob", "tree" or "commit"),
+ * <tagname> is the name of this tag object (and must correspond to the
+ * name of the corresponding ref (if any) in '.git/refs/'). <keywords> is
+ * a comma-separated list of keywords associated with this tag object, and
+ * <committer> holds the "name <email>" of the tag creator and timestamp
+ * of when the tag object was created (analogous to "committer" in commit
+ * objects).
+ *
+ * The first two lines are guaranteed to be at least 57 bytes:
+ * "object <sha1>\n" is 48 bytes, and "type tag\n" at 9 bytes is
+ * the shortest possible "type" line. The tagger line is at least
+ * "tagger \n" (8 bytes). Therefore a tag object _must_ have >= 65 bytes.
+ *
+ * If "tag <tagname>" is omitted, <tagname> defaults to the empty string.
+ * If "keywords <keywords>" is omitted, <keywords> defaults to "tag" if
+ * a <tagname> was given, "note" otherwise.
  */
 
 int main(int argc, char **argv)
diff --git a/tag.c b/tag.c
index 1caec19..af4356e 100644
--- a/tag.c
+++ b/tag.c
@@ -79,7 +79,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		item->object.parsed = 1;
 	}
 
-	if (size < 64)
+	if (size < 65)
 		return error("Tag object failed preliminary size check");
 
 	/* Verify mandatory object line */
-- 
1.5.2

[PATCH 18/21] git-fsck: Do thorough verification of tag objects

[Johan Herland]

Teach git-fsck to do the same kind of verification on tag objects that is
already done by git-mktag.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-fsck.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 944a496..7b4c36b 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -359,11 +359,26 @@ static int fsck_commit(struct commit *commit)
 static int fsck_tag(struct tag *tag)
 {
 	struct object *tagged = tag->tagged;
+	enum object_type type;
+	unsigned long size;
+	char *data = (char *) read_sha1_file(tag->object.sha1, &type, &size);
 
 	if (verbose)
 		fprintf(stderr, "Checking tag %s\n",
 			sha1_to_hex(tag->object.sha1));
 
+	if (!data)
+		return objerror(&tag->object, "could not read tag");
+	if (type != OBJ_TAG) {
+		free(data);
+		return objerror(&tag->object, "not a tag (internal error)");
+	}
+	if (parse_and_verify_tag_buffer(0, data, size, 1)) { /* thoroughly verify tag object */
+		free(data);
+		return objerror(&tag->object, "failed thorough tag object verification");
+	}
+	free(data);
+
 	if (!tagged) {
 		return objerror(&tag->object, "could not load tagged object");
 	}
-- 
1.5.2

[PATCH 19/21] Documentation/git-mktag: Document the changes in tag object structure

[Johan Herland]

The new structure of tag objects is documented.

Also some much-needed cleanup is done. E.g. remove the paragraph on the
8kB limit, since this limit was removed ages ago.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-mktag.txt |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt
index 0ac3be1..411105d 100644
--- a/Documentation/git-mktag.txt
+++ b/Documentation/git-mktag.txt
@@ -8,38 +8,57 @@ git-mktag - Creates a tag object
 
 SYNOPSIS
 --------
-'git-mktag' < signature_file
+[verse]
+'git-mktag' < tag_data_file
+
 
 DESCRIPTION
 -----------
-Reads a tag contents on standard input and creates a tag object
+Reads tag object data on standard input and creates a tag object
 that can also be used to sign other objects.
 
 The output is the new tag's <object> identifier.
 
-Tag Format
+
+DISCUSSION
 ----------
-A tag signature file has a very simple fixed format: three lines of
+Tag object data has the following format
 
+[verse]
   object <sha1>
   type <typename>
-  tag <tagname>
+  tag <tagname>               (optional)
+  keywords <keywords>         (optional)
+  tagger <committer>
+
+followed by a blank line and a free-form message and an optional signature
+that git itself doesn't care about, but that may be verified with gpg or
+similar.
 
-followed by some 'optional' free-form signature that git itself
-doesn't care about, but that can be verified with gpg or similar.
+In the above listing, `<sha1>` represents the object pointed to by this tag,
+`<typename>` is the type of the object pointed to ("tag", "blob", "tree" or
+"commit"), `<tagname>` is the name of this tag object (and must correspond
+to the name of the corresponding ref (if any) in `.git/refs/`). `<keywords>`
+is a comma-separated list of keywords associated with this tag object, and
+`<committer>` holds the "`name <email>`" of the tag creator and timestamp
+of when the tag object was created (analogous to "committer" in commit
+objects).
 
-The size of the full object is artificially limited to 8kB.  (Just
-because I'm a lazy bastard, and if you can't fit a signature in that
-size, you're doing something wrong)
+If "`tag <tagname>`" is omitted, <tagname> defaults to the empty string.
+If "`keywords <keywords>`" is omitted, <keywords> defaults to "`tag`" if
+a <tagname> was given, "`note`" otherwise.
 
 
 Author
 ------
 Written by Linus Torvalds <torvalds@osdl.org>
 
+
 Documentation
 --------------
-Documentation by David Greaves, Junio C Hamano and the git-list <git@vger.kernel.org>.
+Documentation by Johan Herland, David Greaves, Junio C Hamano and the
+git-list <git@vger.kernel.org>.
+
 
 GIT
 ---
-- 
1.5.2

[PATCH 20/21] git-mktag tests: Expand on mktag selftests according to the new tag object structure

[Johan Herland]

Some more tests are added to test the new "keywords" header, and to test
the more thorough verification routine.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3800-mktag.sh |  212 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 200 insertions(+), 12 deletions(-)

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 3381239..b3b3121 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -46,6 +46,8 @@ cat >tag.sig <<EOF
 xxxxxx 139e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger foo
+
 EOF
 
 cat >expect.pat <<EOF
@@ -61,6 +63,8 @@ cat >tag.sig <<EOF
 object zz9e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger foo
+
 EOF
 
 cat >expect.pat <<EOF
@@ -76,6 +80,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 xxxx tag
 tag mytag
+tagger foo
+
 EOF
 
 cat >expect.pat <<EOF
@@ -103,6 +109,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tag
 xxx mytag
+tagger foo
+
 EOF
 
 cat >expect.pat <<EOF
@@ -118,6 +126,9 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type taggggggggggggggggggggggggggggggg
 tag
+keywords foo
+tagger bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
@@ -127,13 +138,15 @@ EOF
 check_verify_failure '"tag" line label check #2'
 
 ############################################################
-#  8. type line type-name length check
+#  8. type line type name length check
 
 cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type taggggggggggggggggggggggggggggggg
 tag mytag
-tagger a
+keywords foo
+tagger bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
@@ -149,7 +162,9 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tagggg
 tag mytag
-tagger a
+keywords foo
+tagger bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
@@ -159,13 +174,15 @@ EOF
 check_verify_failure 'verify object (SHA1/type) check'
 
 ############################################################
-# 10. verify tag-name check
+# 10. verify tag name check
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag my	tag
-tagger a
+keywords foo
+tagger bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
@@ -175,28 +192,120 @@ EOF
 check_verify_failure 'verify tag-name check'
 
 ############################################################
-# 11. tagger line label check #1
+# 11. keywords line label check #1
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
+xxxxxxxx foo
+tagger bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
-^error: .*char 70.*Could not find "tagger ".*$
+^error: .*char 70.*$
+EOF
+
+check_verify_failure '"keywords" line label check #1'
+
+############################################################
+# 12. keywords line label check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 70.*$
+EOF
+
+check_verify_failure '"keywords" line label check #2'
+
+############################################################
+# 13. keywords line check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 82.*$
+EOF
+
+check_verify_failure '"keywords" line check #1'
+
+############################################################
+# 14. keywords line check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 86.*$
+EOF
+
+check_verify_failure '"keywords" line check #2'
+
+############################################################
+# 15. keywords line check #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,,bar
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 82.*Found empty keyword.*$
+EOF
+
+check_verify_failure '"keywords" line check #3'
+
+############################################################
+# 16. tagger line label check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 70.*$
 EOF
 
 check_verify_failure '"tagger" line label check #1'
 
 ############################################################
-# 12. tagger line label check #2
+# 17. tagger line label check #2
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
-tagger
+xxxxxx bar@baz.com
+
 EOF
 
 cat >expect.pat <<EOF
@@ -206,25 +315,104 @@ EOF
 check_verify_failure '"tagger" line label check #2'
 
 ############################################################
-# 13. create valid tag
+# 18. tagger line label check #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo
+tagger
+
+EOF
+
+cat >expect.pat <<EOF
+^error: .*char 83.*$
+EOF
+
+check_verify_failure '"tagger" line label check #3'
+
+############################################################
+# 19. create valid tag #1
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
 tagger another@example.com
+
 EOF
 
 test_expect_success \
-    'create valid tag' \
+    'create valid tag #1' \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 14. check mytag
+# 20. check mytag
 
 test_expect_success \
     'check mytag' \
     'git-tag -l | grep mytag'
 
+############################################################
+# 21. create valid tag #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #2' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 22. create valid tag #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+keywords foo,bar,baz,spam,spam,spam,spam,spam,spam,spam,spam
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #3' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 23. create valid tag #4
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 24. create valid tag #5 (with empty message)
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger a
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
 
 test_done
-- 
1.5.2

[PATCH 21/21] Add fsck_verify_ref_to_tag_object() to verify that refname matches name stored in tag object

[Johan Herland]

On Monday 28 May 2007, Junio C Hamano wrote:
> However it would be a good
> idea to add logic to fsck to warn upon inconsistencis (perhaps
> by mistake) between refname and tag's true name.
>
> The check would say something like:
>
>   If an annotated (signed or unsigned) tag has a "tag"
>   line to give it the official $name, and if it is pointed
>   at by a ref, the refname must end with "/$name".
>   Otherwise we warn.
>
> Trivially, the above rule says that having v2.6.22 tag under
> refs/tags/v2.6.20 is a mistake we would want to be warned upon.

This patch adds the check described by Junio.

It also includes a bugfix when tag object parsing fails, from
Johannes Schindelin <Johannes.Schindelin@gmx.de>.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-fsck.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 7b4c36b..a47976c 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -542,6 +542,30 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in
 	return 0;
 }
 
+static void fsck_verify_ref_to_tag_object(const char *refname, struct object *obj)
+{
+	/* Verify that refname matches the name stored in obj's "tag" header */
+	struct tag *tagobj = (struct tag *) parse_object(obj->sha1);
+	size_t tagname_len;
+	size_t refname_len = strlen(refname);
+
+	if (!tagobj->tag) {
+		error("%s: Failed to parse tag object %s", refname, sha1_to_hex(obj->sha1));
+		return;
+	}
+	tagname_len = strlen(tagobj->tag);
+	if (!tagname_len) return; /* No tag name stored in tagobj. Nothing to do. */
+
+	if (tagname_len < refname_len &&
+		!memcmp(tagobj->tag, refname + (refname_len - tagname_len), tagname_len) &&
+		refname[(refname_len - tagname_len) - 1] == '/') {
+		/* OK: tag name is "$name", and refname ends with "/$name" */
+		return;
+	}
+	else
+		error("%s: Mismatch between tag ref and tag object's name: '%s'", refname, tagobj->tag);
+}
+
 static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	struct object *obj;
@@ -556,6 +580,8 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f
 		/* We'll continue with the rest despite the error.. */
 		return 0;
 	}
+	if (obj->type == OBJ_TAG) /* ref to tag object */
+		fsck_verify_ref_to_tag_object(refname, obj);
 	default_refs++;
 	obj->used = 1;
 	mark_reachable(obj, REACHABLE);
-- 
1.5.2

Re: [PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> diff --git a/mktag.c b/mktag.c
> index b82e377..0bc20c8 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
>  	return ret;
>  }
>  
> +static int verify_tag(char *data, unsigned long size)
> +{
>  #ifdef NO_C99_FORMAT
>  #define PD_FMT "%d"
>  #else
>  #define PD_FMT "%td"
>  #endif
>  
> -static int verify_tag(char *buffer, unsigned long size)

Why this rename from buffer to data?

> -{
> -	int typelen;
> -	char type[20];
>  	unsigned char sha1[20];
> -	const char *object, *type_line, *tag_line, *tagger_line;
> +	char type[20];
> +	const char *type_line, *tag_line, *tagger_line;
> +	unsigned long type_len;

Why this change of order?

>  	if (size < 64)
>  		return error("wanna fool me ? you obviously got the size wrong !");
>  
> -	buffer[size] = 0;

Are you sure that your buffer is always NUL terminated?

> -	type_line = object + 48;
> +	type_line = data + 48;

Quite a lot of changes seem to do this object->data. The patch would have 
been much more compact if you just had renamed buffer to object instead of 
data.

> -	typelen = tag_line - type_line - strlen("type \n");
> -	if (typelen >= sizeof(type))
> -		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
> -
> -	memcpy(type, type_line+5, typelen);
> -	type[typelen] = 0;
> +	type_len = tag_line - type_line - strlen("type \n");
> +	if (type_len >= sizeof(type))
> +		return error("char" PD_FMT ": type too long", type_line + 5 - data);
> +	memcpy(type, type_line + 5, type_len);
> +	type[type_len] = '\0';

This renaming variables has nothing to do with refactoring. In fact, I 
have a hard time to find code changes (which your subject suggests, as you 
want to make two functions more similar).

>  	/* TODO: check for committer info + blank line? */
>  	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
>  
>  	/* The actual stuff afterwards we don't care about.. */
>  	return 0;
> -}
>  
>  #undef PD_FMT
> +}

Any particular reason for this?

> @@ -124,6 +120,7 @@ int main(int argc, char **argv)
>  		free(buffer);
>  		die("could not read from stdin");
>  	}
> +	buffer[size] = 0;

Ah, so you terminate the buffer here. From the patch, it is relatively 
hard to see if this line is always hit _before_ the function is called 
that evidently relies on NUL termination. By moving it here, I think it is 
much more likely to overlook the fact that the function, which made sure 
that its assumption was met, needs this line. Whereas if you left it where 
it was, the assumption would always be met.

> -        if (item->object.parsed)
> -                return 0;
> -        item->object.parsed = 1;
> +	if (item->object.parsed)
> +		return 0;
> +	item->object.parsed = 1;

Again, this has nothing to do with refactoring.

> @@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
>  	if (memcmp(data, "object ", 7))
>  		return error("char%d: does not start with \"object \"", 0);
>  
> -	if (get_sha1_hex((char *) data + 7, sha1))
> +	if (get_sha1_hex(data + 7, sha1))

Is this really necessary? Even if (which I doubt), it has nothing to do 
with refactoring.

If you _want_ to _explicitely_ do arithmetic on a char* instead of void*, 
why not DRT and change the function signature?

> -	sig_line++;
> +	tagger_line++;

I am really reluctant with renamings like these. IMHO they don't buy you 
much, except for possible confusion. It is evident that sig means the 
signer (and it is obvious in the case of an unsigned tag, who is meant, 
too).

> +int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
> +{
> +	return parse_tag_buffer_internal(item, (const char *) data, size);
> +}

This cast (and indeed, this function, if you ask me) is unnecessary.

I reviewed only this patch out of your long series, mostly because I found 
the subject line interesting. But IMHO the patch does not what the subject 
line suggests.

Unfortunately, it's unlikely that I will have time until Monday night to 
continue with this patch series.

Ciao,
Dscho

Re: [PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar

[Johan Herland]

On Saturday 09 June 2007, Johannes Schindelin wrote:
> Hi,

Hi. Thanks for taking the time to look at (some of) my patch. Most of your
questions below can be answered with a single answer:

The main purpose of the patch is (as the subject line says) to bring the
two functions more in line with eachother. At the time I made the patch,
I had made the observation that these function were trying to do much the
same thing, albeit in a slightly different form. This patch is therefore
about applying a series of (mostly non-functional) refactorings to make
their diff as small as possible. This involves "stupid" changes such as
renaming variables, tweaking whitespace, reordering the declaration of
variables, etc. It's all to make the functions similar to the point where
I can diff them, get a small and meaningful result, see the remaining
_real_ differences, and in the end, _merge_ them (see patches 7-9).
If this whole exercise didn't end up with merging the two functions into
one, I would _totally_ agree with you that all this refactoring is more
harmful than beneficial.

> On Sat, 9 Jun 2007, Johan Herland wrote:
> >  	if (size < 64)
> >  		return error("wanna fool me ? you obviously got the size wrong !");
> >  
> > -	buffer[size] = 0;
> 
> Are you sure that your buffer is always NUL terminated?

First, (and you'll see this in the commit message) I'm _moving_ (not
removing) the NUL termination out of verify_tag() and into main() (which I
can be sure is the only caller of verify_tag(), since verify_tag is
declared static, and there is no other call in that file). Two reasons for
doing this:

1. Make verify_tag more similar to parse_tag_buffer() (because
parse_tag_buffer() does not NUL terminate)

2. Do the NUL termination as close to the code that actually populated the
buffer with data (the read_pipe() in main())


So now you can ask: Why doesn't parse_tag_buffer() NUL terminate its
input? It _that_ safe? And I ran around checking all the callers of
parse_tag_buffer, and found that all of them use data (most of which
originates from read_sha1_file()) that's already NUL terminated.

In the end, I also put in a comment on the resulting function
(parse_and_verify_tag_buffer()), explicitly saying the given data _must_
be NUL terminated.



Side note: At first I actually thought the manual NUL termination
could cause a buffer overflow (i.e. if the given size was the same as the
allocated size), so I actually have a version of all of this where I
_don't_ assume the buffer is NUL-terminated at all, and put in lots of
bounds checking, replace strchr() with memchr(), etc.

I then took a hard look at read_pipe(), and discovered that if you
use it to fill a 4096-byte buffer with 4096 bytes of data, it actually
_will_ reallocate to 8192 bytes and leave room for the NUL termination
(and much more) (I believe this should have been documented in read_pipe).
Thus the NUL termination was safe all along.


> > -	type_line = object + 48;
> > +	type_line = data + 48;
> 
> Quite a lot of changes seem to do this object->data. The patch would have 
> been much more compact if you just had renamed buffer to object instead of 
> data.

Yes, but it would have made the aforementioned diff to parse_tag_buffer()
larger.

> >  	/* TODO: check for committer info + blank line? */
> >  	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
> >  
> >  	/* The actual stuff afterwards we don't care about.. */
> >  	return 0;
> > -}
> >  
> >  #undef PD_FMT
> > +}
> 
> Any particular reason for this?

Well, PD_FMT is only used inside the function, so I found it easier to
move the #definition of PD_FMT inside the function to indicate the scope
(_perceived_ scope; I know it hasn't any effect on the compiled code).
But since the whole function is going away in a few patches anyway,
I should have probably left it out of this patch entirely.

> > @@ -124,6 +120,7 @@ int main(int argc, char **argv)
> >  		free(buffer);
> >  		die("could not read from stdin");
> >  	}
> > +	buffer[size] = 0;
> 
> Ah, so you terminate the buffer here. From the patch, it is relatively 
> hard to see if this line is always hit _before_ the function is called 
> that evidently relies on NUL termination. By moving it here, I think it is 
> much more likely to overlook the fact that the function, which made sure 
> that its assumption was met, needs this line. Whereas if you left it where 
> it was, the assumption would always be met.

But if I leave the NUL termination within the function I would have to
backtrack out of the function to all of its potential callers and check
whether it's safe to write to index size. Since the word "size" could
easily mean "allocated size" I would have the initial feeling that this
might be a buffer overflow, i.e. _not_ safe.

In the end, I think the best solution is to make sure NUL termination
happens before calling the function, and then documenting explicitly
that the function assumes NUL terminated input. Which is exactly what
I end up with at the end of the patch series.

> > -	sig_line++;
> > +	tagger_line++;
> 
> I am really reluctant with renamings like these. IMHO they don't buy you 
> much, except for possible confusion. It is evident that sig means the 
> signer (and it is obvious in the case of an unsigned tag, who is meant, 
> too).

Hmm. The "type" line is found in the variable type_line, the "tag" line is
found in the variable tag_line, and the "tagger" line is found in the
variable ... sig_line? Nope, I don't buy it.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 02/21] Return error messages when parsing fails.

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This patch brings the already similar tag.c:parse_tag_buffer() and
> mktag.c:verify_tag() a little bit closer to eachother.

While I would agree that it makes sense to have the same
definition of what is and is not a 100% well formatted tag
object for producer side and consumer side, I do not necessarily
think it is a good idea to make parse_tag_buffer() chattier on
errors.  mktag.c:verify_tag() can afford to be verbose in its
diagnosis, because it is used when the user is _creating_ the
tag, and it is generally a good idea to be strict when we
create.

On the other hand, parse_tag_buffer() is on the side of users
who use existing tag objects that were produced by somebody
else.  It is generally a good practice to be more lenient when
you are consuming.

Also, callers of parse_tag_buffer() know the function is silent
on errors (unless there is something seriously wrong with the
repository); they do their own error messages when they get an
error return.

Re: [PATCH 04/21] Refactor verification of "tagger" line to be more similar to verification of "type" and "tagger" lines

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Also update selftests to reflect that verification of "tagger" now
> happens _before_ verification of type name, object sha1 and tag name.
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  mktag.c          |   16 ++++++++--------
>  t/t3800-mktag.sh |    3 +++
>  tag.c            |    6 +++---
>  3 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/mktag.c b/mktag.c
> index 0bc20c8..4dbefab 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)
>  
>  	/* Verify tag-line */
>  	tag_line = strchr(type_line, '\n');
> -	if (!tag_line)
> +	if (!tag_line++)
>  		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
> -	tag_line++;

Code churn "while we are at it" makes reviewing the rest more
cumbersome.  A clean-up like this should be a separate patch.

> diff --git a/tag.c b/tag.c
> index 8d31603..19c66cd 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -73,10 +73,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
>  	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
>  		return error("char" PD_FMT ": no \"tag \" found", tag_line - data);
>  
> +	/* Verify the tagger line */
>  	tagger_line = strchr(tag_line, '\n');
> -	if (!tagger_line)
> -		return -1;
> -	tagger_line++;
> +	if (!tagger_line++)
> +		return error("char" PD_FMT ": could not find next \"\\n\"", tag_line - data);
>  
>  	/* Get the actual type */
>  	type_len = tag_line - type_line - strlen("type \n");

Same comments on extra verbosity, as for [2/21].

Re: [PATCH 05/21] Make parse_tag_buffer_internal() handle item == NULL

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This is in preparation for unifying verify_tag() and
> parse_tag_buffer_internal().
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  tag.c |   54 +++++++++++++++++++++++++++++-------------------------
>  1 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/tag.c b/tag.c
> index 19c66cd..b134967 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
>  	const char *type_line, *tag_line, *tagger_line;
>  	unsigned long type_len, tag_len;
>  
> -	if (item->object.parsed)
> -		return 0;
> -	item->object.parsed = 1;
> +	if (item) {
> +		if (item->object.parsed)
> +			return 0;
> +		item->object.parsed = 1;
> +	}
>  
>  	if (size < 64)
>  		return error("failed preliminary size check");

Passing both item and data does not feel right.  If you are
trying to make the factored out function to do the verification
of data, then perhaps the caller should do the "don't handle the
same data twice" optimization using item?

Re: [PATCH 02/21] Return error messages when parsing fails.

[Johan Herland]

On Saturday 09 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> 
> > This patch brings the already similar tag.c:parse_tag_buffer() and
> > mktag.c:verify_tag() a little bit closer to eachother.
> 
> While I would agree that it makes sense to have the same
> definition of what is and is not a 100% well formatted tag
> object for producer side and consumer side, I do not necessarily
> think it is a good idea to make parse_tag_buffer() chattier on
> errors.  mktag.c:verify_tag() can afford to be verbose in its
> diagnosis, because it is used when the user is _creating_ the
> tag, and it is generally a good idea to be strict when we
> create.
> 
> On the other hand, parse_tag_buffer() is on the side of users
> who use existing tag objects that were produced by somebody
> else.  It is generally a good practice to be more lenient when
> you are consuming.
> 
> Also, callers of parse_tag_buffer() know the function is silent
> on errors (unless there is something seriously wrong with the
> repository); they do their own error messages when they get an
> error return.
> 
> 

Ok. I can make the error messages conditional on 'thorough_verify'.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH] Silence error messages unless 'thorough_verify' is set

[Johan Herland]

We don't want to print error message on regular parsing of tag objects.

With this patch error messages are only printed when 'thorough_verify'
is set, i.e. when creating new or fscking tag objects.

Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---

On Saturday 09 June 2007, Johan Herland wrote:
> On Saturday 09 June 2007, Junio C Hamano wrote:
> > While I would agree that it makes sense to have the same
> > definition of what is and is not a 100% well formatted tag
> > object for producer side and consumer side, I do not necessarily
> > think it is a good idea to make parse_tag_buffer() chattier on
> > errors.  mktag.c:verify_tag() can afford to be verbose in its
> > diagnosis, because it is used when the user is _creating_ the
> > tag, and it is generally a good idea to be strict when we
> > create.
> > 
> > On the other hand, parse_tag_buffer() is on the side of users
> > who use existing tag objects that were produced by somebody
> > else.  It is generally a good practice to be more lenient when
> > you are consuming.
> > 
> > Also, callers of parse_tag_buffer() know the function is silent
> > on errors (unless there is something seriously wrong with the
> > repository); they do their own error messages when they get an
> > error return.
> 
> Ok. I can make the error messages conditional on 'thorough_verify'.

Is this one ok?

(It goes on top of the patch series as a whole)


...Johan

 tag.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/tag.c b/tag.c
index af4356e..c3a2855 100644
--- a/tag.c
+++ b/tag.c
@@ -67,6 +67,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 #define PD_FMT "%td"
 #endif
 
+#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )
+
 	unsigned char sha1[20];
 	char type[20];
 	const char   *type_line, *tag_line, *keywords_line, *tagger_line;
@@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	}
 
 	if (size < 65)
-		return error("Tag object failed preliminary size check");
+		return FAIL("Tag object failed preliminary size check");
 
 	/* Verify mandatory object line */
 	if (prefixcmp(data, "object "))
-		return error("Tag object (@ char 0): "
+		return FAIL("Tag object (@ char 0): "
 			"Does not start with \"object \"");
 
 	if (get_sha1_hex(data + 7, sha1))
-		return error("Tag object (@ char 7): Could not get SHA1 hash");
+		return FAIL("Tag object (@ char 7): Could not get SHA1 hash");
 
 	/* Verify mandatory type line */
 	type_line = data + 48;
 	if (prefixcmp(type_line - 1, "\ntype "))
-		return error("Tag object (@ char 47): "
+		return FAIL("Tag object (@ char 47): "
 			"Could not find \"\\ntype \"");
 
 	/* Verify optional tag line */
 	tag_line = strchr(type_line, '\n');
 	if (!tag_line++)
-		return error("Tag object (@ char " PD_FMT "): "
+		return FAIL("Tag object (@ char " PD_FMT "): "
 			"Could not find \"\\n\" after \"type\"",
 			type_line - data);
 	if (prefixcmp(tag_line, "tag ")) /* no tag name given */
@@ -107,7 +109,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	else {                           /* tag name given */
 		keywords_line = strchr(tag_line, '\n');
 		if (!keywords_line++)
-			return error("Tag object (@ char " PD_FMT "): "
+			return FAIL("Tag object (@ char " PD_FMT "): "
 				"Could not find \"\\n\" after \"tag\"",
 				tag_line - data);
 	}
@@ -118,7 +120,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	else {                                     /* keywords given */
 		tagger_line = strchr(keywords_line, '\n');
 		if (!tagger_line++)
-			return error("Tag object (@ char " PD_FMT "): "
+			return FAIL("Tag object (@ char " PD_FMT "): "
 				"Could not find \"\\n\" after \"keywords\"",
 				keywords_line - data);
 	}
@@ -132,7 +134,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	 */
 	if (prefixcmp(tagger_line, "tagger ")) { /* no tagger given */
 		if (thorough_verify)
-			return error("Tag object (@ char " PD_FMT "): "
+			return FAIL("Tag object (@ char " PD_FMT "): "
 				"Could not find \"tagger \"",
 				tagger_line - data);
 		header_end = tagger_line;
@@ -140,7 +142,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	else {                                   /* tagger given */
 		header_end = strchr(tagger_line, '\n');
 		if (!header_end++)
-			return error("Tag object (@ char " PD_FMT "): "
+			return FAIL("Tag object (@ char " PD_FMT "): "
 				"Could not find \"\\n\" after \"tagger\"",
 				tagger_line - data);
 	}
@@ -164,7 +166,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 	/* Get the actual type */
 	if (type_len >= sizeof(type))
-		return error("Tag object (@ char " PD_FMT "): "
+		return FAIL("Tag object (@ char " PD_FMT "): "
 			"Type too long", type_line - data);
 	memcpy(type, type_line, type_len);
 	type[type_len] = '\0';
@@ -174,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 		/* Verify that the object matches */
 		if (verify_object(sha1, type))
-			return error("Tag object (@ char 7): "
+			return FAIL("Tag object (@ char 7): "
 				"Could not verify tagged object %s",
 				sha1_to_hex(sha1));
 
@@ -184,7 +186,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				unsigned char c = tag_line[i];
 				if (c > ' ' && c != 0x7f)
 					continue;
-				return error("Tag object (@ char " PD_FMT "): "
+				return FAIL("Tag object (@ char " PD_FMT "): "
 					"Could not verify tag name",
 					tag_line + i - data);
 			}
@@ -199,12 +201,12 @@ int parse_and_verify_tag_buffer(struct tag *item,
 				unsigned char c = keywords_line[i];
 				if (c == ',' && keywords_line[i + 1] == ',')
 					/* consecutive commas */
-					return error("Tag object (@ char "
+					return FAIL("Tag object (@ char "
 						PD_FMT "): Found empty keyword",
 						keywords_line + i - data);
 				if (c > ' ' && c != 0x7f)
 					continue;
-				return error("Tag object (@ char " PD_FMT "): "
+				return FAIL("Tag object (@ char " PD_FMT "): "
 					"Could not verify keywords",
 					keywords_line + i - data);
 			}
@@ -248,7 +250,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		} else if (!strcmp(type, tag_type)) {
 			item->tagged = &lookup_tag(sha1)->object;
 		} else {
-			error("Tag object (@ char " PD_FMT "): "
+			FAIL("Tag object (@ char " PD_FMT "): "
 				"Unknown type '%s'", type_line - data, type);
 			item->tagged = NULL;
 		}
@@ -262,6 +264,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 	return 0;
 
+#undef FAIL
+
 #undef PD_FMT
 }
 
-- 
1.5.2.1.144.gabc40

Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  mktag.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)

What is this change good for?
How did you justify the type selection for your
loop index variable?

IOW,  the patch looks very useless.

Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> +               /* Verify the tag-name: we don't allow control characters or spaces in it */
> +               for (i = 4;;) {
> +                       unsigned char c = tag_line[i++];
> +                       if (c == '\n')
> +                               break;
> +                       if (c > ' ')
> +                               continue;
> +                       return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
> +               }

This looks very familiar. Haven't you just made a very useless patch
which had this very same code? How about putting it in its own
function and just call it from these two places? And what problem
do you have with pointers?!

Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > Signed-off-by: Johan Herland <johan@herland.net>
> > ---
> >  mktag.c |   29 ++++++++++++++++-------------
> >  1 files changed, 16 insertions(+), 13 deletions(-)
> 
> What is this change good for?
> How did you justify the type selection for your
> loop index variable?
> 
> IOW,  the patch looks very useless.

I agree. By itself, the patch is useless.

However, if you look at the next patch, you'll see that this exact piece of 
code is moved from verify_tag() to parse_and_verify_tag_buffer(), and in 
the new context, we can't increment tag_line, since the code that follows 
depends on tag_line not being moved.

In other words this patch is here so that the next patch will be easier to 
follow. because it's _literally_ moving copying code from verify_tag() and 
pasting it in parse_and_verify_tag_buffer().

I'm sorry if this is not clear from the patches.


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 10/21] Free mktag's buffer before dying

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> +       if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> +               free(buffer);
> +               die("invalid tag data file");

This, and the similar one below are useless. You're destroying the
process, what do you free that buffer for? Either handle the error
case or do not needlessly complicate your change, which really
also absolutely unneeded.

Re: [PATCH 09/21] Remove unneeded code from mktag.c

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  mktag.c |   94 ---------------------------------------------------------------
>  1 files changed, 0 insertions(+), 94 deletions(-)

So, you do some useless changes just to remove the
function completely afterwards?

Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > +               /* Verify the tag-name: we don't allow control characters or spaces in it */
> > +               for (i = 4;;) {
> > +                       unsigned char c = tag_line[i++];
> > +                       if (c == '\n')
> > +                               break;
> > +                       if (c > ' ')
> > +                               continue;
> > +                       return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
> > +               }
> 
> This looks very familiar. Haven't you just made a very useless patch
> which had this very same code? How about putting it in its own
> function and just call it from these two places? And what problem
> do you have with pointers?!

I just answered your comment on the previous patch, and that answer should
apply here as well.

I'm probably splitting this up into too small pieces, since I keep getting
comments that fail to see the overall picture of what I'm trying to do,
namely taking two similar pieces of code and slowly unifying them to the
point where I can replace one of them by a call to the other (see the two
next patches).

Hope this helps,


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> +/*
> + * Perform parsing and verification of tag object data.
> + *
> + * The 'item' parameter may be set to NULL if only verification is desired.
> + *
> + * The given data _must_ be null-terminated.
> + */
>  int parse_and_verify_tag_buffer(struct tag *item,
>                 const char *data, const unsigned long size, int thorough_verify)

This hunk really belongs into commit which introduced the function
parse_and_verify_tag_buffer.

Re: [PATCH 09/21] Remove unneeded code from mktag.c

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > Signed-off-by: Johan Herland <johan@herland.net>
> > ---
> >  mktag.c |   94 ---------------------------------------------------------------
> >  1 files changed, 0 insertions(+), 94 deletions(-)
> 
> So, you do some useless changes just to remove the
> function completely afterwards?

Yes. Basically so that people can follow my process. If you don't want the
intermediary/useless states, just look at my first patch series that was
replaced by this series because it was too bulky and disruptive to follow.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 10/21] Free mktag's buffer before dying

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > +       if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > +               free(buffer);
> > +               die("invalid tag data file");
> 
> This, and the similar one below are useless. You're destroying the
> process, what do you free that buffer for? Either handle the error
> case or do not needlessly complicate your change, which really
> also absolutely unneeded.

Well, I was taught to treat my memory with care.

Right now it doesn't make any difference in practice (except that
Valgrind might be a bit happier with it), but in the future -- with
the libifaction effort and whatnot -- you never know what might happen
to this piece of code, and I'd like to stay on the safe side.

Feel free to drop this patch from the series if I'm the only one thinking
like this.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > +/*
> > + * Perform parsing and verification of tag object data.
> > + *
> > + * The 'item' parameter may be set to NULL if only verification is desired.
> > + *
> > + * The given data _must_ be null-terminated.
> > + */
> >  int parse_and_verify_tag_buffer(struct tag *item,
> >                 const char *data, const unsigned long size, int thorough_verify)
> 
> This hunk really belongs into commit which introduced the function
> parse_and_verify_tag_buffer.

Yes. I'm sorry it slipped out of that patch and into this one.


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 16/21] Introduce optional "keywords" on tag objects

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> This patch introduces a new optional header line to the tag object, called
> "keywords". The "keywords" line may contain a comma-separated list of
> custom keywords associated with the tag object.

What is the character set for the keywords?

> +                       for (i = 0; i < keywords_len; ++i) {
> +                               unsigned char c = keywords_line[i];
> +                               if (c == ',' && keywords_line[i + 1] == ',')
> +                                       /* consecutive commas */
> +                                       return error("Tag object (@ char "
> +                                               PD_FMT "): Found empty keyword",
> +                                               keywords_line + i - data);
> +                               if (c > ' ' && c != 0x7f)
> +                                       continue;

And what is so special about 0x7f?


> +               if (keywords_len) { /* optional keywords string was given */
> +                       item->keywords = xmalloc(keywords_len + 1);

Who frees the keywords and what's wrong with strndup?

Re: [PATCH 10/21] Free mktag's buffer before dying

[Alex Riesen]

On 6/9/07, Johan Herland <johan@herland.net> wrote:
> On Saturday 09 June 2007, Alex Riesen wrote:
> > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > +       if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > > +               free(buffer);
> > > +               die("invalid tag data file");
> >
> > This, and the similar one below are useless. You're destroying the
> > process, what do you free that buffer for? Either handle the error
> > case or do not needlessly complicate your change, which really
> > also absolutely unneeded.
>
> Well, I was taught to treat my memory with care.
>

How do you treat your performance?
Besides, was that systems with common address space
where you were taught? Like DOS or MacOS, perhaps?

> Right now it doesn't make any difference in practice (except that
> Valgrind might be a bit happier with it), but in the future -- with
> the libifaction effort and whatnot -- you never know what might happen
> to this piece of code, and I'd like to stay on the safe side.

So that people have to check your free as well (they will have to,
they come looking for die-calls). You just made more work for them.

Re: [PATCH 16/21] Introduce optional "keywords" on tag objects

[Johan Herland]

On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > This patch introduces a new optional header line to the tag object, called
> > "keywords". The "keywords" line may contain a comma-separated list of
> > custom keywords associated with the tag object.
> 
> What is the character set for the keywords?

Hmm. Same as for the "tag" line, and the rest of the tag object, I guess.
I agree this should probably be specified. What are the rules for other
git objects?

> > +                       for (i = 0; i < keywords_len; ++i) {
> > +                               unsigned char c = keywords_line[i];
> > +                               if (c == ',' && keywords_line[i + 1] == ',')
> > +                                       /* consecutive commas */
> > +                                       return error("Tag object (@ char "
> > +                                               PD_FMT "): Found empty keyword",
> > +                                               keywords_line + i - data);
> > +                               if (c > ' ' && c != 0x7f)
> > +                                       continue;
> 
> And what is so special about 0x7f?

Isn't DEL a control char?

> > +               if (keywords_len) { /* optional keywords string was given */
> > +                       item->keywords = xmalloc(keywords_len + 1);
> 
> Who frees the keywords and what's wrong with strndup?

The code that should free the tag name should also free the keywords.
No code appear to do this at the moment (regardless of whether you use my patch
series or not), which is a shame. We should make sure all objects are properly
deallocated. This is currently a general problem in git, isn't it?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 10/21] Free mktag's buffer before dying

[Johan Herland]

On Sunday 10 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > On Saturday 09 June 2007, Alex Riesen wrote:
> > > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > > +       if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > > > +               free(buffer);
> > > > +               die("invalid tag data file");
> > >
> > > This, and the similar one below are useless. You're destroying the
> > > process, what do you free that buffer for? Either handle the error
> > > case or do not needlessly complicate your change, which really
> > > also absolutely unneeded.
> >
> > Well, I was taught to treat my memory with care.
> 
> How do you treat your performance?

Hopefully with care, as well. However, I tend to look at performance _after_
correctness.

> Besides, was that systems with common address space
> where you were taught? Like DOS or MacOS, perhaps?

Nope. Never programmed on either. I thought care with memory was generally
considered a good principle. If I'm wrong, please point me at the relevant
documentation.

> > Right now it doesn't make any difference in practice (except that
> > Valgrind might be a bit happier with it), but in the future -- with
> > the libifaction effort and whatnot -- you never know what might happen
> > to this piece of code, and I'd like to stay on the safe side.
> 
> So that people have to check your free as well (they will have to,
> they come looking for die-calls). You just made more work for them.

Ok. Drop it. This isn't particularily important to me. I just try to
follow good principles when I can.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH] Use xstrndup() instead of xmalloc() and memcpy(); fix buglet with generating default item->keywords.

[Johan Herland]

Using xstrndup() yields more compact and readable code than using
xmalloc(), memcpy() and manual NUL termination.
Thanks to Alex Riesen <raa.lkml@gmail.com> for suggesting this.

Also fixes a buglet where item->keywords would always be set to "tag",
even if item->tag was empty.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Saturday 09 June 2007, Alex Riesen wrote:
> ... and what's wrong with strndup?

Nothing. 


...Johan

 tag.c |   27 ++++++++++-----------------
 1 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/tag.c b/tag.c
index c3a2855..2307ec9 100644
--- a/tag.c
+++ b/tag.c
@@ -219,26 +219,19 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	}
 
 	if (item) { /* Store parsed information into item */
-		if (tag_len) { /* optional tag name was given */
-			item->tag = xmalloc(tag_len + 1);
-			memcpy(item->tag, tag_line, tag_len);
-			item->tag[tag_len] = '\0';
-		}
-		else { /* optional tag name not given */
-			item->tag = xmalloc(1);
-			item->tag[0] = '\0';
-		}
+		if (tag_len) /* optional tag name was given */
+			item->tag = xstrndup(tag_line, tag_len);
+		else /* optional tag name not given */
+			item->tag = xstrndup("", 0);
 
-		if (keywords_len) { /* optional keywords string was given */
-			item->keywords = xmalloc(keywords_len + 1);
-			memcpy(item->keywords, keywords_line, keywords_len);
-			item->keywords[keywords_len] = '\0';
-		}
+		if (keywords_len) /* optional keywords string was given */
+			item->keywords = xstrndup(keywords_line, keywords_len);
 		else { /* optional keywords string not given. Set default */
 			/* if tag name is set, use "tag"; else use "note" */
-			const char *default_kw = item->tag ? "tag" : "note";
-			item->keywords = xmalloc(strlen(default_kw) + 1);
-			memcpy(item->keywords, default_kw, strlen(default_kw) + 1);
+			if (*(item->tag))
+				item->keywords = xstrndup("tag", 3);
+			else
+				item->keywords = xstrndup("note", 4);
 		}
 
 		if (!strcmp(type, blob_type)) {
-- 
1.5.2.1.144.gabc40

Re: [PATCH 16/21] Introduce optional "keywords" on tag objects

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 6/9/07, Johan Herland <johan@herland.net> wrote:
>> This patch introduces a new optional header line to the tag object, called
>> "keywords". The "keywords" line may contain a comma-separated list of
>> custom keywords associated with the tag object.
>
> What is the character set for the keywords?
>
>> +                       for (i = 0; i < keywords_len; ++i) {
>> +                               unsigned char c = keywords_line[i];
>> +                               if (c == ',' && keywords_line[i + 1] == ',')
>> +                                       /* consecutive commas */
>> +                                       return error("Tag object (@ char "
>> +                                               PD_FMT "): Found empty keyword",
>> +                                               keywords_line + i - data);
>> +                               if (c > ' ' && c != 0x7f)
>> +                                       continue;
>
> And what is so special about 0x7f?

It is DEL, but as the code uses uchar, it probably also error on
0x80 or higher, if the intent is "printable ASCII".

[PATCH] Fail if tag name and keywords is not within "printable ASCII"

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---

On Sunday 10 June 2007, Junio C Hamano wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
> > And what is so special about 0x7f?
> 
> It is DEL, but as the code uses uchar, it probably also error on
> 0x80 or higher, if the intent is "printable ASCII".

Is this better?


...Johan

 tag.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tag.c b/tag.c
index 2307ec9..2b92465 100644
--- a/tag.c
+++ b/tag.c
@@ -183,8 +183,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		/* Verify tag name: disallow control characters or spaces */
 		if (tag_len) { /* tag name was given */
 			for (i = 0; i < tag_len; ++i) {
-				unsigned char c = tag_line[i];
-				if (c > ' ' && c != 0x7f)
+				char c = tag_line[i];
+				if (c > ' ' && c < 0x7f)
 					continue;
 				return FAIL("Tag object (@ char " PD_FMT "): "
 					"Could not verify tag name",
@@ -198,13 +198,13 @@ int parse_and_verify_tag_buffer(struct tag *item,
 		 */
 		if (keywords_len) { /* keywords line was given */
 			for (i = 0; i < keywords_len; ++i) {
-				unsigned char c = keywords_line[i];
+				char c = keywords_line[i];
 				if (c == ',' && keywords_line[i + 1] == ',')
 					/* consecutive commas */
 					return FAIL("Tag object (@ char "
 						PD_FMT "): Found empty keyword",
 						keywords_line + i - data);
-				if (c > ' ' && c != 0x7f)
+				if (c > ' ' && c < 0x7f)
 					continue;
 				return FAIL("Tag object (@ char " PD_FMT "): "
 					"Could not verify keywords",
-- 
1.5.2.1.144.gabc40

	

[PATCH] Move check for already parsed tag object to parse_tag_buffer() wrapper function

[Johan Herland]

This effectively disables the check when creating and fscking tag objects,
which is desirable because we don't want the fact that the object is
already parsed to stop us from verifying thoroughly.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Saturday 09 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > diff --git a/tag.c b/tag.c
> > index 19c66cd..b134967 100644
> > --- a/tag.c
> > +++ b/tag.c
> > @@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
> >  	const char *type_line, *tag_line, *tagger_line;
> >  	unsigned long type_len, tag_len;
> >  
> > -	if (item->object.parsed)
> > -		return 0;
> > -	item->object.parsed = 1;
> > +	if (item) {
> > +		if (item->object.parsed)
> > +			return 0;
> > +		item->object.parsed = 1;
> > +	}
>
> Passing both item and data does not feel right.  If you are
> trying to make the factored out function to do the verification
> of data, then perhaps the caller should do the "don't handle the
> same data twice" optimization using item?

You mean, like this?


...Johan

 tag.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tag.c b/tag.c
index 2b92465..4e79943 100644
--- a/tag.c
+++ b/tag.c
@@ -75,12 +75,6 @@ int parse_and_verify_tag_buffer(struct tag *item,
 	unsigned long type_len,   tag_len,   keywords_len,   tagger_len;
 	const char *header_end;
 
-	if (item) {
-		if (item->object.parsed)
-			return 0;
-		item->object.parsed = 1;
-	}
-
 	if (size < 65)
 		return FAIL("Tag object failed preliminary size check");
 
@@ -264,6 +258,10 @@ int parse_and_verify_tag_buffer(struct tag *item,
 
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
+	if (item->object.parsed)
+		return 0;
+	item->object.parsed = 1;
+
 	return parse_and_verify_tag_buffer(item, (const char *) data, size, 0);
 }
 
-- 
1.5.2.1.144.gabc40

Re: [PATCH] Fail if tag name and keywords is not within "printable ASCII"

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>
> On Sunday 10 June 2007, Junio C Hamano wrote:
>> "Alex Riesen" <raa.lkml@gmail.com> writes:
>> > And what is so special about 0x7f?
>> 
>> It is DEL, but as the code uses uchar, it probably also error on
>> 0x80 or higher, if the intent is "printable ASCII".
>
> Is this better?

I said "*if* the intent is to limit to printable ASCII".

It is still unclear what use cases the "keywords" need to
support (e.g. do we want to have "pick tags that have keyword
that matches this pattern"?  if so, what kind of pattern
language do we want to use?  Glob?  Regexp?  Would it be more
convenient if the keywords are treated case insensitively?  Is
there a useful use case if you are allowed to use people's names
as keywords?  Is it reasonable to assume that the keywords can
be split with a comma?  Do we want to allow a comma as part of
keywords?  If so, how would we quote a comma that is inside a
keyword?  etc.).  It depends on the answers to these questions
if "printable ASCII" is a good set.

As a general rule, if you make the initially allowed set of
values too wide, it gets _extremely_ hard to tighten it later.
So if we are to stick to printable ASCII (iow, "no people's
names or names of location as keywords"), I would even suggest
to limit the valid set further, say "^[-A-Za-z0-9_+.]+$", at
least initially.

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> +#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )
> +
>  	unsigned char sha1[20];
>  	char type[20];
>  	const char   *type_line, *tag_line, *keywords_line, *tagger_line;
> @@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
>  	}
>  
>  	if (size < 65)
> -		return error("Tag object failed preliminary size check");
> +		return FAIL("Tag object failed preliminary size check");

This is ugly.

If you _have_ to output the error message in one case, and not in the 
other, I'd rather do

	enum tag_error { TAG_SIZE_CHECK, TAG_BLA_BLUB, ... };
	const char **tag_error_strings = { "tag: size error", ... };

Of course, you'd lose the ability to output some numbers. But those 
numbers that you output are even uglier than the code. Guess how surprised 
_I_ was, when I hit the error message which made me go mad.

Having said that, I still do not agree in this unifying.

Your rationale seems to be: use the same checking for the tag creation as 
for the tag validation.

But this is _wrong_. We _do_ have tags that do not conform to the strict 
standards of git-tag, and even if we did _not_, it would _still_ be wrong 
to be that strict when _reading_ tags.

To drive that point home: strict checking when creating tags is good. 
Strict checking when reading tags is bad.

I strongly encourage keeping both validations separate.

You'd also avoid having that many lines which are well over the encouraged 
80 character limit.

Ciao,
Dscho

Re: [PATCH 04/21] Refactor verification of "tagger" line to be more similar to verification of "type" and "tagger" lines

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Junio C Hamano wrote:

> Johan Herland <johan@herland.net> writes:
> 
> > +	if (!tag_line++)
> >  		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
> > -	tag_line++;

BTW if you _are_ verbosing the output, you might just as well make it 
useful.

The common format is "filename:line[:column]:message", not "char[n]", 
which is a misnomer to begin with, since you are talking about an offset, 
not a char (remember, characters are those things that are displayed in 
place for a given number, so I fully expected char32 to be a space).

Ciao,
Dscho

Re: [PATCH 05/21] Make parse_tag_buffer_internal() handle item == NULL

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> -	tag_len = tagger_line - tag_line - strlen("tag \n");
> -	item->tag = xmalloc(tag_len + 1);
> -	memcpy(item->tag, tag_line + 4, tag_len);
> -	item->tag[tag_len] = '\0';
> -
> -	if (!strcmp(type, blob_type)) {
> -		item->tagged = &lookup_blob(sha1)->object;
> -	} else if (!strcmp(type, tree_type)) {
> -		item->tagged = &lookup_tree(sha1)->object;
> -	} else if (!strcmp(type, commit_type)) {
> -		item->tagged = &lookup_commit(sha1)->object;
> -	} else if (!strcmp(type, tag_type)) {
> -		item->tagged = &lookup_tag(sha1)->object;
> -	} else {
> -		error("Unknown type %s", type);
> -		item->tagged = NULL;
> -	}
> -
> -	if (item->tagged && track_object_refs) {
> -		struct object_refs *refs = alloc_object_refs(1);
> -		refs->ref[0] = item->tagged;
> -		set_object_refs(&item->object, refs);
> +	if (item) {
> +		tag_len = tagger_line - tag_line - strlen("tag \n");
> +		item->tag = xmalloc(tag_len + 1);
> +		memcpy(item->tag, tag_line + 4, tag_len);
> +		item->tag[tag_len] = '\0';
> +
> +		if (!strcmp(type, blob_type)) {
> +			item->tagged = &lookup_blob(sha1)->object;
> +		} else if (!strcmp(type, tree_type)) {
> +			item->tagged = &lookup_tree(sha1)->object;
> +		} else if (!strcmp(type, commit_type)) {
> +			item->tagged = &lookup_commit(sha1)->object;
> +		} else if (!strcmp(type, tag_type)) {
> +			item->tagged = &lookup_tag(sha1)->object;
> +		} else {
> +			error("Unknown type %s", type);
> +			item->tagged = NULL;
> +		}
> +
> +		if (item->tagged && track_object_refs) {
> +			struct object_refs *refs = alloc_object_refs(1);
> +			refs->ref[0] = item->tagged;
> +			set_object_refs(&item->object, refs);
> +		}
>  	}
>  
>  	return 0;

The patch would have been so much simpler, and so much more descriptive of 
what you're doing, had you just said

	if (!item)
		return 0;

Ciao,
Dscho

Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> On Saturday 09 June 2007, Alex Riesen wrote:
> > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > Signed-off-by: Johan Herland <johan@herland.net>
> > > ---
> > >  mktag.c |   29 ++++++++++++++++-------------
> > >  1 files changed, 16 insertions(+), 13 deletions(-)
> > 
> > What is this change good for?
> > How did you justify the type selection for your
> > loop index variable?
> > 
> > IOW,  the patch looks very useless.
> 
> I agree. By itself, the patch is useless.

Then it shouldn't be there.

It seems that you do not place the cuts between patches at the 
_conceptual_ layer. Therefore, they seem intrusive and often the meaning 
evades me.

So, if I understood the purpose of this patch series correctly, namely to 
use the same verification routines both for creation as for validation of 
tags, you could have

	- moved one function into the library (the stricter one), saying 
	  "move this_function() into libgit.a to make it usable from 
	   git-bla" in the commit body,

	- used that from the other program, removing the now-unused 
	  function,

	- and then changed the behaviour to be more chatty or some such.

As it is, you have a mix of conceptually different changes in almost every 
patch, and some changes that conceptually belong into the same patch, are 
not.

Be that as may, I think it is not a good change to reuse the same function 
like you did, exactly because one version _should_ be more forgiving than 
the other.

Ciao,
Dscho

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sat, 9 Jun 2007, Johan Herland wrote:
> ...
>> @@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
>>  	}
>>  
>>  	if (size < 65)
>> -		return error("Tag object failed preliminary size check");
>> +		return FAIL("Tag object failed preliminary size check");
>
> This is ugly.

... quite a bit.  A less uglier alternative we seem to use in
other places is not much better (return NULL on failure or an
error message string on error).

> ...  Guess how surprised 
> _I_ was, when I hit the error message which made me go mad.

To be fair, that ugly "char%d" was taken from mktag and not
Johan's invention.

> To drive that point home: strict checking when creating tags is good. 
> Strict checking when reading tags is bad.
>
> I strongly encourage keeping both validations separate.

While I tend ot think that keeping two separate versions is
probably better for this particular case, the above statement
has a leap in its logic.  With your "error code" scheme, you
could implement a single, verifier/parser that defines the
concrete and complete rule of how the data should look like.
That unified verifier/parser itself should be silent.  Then, you
can have each of the callers decide how lenient it wants to be,
depending on the seriousness of the error.  You can make
producer very strict and chatty while leaving consumer liberal
and more silent.

There are pros-and-cons, however.

 - Such a scheme to return error codes and have two callers that
   have different behaviours is cumbersome to set up and use.

   A good example of this is the switch/case mess in each of the
   callers of run_command_v_opt() in builtin-push.c,
   builtin-revert.c, receive-pack.c etc.  For run_command, the
   mess is justifiable because the function has enough number of
   different callers, but in the current thread, we are only
   talking about two callers (parsing vs verifying of tag
   objects).

 - It has a risk to introduce inconsitent definition of the data
   format to have completely separate producer and consumer
   implementations; this is especially true when the data in
   question is complex.

   However, a tag is sufficiently simple that my personal
   feeling is that, combined with the cumbersomeness argument
   against the unified verifier, separate producer and consumer
   implementations would be easier to manage for this particular
   case.

Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> I'm probably splitting this up into too small pieces, since I keep 
> getting comments that fail to see the overall picture of what I'm trying 
> to do, [...]

Maybe I said that your patch was too large. But then, I said something 
much more important: hard-to-review.

These small patches, split in a manner making it even more difficult to 
understand what you want to accomplish, do not help.

Yes, you should make small patches. Even small patch series. But in such a 
fashion that a reviewer can see that it is a good patch[*1*]. Just lean 
back, look at your patches, and ask yourself how you would have reacted if 
you had reviewed them.

Ciao,
Dscho

*1* A good patch follows the immortal words of Saint Exupery: A designer 
knows he has achieved perfection not when there is nothing left to add, 
but when there is nothing left to take away.

Re: [PATCH 10/21] Free mktag's buffer before dying

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

How does this:

> -		usage("git-mktag < signaturefile");
> +		usage("git-mktag < tag_data_file");

and this:

> -

relate with your commit message?

You might understand that people _could_ get the impression that the 
patches were not very carefully crafted. Or even worse, the impression, 
that it was tried to slip some changes by, with a totally unrelated 
"official" purpose. (Again, reminds me of politics: it's like 
slipping an intrusive privacy law into an agriculture related law at the 
11th hour, just before Christmas, when people do not really pay 
attention.)

Ciao,
Dscho

Re: [PATCH 11/21] Rewrite error messages; fix up line lengths

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> Also update selftests to reflect new error messages.

Had you split the patch conceptually, the changes to the tests would have 
been included where appropriate.

I consider a patch _breaking_ a test case, with a follow up patch to the 
test case, a _bug_.

And I consider a change in error messages not good, _unless_ the changes 
have a real value in practice. Which I maintain they do not, in this 
patch-series' case.

Ciao,
Dscho

Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:
>
> -	if (memcmp(data, "object ", 7))
> +	if (prefixcmp(data, "object "))

FWIW I think that _these_ changes are actually somewhat worth it. And they 
should have come _instead_ of moving the order of the memcmp() around 
(you know which patch that was).

Ciao,
Dscho

Re: [PATCH 13/21] Collect skipping of header field names and calculation of line lengths in one place

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> +	/*
> +	 * Advance header field pointers past their initial identifier.
> +	 * Calculate lengths of header fields.
> +	 */
> +	type_line += strlen("type ");
> +	type_len   = tag_line - type_line - 1;
> +	tag_line  += strlen("tag ");
> +	tag_len    = tagger_line - tag_line - 1;
> +
>  	/* Get the actual type */
> -	type_len = tag_line - type_line - strlen("type \n");
>  	if (type_len >= sizeof(type))
>  		return error("Tag object (@ char " PD_FMT "): "
> -			"Type too long", type_line + 5 - data);
> -	memcpy(type, type_line + 5, type_len);
> +			"Type too long", type_line - data);
> +	memcpy(type, type_line, type_len);

This change does not clarify anything. It is exactly as confusing as 
before.

> -		for (i = 4;;) {
> +		for (i = 0;;) {

I know you introduced this in another patch. This is an ugly construct. If 
you want people to review your patches, you might as well put in the 
effort to make the reviewing more pleasant.

Ciao,
Dscho

Re: [PATCH 14/21] Add proper parsing of "tagger" line, but only when thorough_verify is set

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> Be explicit about the fact that the "tagger" line is now considered a 
> mandatory part of the tag object. There are however old tags (from 
> before July 2005) that don't have a "tagger" line. We therefore consider 
> the "tagger" line _optional_ when parsing tags without thorough_verify 
> set.

No. The "before July 2005" part is _not_ the reason that we consider this 
line optional.

The fact that it is bad to fail on a fetch, just because you happen to 
have an invalid tag in your repository, is a good reason not to.

The fact that it is bad to fail on a git branch, just because you happen 
to have an invalid tag in your repository, is a good reason not to.

The fact that it is bad to fail on an fsck, just because you happen to 
have an invalid tag in your repository, is a good reason not to.

And yes, if I remember correctly, your original patch did exactly that.

The paradigm to follow is: fail gracefully. I could have an invalid 
_commit_ in my repository, and would still want _every_ Git operation to 
succeed, _as long_ as it does not touch that bad object.

And I damned well want git-fsck to not crash, just because some 
assumptions are made.

Since this is a fundamental critique on your patch series, I will do the 
detailed review on _this_ patch in another mail.

Ciao,
Dscho

Re: [PATCH 14/21] Add proper parsing of "tagger" line, but only when thorough_verify is set

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> -	const char *type_line, *tag_line, *tagger_line;
> -	unsigned long type_len, tag_len;
> +	const char   *type_line, *tag_line, *tagger_line;
> +	unsigned long type_len,   tag_len,   tagger_len;
> +	const char *header_end;

This is ugly. Really ugly. Besides, it breaks the minimal patch paradigm.

> @@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
>  	if (size < 64)
>  		return error("Tag object failed preliminary size check");
>  
> -	/* Verify object line */
> +	/* Verify mandatory object line */
>  	if (prefixcmp(data, "object "))
>  		return error("Tag object (@ char 0): "
>  			"Does not start with \"object \"");

Hmph. I think everybody reading C code understands that this is mandatory. 
I even think that the comment is useless. It is sort of a 
code-in-human-language duplicated code.

> -	/* Verify the tagger line */
> +	/*
> +	 * Verify mandatory tagger line, but only when we're checking
> +	 * thoroughly, i.e. on inserting a new tag, and on fsck.
> +	 * There are existing tag objects without a tagger line (most
> +	 * notably the "v0.99" tag in the main git repo), and we don't
> +	 * want to fail parsing on these.
> +	 */

I maintain that even with thorough checking, it is _wrong_ to error on a 
missing tagger. Since we have to deal with tagger-less tags _anyway_, and 
since it is not like you could really do something about it (the tag is 
immutable), you should go with a warning.

> -	 * Calculate lengths of header fields.
> +	 * Calculate lengths of header fields (0 for fields that are not given).

Does that really make sense? You effectively treat a missing field as if 
it were empty. IMHO that is wrong. Besides, this

> +	tagger_len     = header_end > tagger_line ?
> +			(header_end - tagger_line) - 1 : 0;

is really ugly, what with in-line spaces, _and_ special casing.

> -		/* Verify the tagger line */
> +		/* Verify tagger line */

Hmpf.

Ciao,
Dscho

Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> diff --git a/mktag.c b/mktag.c
> index 4dbefab..2e70504 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)
>  	memcpy(type, type_line + 5, type_len);
>  	type[type_len] = '\0';
>  
> -	/* Verify that the object matches */
> -	if (verify_object(sha1, type))
> -		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
> -
> -	/* Verify the tag-name: we don't allow control characters or spaces in it */
> -	tag_line += 4;
> -	for (;;) {
> -		unsigned char c = *tag_line++;
> -		if (c == '\n')
> -			break;
> -		if (c > ' ')
> -			continue;
> -		return error("char" PD_FMT ": could not verify tag name", tag_line - data);
> +	{
> +		unsigned long i;

Do you realize that half of your diff consists of reindenting, just 
because you introduced this ugly construct, instead of being a good boy 
and put the declarations where they belong -- at the beginning of the 
function (or if it exists, block)?

> +		/* Verify the tag-name: we don't allow control characters or spaces in it */
> +		for (i = 4;;) {

Yes, you can write this construct. That does not change the fact that it 
gives me eye cancer.

Ciao,
Dscho

Re: [PATCH 15/21] Make tag names (i.e. the tag object's "tag" line) optional

[Johannes Schindelin]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> The tag line is now optional. If not given in the tag object data, it
> defaults to the empty string ("") in the parsed tag object.
> 
> The patch also adds a change to git-show; when asked to display a tag
> object with no name (missing "tag" header), we will show the tag's sha1
> instead of an empty string.
> 
> Finally the patch includes some tweaks to the selftests to make them 
> work with optional tag names.

If you don't actually _test_ missing tag names, you might just as well 
leave the tests alone.

> -					t->tag,
> +					*(t->tag) ? t->tag : name,

This is misleading. What you wanted to say is t->tag[0] == '\0', or 
*(t->tag) == '\0'.

As you wrote it, you have to think a couple of times why it is okay to 
dereference t->tag, to check if you say t->tag.

Besides, it breaks if you _do_ have an empty tag. In that case, I _want_ 
to see that it is actually empty, and _not_ the SHA1 substituted for it.

Ciao,
Dscho

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Johannes Schindelin]

Hi,

On Sun, 10 Jun 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ...  Guess how surprised 
> > _I_ was, when I hit the error message which made me go mad.
> 
> To be fair, that ugly "char%d" was taken from mktag and not
> Johan's invention.

Yes, I should have said that. I tried to hint to this by "you could just 
as well clean the code up", meaning the existing code.

Now, _that_ would be a patch I'd be really thankful for.

As for the general direction of implementing notes as tags: If you want to 
make them fetchable, you have to deal with conflicts. If you want to be 
able to amend notes, _especially_ when they should be fetchable, you want 
a history on them.

Which makes me think that tags are not the right object type for notes.

But I guess I'll just wait if somebody actually comments on my RFC for 
lightweight commit annotations (that's what I put into that discussion). 
BTW I just realized that I marked it [PATCH], while it should have been 
[RFC]. Sorry.

Ciao,
Dscho

[PATCH 0/4] Restructure the tag object

[Johan Herland]

Ok, I'm pulling the 21-part patch series from hell. It's just not worth all
the flak. Here's a 4-part patch series that tries to do the changes needed
without all the crap^Wrefactoring.

Obviously this patch series does none of the much needed cleanup in this
part of the code (e.g. better error messages, specifying encodings of header
fields, possibly unifying the common parts between the parser and the
verifier). I'll leave that cleanup to someone who writes less crappy code.

Here's the shortlog for the series:

Johan Herland (4):
      Make tag names (i.e. the tag object's "tag" line) optional
      Introduce optional "keywords" on tag objects
      Documentation/git-mktag: Document the changes in tag object structure
      git-mktag tests: Expand on mktag selftests according to the new tag object structure

 Documentation/git-mktag.txt |   38 +++++++---
 mktag.c                     |   65 +++++++++++-----
 t/t3800-mktag.sh            |  172 ++++++++++++++++++++++++++++++++++++++++---
 tag.c                       |   44 +++++++++--
 tag.h                       |    3 +-
 5 files changed, 270 insertions(+), 52 deletions(-)


...Johan

[PATCH 1/4] Make tag names (i.e. the tag object's "tag" line) optional

[Johan Herland]

The tag line is now optional. If not given in the tag object data, it
defaults to the empty string ("") in the parsed tag object.

Also includes selftest tweaks to make them work with optional tag names.

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c          |   37 ++++++++++++++++++++-----------------
 t/t3800-mktag.sh |   16 +++++++++-------
 tag.c            |   18 ++++++++++++------
 tag.h            |    2 +-
 4 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/mktag.c b/mktag.c
index 070bc96..5e80d3d 100644
--- a/mktag.c
+++ b/mktag.c
@@ -2,16 +2,15 @@
 #include "tag.h"
 
 /*
- * A signature file has a very simple fixed format: four lines
- * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
+ * A signature file has a very simple format: 3-4 lines
+ * of "object <sha1>" + "type <typename>" + "tag <tagname>" (optional) +
  * "tagger <committer>", followed by a blank line, a free-form tag
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *
- * The first three lines are guaranteed to be at least 63 bytes:
- * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, and "tag .\n" at 6 bytes is the
- * shortest single-character-tag line.
+ * The first two lines are guaranteed to be at least 57 bytes:
+ * "object <sha1>\n" is 48 bytes and "type tag\n" at 9 bytes is the
+ * shortest possible type-line.
  *
  * We also artificially limit the size of the full object to 8kB.
  * Just because I'm a lazy bastard, and if you can't fit a signature
@@ -52,7 +51,7 @@ static int verify_tag(char *buffer, unsigned long size)
 	unsigned char sha1[20];
 	const char *object, *type_line, *tag_line, *tagger_line;
 
-	if (size < 64)
+	if (size < 58)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
 	buffer[size] = 0;
@@ -75,8 +74,6 @@ static int verify_tag(char *buffer, unsigned long size)
 	if (!tag_line)
 		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
 	tag_line++;
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
 
 	/* Get the actual type */
 	typelen = tag_line - type_line - strlen("type \n");
@@ -91,14 +88,20 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
 
 	/* Verify the tag-name: we don't allow control characters or spaces in it */
-	tag_line += 4;
-	for (;;) {
-		unsigned char c = *tag_line++;
-		if (c == '\n')
-			break;
-		if (c > ' ')
-			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
+	if (!memcmp(tag_line, "tag ", 4)) {
+		if (tag_line[4] == '\n')
+			return error("char" PD_FMT ": no \"tag \" found",
+					tag_line - buffer);
+		tag_line += 4;
+		for (;;) {
+			unsigned char c = *tag_line++;
+			if (c == '\n')
+				break;
+			if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify tag name",
+					tag_line - buffer);
+		}
 	}
 
 	/* Verify the tagger line */
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 7c7e433..ca90662 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -100,13 +100,14 @@ check_verify_failure '"type" line eol check'
 #  6. tag line label check #1
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
-type tag
+object $head
+type commit
 xxx mytag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char57: no "tag " found$
+^error: char60: could not find "tagger"$
 EOF
 
 check_verify_failure '"tag" line label check #1'
@@ -115,13 +116,14 @@ check_verify_failure '"tag" line label check #1'
 #  7. tag line label check #2
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
-type taggggggggggggggggggggggggggggggg
+object $head
+type commit
 tag
+tagger a
 EOF
 
 cat >expect.pat <<EOF
-^error: char87: no "tag " found$
+^error: char60: could not find "tagger"$
 EOF
 
 check_verify_failure '"tag" line label check #2'
@@ -130,7 +132,7 @@ check_verify_failure '"tag" line label check #2'
 #  8. type line type-name length check
 
 cat >tag.sig <<EOF
-object 779e9b33986b1c2670fff52c5067603117b3e895
+object $head
 type taggggggggggggggggggggggggggggggg
 tag mytag
 EOF
diff --git a/tag.c b/tag.c
index bbacd59..a7a3454 100644
--- a/tag.c
+++ b/tag.c
@@ -44,7 +44,7 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
                 return 0;
         item->object.parsed = 1;
 
-	if (size < 64)
+	if (size < 58)
 		return -1;
 	if (memcmp("object ", data, 7) || get_sha1_hex((char *) data + 7, sha1))
 		return -1;
@@ -54,13 +54,17 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 		return -1;
 
 	tag_line = strchr(type_line, '\n');
-	if (!tag_line || memcmp("tag ", ++tag_line, 4))
+	if (!tag_line)
 		return -1;
 
-	sig_line = strchr(tag_line, '\n');
-	if (!sig_line)
-		return -1;
-	sig_line++;
+	if (!memcmp("tag ", ++tag_line, 4)) {
+		sig_line = strchr(tag_line, '\n');
+		if (!sig_line)
+			return -1;
+		sig_line++;
+	}
+	else
+		sig_line = tag_line;
 
 	typelen = tag_line - type_line - strlen("type \n");
 	if (typelen >= 20)
@@ -68,6 +72,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 	memcpy(type, type_line + 5, typelen);
 	type[typelen] = '\0';
 	taglen = sig_line - tag_line - strlen("tag \n");
+	if (taglen < 0) /* missing tag name */
+		taglen = 0;
 	item->tag = xmalloc(taglen + 1);
 	memcpy(item->tag, tag_line + 4, taglen);
 	item->tag[taglen] = '\0';
diff --git a/tag.h b/tag.h
index 7a0cb00..7e0abbe 100644
--- a/tag.h
+++ b/tag.h
@@ -8,7 +8,7 @@ extern const char *tag_type;
 struct tag {
 	struct object object;
 	struct object *tagged;
-	char *tag;
+	char *tag;       /* optional, may be empty ("") */
 	char *signature; /* not actually implemented */
 };
 
-- 
1.5.2.1.144.gabc40

[PATCH 2/4] Introduce optional "keywords" on tag objects

[Johan Herland]

This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object. There are two "special"
keywords, however: "tag" and "note": When the "keywords" header is
missing, its default value is set to "tag" if a "tag" header is
present; else the default "keywords" value is set to "note". The
"keywords" header is meant to be used by porcelains for classifying
different types of tag objects. This classification may then be used to
filter tag objects in the presentation layer (e.g. by implementing
extra filter options to --decorate, etc.).

The encoding rules for keywords are identical to those of tag names.

Signed-off-by: Johan Herland <johan@herland.net>
---
 mktag.c |   30 ++++++++++++++++++++++++++----
 tag.c   |   30 +++++++++++++++++++++++++-----
 tag.h   |    1 +
 3 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/mktag.c b/mktag.c
index 5e80d3d..37e10c6 100644
--- a/mktag.c
+++ b/mktag.c
@@ -2,9 +2,10 @@
 #include "tag.h"
 
 /*
- * A signature file has a very simple format: 3-4 lines
+ * A signature file has a very simple format: 3-5 lines
  * of "object <sha1>" + "type <typename>" + "tag <tagname>" (optional) +
- * "tagger <committer>", followed by a blank line, a free-form tag
+ * "keywords <keywords>" (optional) + "tagger <committer>",
+ * followed by a blank line, a free-form tag
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *
@@ -49,7 +50,7 @@ static int verify_tag(char *buffer, unsigned long size)
 	int typelen;
 	char type[20];
 	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line;
+	const char *object, *type_line, *tag_line, *keywords_line, *tagger_line;
 
 	if (size < 58)
 		return error("wanna fool me ? you obviously got the size wrong !");
@@ -104,8 +105,29 @@ static int verify_tag(char *buffer, unsigned long size)
 		}
 	}
 
+	/* Verify the keywords: disallow ctrl chars, spaces and double commas */
+	keywords_line = tag_line;
+
+	if (!memcmp(tag_line, "keywords ", 9)) {
+		if (tag_line[9] == '\n')
+			return error("char" PD_FMT ": no \"keywords \" found",
+					keywords_line - buffer);
+		keywords_line += 9;
+		for (;;) {
+			unsigned char c = *keywords_line++;
+			if (c == '\n')
+				break;
+			if (c == ',' && *keywords_line == ',')
+				/* double commas. fall through to error() */;
+			else if (c > ' ')
+				continue;
+			return error("char" PD_FMT ": could not verify keywords",
+					keywords_line - buffer);
+		}
+	}
+
 	/* Verify the tagger line */
-	tagger_line = tag_line;
+	tagger_line = keywords_line;
 
 	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
 		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
diff --git a/tag.c b/tag.c
index a7a3454..b74f09f 100644
--- a/tag.c
+++ b/tag.c
@@ -35,9 +35,9 @@ struct tag *lookup_tag(const unsigned char *sha1)
 
 int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 {
-	int typelen, taglen;
+	int typelen, taglen, keywordslen;
 	unsigned char sha1[20];
-	const char *type_line, *tag_line, *sig_line;
+	const char *type_line, *tag_line, *keywords_line, *sig_line;
 	char type[20];
 
         if (item->object.parsed)
@@ -58,25 +58,45 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
 		return -1;
 
 	if (!memcmp("tag ", ++tag_line, 4)) {
-		sig_line = strchr(tag_line, '\n');
+		keywords_line = strchr(tag_line, '\n');
+		if (!keywords_line)
+			return -1;
+		keywords_line++;
+	}
+	else
+		keywords_line = tag_line;
+
+	if (!memcmp("keywords ", keywords_line, 9)) {
+		sig_line = strchr(keywords_line, '\n');
 		if (!sig_line)
 			return -1;
 		sig_line++;
 	}
 	else
-		sig_line = tag_line;
+		sig_line = keywords_line;
 
 	typelen = tag_line - type_line - strlen("type \n");
 	if (typelen >= 20)
 		return -1;
 	memcpy(type, type_line + 5, typelen);
 	type[typelen] = '\0';
-	taglen = sig_line - tag_line - strlen("tag \n");
+	taglen = keywords_line - tag_line - strlen("tag \n");
 	if (taglen < 0) /* missing tag name */
 		taglen = 0;
 	item->tag = xmalloc(taglen + 1);
 	memcpy(item->tag, tag_line + 4, taglen);
 	item->tag[taglen] = '\0';
+	keywordslen = sig_line - keywords_line - strlen("keywords \n");
+	if (keywordslen > 0)
+		keywords_line += strlen("keywords ");
+	else { /* missing keywords */
+		if (taglen) /* tag name given */
+			keywords_line = "tag";
+		else
+			keywords_line = "note";
+		keywordslen = strlen(keywords_line);
+	}
+	item->keywords = xstrndup(keywords_line, keywordslen);
 
 	if (!strcmp(type, blob_type)) {
 		item->tagged = &lookup_blob(sha1)->object;
diff --git a/tag.h b/tag.h
index 7e0abbe..6e687b2 100644
--- a/tag.h
+++ b/tag.h
@@ -9,6 +9,7 @@ struct tag {
 	struct object object;
 	struct object *tagged;
 	char *tag;       /* optional, may be empty ("") */
+	char *keywords;  /* optional, defaults to (tag ? "tag" : "note") */
 	char *signature; /* not actually implemented */
 };
 
-- 
1.5.2.1.144.gabc40

[PATCH 3/4] Documentation/git-mktag: Document the changes in tag object structure

[Johan Herland]

The new structure of tag objects is documented.

Also some much-needed cleanup is done. E.g. remove the paragraph on the
8kB limit, since this limit was removed ages ago.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-mktag.txt |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt
index 0ac3be1..e6dfed6 100644
--- a/Documentation/git-mktag.txt
+++ b/Documentation/git-mktag.txt
@@ -8,29 +8,44 @@ git-mktag - Creates a tag object
 
 SYNOPSIS
 --------
-'git-mktag' < signature_file
+[verse]
+'git-mktag' < tag_data_file
 
 DESCRIPTION
 -----------
-Reads a tag contents on standard input and creates a tag object
+Reads tag object data on standard input and creates a tag object
 that can also be used to sign other objects.
 
 The output is the new tag's <object> identifier.
 
-Tag Format
+DISCUSSION
 ----------
-A tag signature file has a very simple fixed format: three lines of
+Tag object data has the following format
 
+[verse]
   object <sha1>
   type <typename>
-  tag <tagname>
+  tag <tagname>               (optional)
+  keywords <keywords>         (optional)
+  tagger <committer>
 
-followed by some 'optional' free-form signature that git itself
-doesn't care about, but that can be verified with gpg or similar.
+followed by a blank line and a free-form message and an optional
+signature that git itself doesn't care about, but that may be
+verified with gpg or similar.
 
-The size of the full object is artificially limited to 8kB.  (Just
-because I'm a lazy bastard, and if you can't fit a signature in that
-size, you're doing something wrong)
+In the above listing, `<sha1>` represents the object pointed to
+by this tag, `<typename>` is the type of the object pointed to
+("tag", "blob", "tree" or "commit"), `<tagname>` is the name of
+this tag object (and must correspond to the name of the corresponding
+ref (if any) in `.git/refs/`). `<keywords>` is a comma-separated
+list of keywords associated with this tag object, and `<committer>`
+holds the "`name <email>`" of the tag creator and timestamp of when
+the tag object was created (analogous to "committer" in commit
+objects).
+
+If "`tag <tagname>`" is omitted, <tagname> defaults to the empty
+string. If "`keywords <keywords>`" is omitted, <keywords> defaults
+to "`tag`" if a <tagname> was given, "`note`" otherwise.
 
 
 Author
@@ -39,7 +54,8 @@ Written by Linus Torvalds <torvalds@osdl.org>
 
 Documentation
 --------------
-Documentation by David Greaves, Junio C Hamano and the git-list <git@vger.kernel.org>.
+Documentation by Johan Herland, David Greaves, Junio C Hamano
+and the git-list <git@vger.kernel.org>.
 
 GIT
 ---
-- 
1.5.2.1.144.gabc40

[PATCH 4/4] git-mktag tests: Expand on mktag selftests according to the new tag object structure

[Johan Herland]

Some more tests are added to test the new "keywords" header.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3800-mktag.sh |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 152 insertions(+), 4 deletions(-)

diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index ca90662..cc2f246 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -174,7 +174,95 @@ EOF
 check_verify_failure 'verify tag-name check'
 
 ############################################################
-# 11. tagger line label check #1
+# 11. keywords line label check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+xxxxxxxx foo
+EOF
+
+cat >expect.pat <<EOF
+^error: char70: could not find "tagger"$
+EOF
+
+check_verify_failure '"keywords" line label check #1'
+
+############################################################
+# 12. keywords line label check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char70: could not find "tagger"$
+EOF
+
+check_verify_failure '"keywords" line label check #2'
+
+############################################################
+# 13. keywords line check #1
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char83: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #1'
+
+############################################################
+# 14. keywords line check #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,bar	baz
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char87: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #2'
+
+############################################################
+# 15. keywords line check #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords foo,,bar
+tagger bar@baz.com
+
+EOF
+
+cat >expect.pat <<EOF
+^error: char83: could not verify keywords$
+EOF
+
+check_verify_failure '"keywords" line check #3'
+
+############################################################
+# 16. tagger line label check #1
 
 cat >tag.sig <<EOF
 object $head
@@ -189,7 +277,7 @@ EOF
 check_verify_failure '"tagger" line label check #1'
 
 ############################################################
-# 12. tagger line label check #2
+# 17. tagger line label check #2
 
 cat >tag.sig <<EOF
 object $head
@@ -205,7 +293,7 @@ EOF
 check_verify_failure '"tagger" line label check #2'
 
 ############################################################
-# 13. create valid tag
+# 18. create valid tag #1
 
 cat >tag.sig <<EOF
 object $head
@@ -219,11 +307,71 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 14. check mytag
+# 19. check mytag
 
 test_expect_success \
     'check mytag' \
     'git-tag -l | grep mytag'
 
+############################################################
+# 20. create valid tag #2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #2' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 21. create valid tag #3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+keywords foo,bar,baz,spam,spam,spam,spam,spam,spam,spam,spam
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #3' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 22. create valid tag #4
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger another@example.com
+
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 23. create valid tag #5 (with empty message)
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+keywords note
+tagger a
+EOF
+
+test_expect_success \
+    'create valid tag #4' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
 
 test_done
-- 
1.5.2.1.144.gabc40

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Johan Herland]

On Sunday 10 June 2007, Johannes Schindelin wrote:
> As for the general direction of implementing notes as tags: If you want to 
> make them fetchable, you have to deal with conflicts. If you want to be 
> able to amend notes, _especially_ when they should be fetchable, you want 
> a history on them.

I'm not sure what kind of notes you're talking about here. If you're talking 
about my git-note concept, I designed notes to be immutable (thus not 
amendable) and there is therefore _no_ merging or potential for conflicts 
between notes. The only resolution needed is to figure out which order the 
notes for a given object should be presented. The default here is 
chronological sorting.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 0/4] Restructure the tag object

[Johannes Schindelin]

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

> Johan Herland (4):
>       Make tag names (i.e. the tag object's "tag" line) optional
>       Introduce optional "keywords" on tag objects
>       Documentation/git-mktag: Document the changes in tag object structure
>       git-mktag tests: Expand on mktag selftests according to the new tag object structure

Much nicer, thank you.

Ciao,
Dscho

Re: [PATCH 2/4] Introduce optional "keywords" on tag objects

[Johannes Schindelin]

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

> +	/* Verify the keywords: disallow ctrl chars, spaces and double commas */

What about Junio's suggestion, making it really strict at first, and only 
loosening it if we need to? IIRC it was alnum + '_', maybe even '-'.

Other than that, looks good to me. I trust that the test cases are 
exhaustive enough to support the patch from the practical side.

BTW this patch is exactly what I meant by conceptually closed. Thank you.

And please accept my apologies for my language. Reading some of it, I have 
to admit that it sounded as harsh as Junio suggested it to be. My only 
excuse is that I had an unplanned stay at the Paris airport for more than 
9 hours (after a night in the plane where I could hardly sleep), so I 
should really have stayed away from writing emails. But since you 
addressed your emails to me, I wanted to reply to you as soon as I had the 
chance to.

Ciao,
Dscho

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Johannes Schindelin]

Hi,

On Sun, 10 Jun 2007, Johan Herland wrote:

> On Sunday 10 June 2007, Johannes Schindelin wrote:
>
> > As for the general direction of implementing notes as tags: If you 
> > want to make them fetchable, you have to deal with conflicts. If you 
> > want to be able to amend notes, _especially_ when they should be 
> > fetchable, you want a history on them.
> 
> I'm not sure what kind of notes you're talking about here. If you're 
> talking about my git-note concept, I designed notes to be immutable 
> (thus not amendable) and there is therefore _no_ merging or potential 
> for conflicts between notes.

Okay, that is one way you can go about implementing notes.

> The only resolution needed is to figure out which order the notes for a 
> given object should be presented. The default here is chronological 
> sorting.

There are several problems with that approach I'd like to point out:

- In distributed environments, you can not rely on timestamps. Ever.

- If a note is deleted, you will fetch it again as long as the other side 
  did not delete it.

- You cannot undo a typo (since the notes are immutable, you would see 
  both versions), once the typoed note was fetched.

Basically, everything I see as a problem here suggests that note writing 
is very much like working on a branch. That's why I suggest to treat it 
exactly like a branch to begin with.

Ciao,
Dscho

Re: [PATCH 2/4] Introduce optional "keywords" on tag objects

[Johan Herland]

On Sunday 10 June 2007, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 10 Jun 2007, Johan Herland wrote:
> 
> > +	/* Verify the keywords: disallow ctrl chars, spaces and double commas */
> 
> What about Junio's suggestion, making it really strict at first, and only 
> loosening it if we need to? IIRC it was alnum + '_', maybe even '-'.

For now, I couldn't find a good reason why the set of allowed characters
for keywords should be smaller than for the tag name.
Feel free to tighten the set of characters before this makes it into a
release. However, if you do, the same tightening should be considered
for the tag name as well, I guess. Can't see any good reasons for why
one should be tighter than the other.

> And please accept my apologies for my language. Reading some of it, I have 
> to admit that it sounded as harsh as Junio suggested it to be. My only 
> excuse is that I had an unplanned stay at the Paris airport for more than 
> 9 hours (after a night in the plane where I could hardly sleep), so I 
> should really have stayed away from writing emails. But since you 
> addressed your emails to me, I wanted to reply to you as soon as I had the 
> chance to.

Apology accepted. I'm sorry my patch-series-from-hell came at such an
inconvenient time for you. 


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] Silence error messages unless 'thorough_verify' is set

[Johan Herland]

On Sunday 10 June 2007, Johannes Schindelin wrote:
> On Sun, 10 Jun 2007, Johan Herland wrote:
> > On Sunday 10 June 2007, Johannes Schindelin wrote:
> > > As for the general direction of implementing notes as tags: If you 
> > > want to make them fetchable, you have to deal with conflicts. If you 
> > > want to be able to amend notes, _especially_ when they should be 
> > > fetchable, you want a history on them.
> > 
> > I'm not sure what kind of notes you're talking about here. If you're 
> > talking about my git-note concept, I designed notes to be immutable 
> > (thus not amendable) and there is therefore _no_ merging or potential 
> > for conflicts between notes.
> 
> Okay, that is one way you can go about implementing notes.
> 
> > The only resolution needed is to figure out which order the notes for a 
> > given object should be presented. The default here is chronological 
> > sorting.
> 
> There are several problems with that approach I'd like to point out:
> 
> - In distributed environments, you can not rely on timestamps. Ever.

Not really, but that doesn't stop many programs from trying anyway...
(e.g. email clients). And still, it's not like the date (or sorting)
is crucial to the 'notes' concept or implementation.

> - If a note is deleted, you will fetch it again as long as the other side 
>   did not delete it.

Yep. This was considered an acceptable tradeoff in the design. But I
understand that some people won't like it.

> - You cannot undo a typo (since the notes are immutable, you would see 
>   both versions), once the typoed note was fetched.

Yep. Also a tradeoff in the design. Also going to piss off some
people, I guess.

> Basically, everything I see as a problem here suggests that note writing 
> is very much like working on a branch. That's why I suggest to treat it 
> exactly like a branch to begin with.

I see you point.


BTW, I have some patches implementing the 'notes' concept on top
of the softrefs patches. They're just lying around now waiting
to be cleaned up and sent to the list, but I'm not sure it's worth
it, since they don't add anything that's not in your lightweight
annotation patch...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/4] Introduce optional "keywords" on tag objects

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> For now, I couldn't find a good reason why the set of allowed characters
> for keywords should be smaller than for the tag name.

The set of allowed tag names excludes shell metacharacters,
primarily to help scripting.  I think keywords can share the
same reasoning to exclude them.

It also excludes '^', '~' and ':', because tag names can be used
in revision range expressions (i.e. prefix '^' is the "exclude
from the resulting set" operation, postfix "~<number>" is the
"Nth generation ancestor" operation) and general SHA-1
expression (i.e. infix ':' is the "find in the tree-ish the
object at path" operation).  These reasons would not apply to 
keywords.

Having said all of that, I suspect it is premature to talk about
keywords, as it is unclear what their intended use is.  What
kind of operations are useful on them?  

It does not count that "git cat-file tag" would show "keywords
blah" on the header instead of in body.  It is not a compelling
enough reason to introduce a new header type. grep would work
just fine for such a use.

On the other hand, for example, if (the syntax is totally made
up) we make '::keywords=foo::' expand to set of all tags that
have the specified keyword 'foo', and it turns out to be useful
to be able to say "git show ::keywords=foo::" instead of listing
individual tags, that kind of use case may make it a good reason
to add such a new header type.

Re: [PATCH 1/4] Make tag names (i.e. the tag object's "tag" line) optional

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> The tag line is now optional. If not given in the tag object data, it
> defaults to the empty string ("") in the parsed tag object.

Sorry, I may have missed the discussion.  I recall that we
talked about tagger line which may not exist for historical
reasons, but have we heard any reasoning behind this?

Re: [PATCH 1/4] Make tag names (i.e. the tag object's "tag" line) optional

[Johan Herland]

On Monday 11 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> 
> > The tag line is now optional. If not given in the tag object data, it
> > defaults to the empty string ("") in the parsed tag object.
> 
> Sorry, I may have missed the discussion.  I recall that we
> talked about tagger line which may not exist for historical
> reasons, but have we heard any reasoning behind this?

In the discussion that followed the first proof-of-concept of 'notes',
I argued that the "tag" header is _really_ only needed for
signed tag objects (although we want it for annotated tags as well),
and that it doesn't make sense to specify a tag name for unnamed 'note'
objects.

If the "tag" line remains mandatory, I'll have to construct an
artificial name for unnamed 'notes'...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/4] Introduce optional "keywords" on tag objects

[Johan Herland]

On Sunday 10 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > For now, I couldn't find a good reason why the set of allowed characters
> > for keywords should be smaller than for the tag name.
> 
> The set of allowed tag names excludes shell metacharacters,
> primarily to help scripting.  

It already does? Or are you proposing this? Right now the code doesn't 
enforce anything like this, AFAICS...

> I think keywords can share the same reasoning to exclude them.
> 
> It also excludes '^', '~' and ':', because tag names can be used 
> in revision range expressions (i.e. prefix '^' is the "exclude
> from the resulting set" operation, postfix "~<number>" is the
> "Nth generation ancestor" operation) and general SHA-1
> expression (i.e. infix ':' is the "find in the tree-ish the
> object at path" operation).  These reasons would not apply to 
> keywords.

I have nothing against limiting keywords to fairly small set, say
alphanumerics plus a couple of "safe" symbols. It just didn't make
sense to do this when I made the patch without doing it to the
tag name at the same time, and I'm not sure what that restricted
set should be, so I held off on it. Feel free to fix.

> Having said all of that, I suspect it is premature to talk about
> keywords, as it is unclear what their intended use is.  What
> kind of operations are useful on them?  
> 
> It does not count that "git cat-file tag" would show "keywords
> blah" on the header instead of in body.  It is not a compelling
> enough reason to introduce a new header type. grep would work
> just fine for such a use.
> 
> On the other hand, for example, if (the syntax is totally made
> up) we make '::keywords=foo::' expand to set of all tags that
> have the specified keyword 'foo', and it turns out to be useful
> to be able to say "git show ::keywords=foo::" instead of listing
> individual tags, that kind of use case may make it a good reason
> to add such a new header type.

Yes, this is what I'm thinking; using keywords to filter tag objects in 
various settings. Haven't thought much about the syntax yet, but as it 
would have to work on the command-line (possibly together with the other 
weird characters git uses for specifying revisions), I imagine a character 
set similar to the one for tag names should be good.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/4] Introduce optional "keywords" on tag objects

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Sunday 10 June 2007, Junio C Hamano wrote:
>> Johan Herland <johan@herland.net> writes:
>> > For now, I couldn't find a good reason why the set of allowed characters
>> > for keywords should be smaller than for the tag name.
>> 
>> The set of allowed tag names excludes shell metacharacters,
>> primarily to help scripting.  
>
> It already does? Or are you proposing this? Right now the code doesn't 
> enforce anything like this, AFAICS...

Ah, I was being silly.  I somehow thought you were talking about
valid names you can have under refs/tags/.

Re: [PATCH 1/4] Make tag names (i.e. the tag object's "tag" line) optional

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> If the "tag" line remains mandatory, I'll have to construct an
> artificial name for unnamed 'notes'...

Ah, silly me, now I remember.

But let's put this series on hold and see how versioned set of
notes from Johannes turns out.  It seems that people are in
favor of its simplicity, and even though it may be much less
expressive, it might be sufficient for certain class of
applications.  It one certainly is a much smaller and isolated
change that we could easily drop more easily if it does not work
out.