threads / discuss / 8491

error: char103: premature end of data

Subject: error: char103: premature end of data

## tl;dr

90 messages between Jun 7, 2007 and Jun 11, 2007.

replies: 89people: 4as markdown or json

Johannes Schindelin· Jun 7, 2007, 22:50 UTC · lore
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

Johan Herland· Jun 7, 2007, 23:05 UTC · re: Johannes Schindelin · lore

Re: error: char103: premature end of data

On Friday 08 June 2007, Johannes Schindelin wrote:
Show 6 quoted lines
> 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
Johannes Schindelin· Jun 7, 2007, 23:28 UTC · re: Johan Herland · lore

Re: error: char103: premature end of data

Hi,
On Fri, 8 Jun 2007, Johan Herland wrote:
Show 10 quoted lines
> 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

Johan Herland· Jun 7, 2007, 23:47 UTC · re: Johannes Schindelin · lore

Re: error: char103: premature end of data

On Friday 08 June 2007, Johannes Schindelin wrote:
Show 16 quoted lines
> 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.

Show 9 quoted lines
> > 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
Johannes Schindelin· Jun 7, 2007, 23:55 UTC · re: Johan Herland · lore

Re: error: char103: premature end of data

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

Johan Herland· Jun 8, 2007, 00:08 UTC · re: Johan Herland · lore

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

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
Junio C Hamano· Jun 8, 2007, 06:05 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 6 quoted lines
> 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.

Johan Herland· Jun 8, 2007, 08:18 UTC · re: Junio C Hamano · lore

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

On Friday 08 June 2007, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Jun 8, 2007, 16:06 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 15 quoted lines
> 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.
Johan Herland· Jun 9, 2007, 00:10 UTC · re: Junio C Hamano · lore

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

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
Johan Herland· Jun 9, 2007, 00:12 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:13 UTC · re: Johan Herland · lore

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

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
Junio C Hamano· Jun 9, 2007, 18:01 UTC · re: Johan Herland · lore

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

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.

Johan Herland· Jun 9, 2007, 18:28 UTC · re: Junio C Hamano · lore

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

On Saturday 09 June 2007, Junio C Hamano wrote:
Show 25 quoted lines
> 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
Johan Herland· Jun 9, 2007, 19:42 UTC · re: Johan Herland · lore

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

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:
Show 21 quoted lines
> 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
Johannes Schindelin· Jun 10, 2007, 06:48 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 11 quoted lines
> +#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

Junio C Hamano· Jun 10, 2007, 08:15 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> 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.
Johannes Schindelin· Jun 10, 2007, 10:08 UTC · re: Junio C Hamano · lore

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

Hi,
On Sun, 10 Jun 2007, Junio C Hamano wrote:
Show 7 quoted lines
> 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

Johan Herland· Jun 10, 2007, 12:10 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Jun 10, 2007, 18:51 UTC · re: Johan Herland · lore

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

Hi,
On Sun, 10 Jun 2007, Johan Herland wrote:
Show 11 quoted lines
> 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

Johan Herland· Jun 10, 2007, 19:16 UTC · re: Johannes Schindelin · lore

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

On Sunday 10 June 2007, Johannes Schindelin wrote:
Show 21 quoted lines
> 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
Johan Herland· Jun 10, 2007, 11:47 UTC · re: Junio C Hamano · lore

[PATCH 0/4] Restructure the tag object

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
Johan Herland· Jun 10, 2007, 11:49 UTC · re: Johan Herland · lore

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

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
Junio C Hamano· Jun 10, 2007, 22:46 UTC · re: Johan Herland · lore

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

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?

Johan Herland· Jun 10, 2007, 23:01 UTC · re: Junio C Hamano · lore

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

On Monday 11 June 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Junio C Hamano· Jun 11, 2007, 01:11 UTC · re: Johan Herland · lore

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

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.

Johan Herland· Jun 10, 2007, 11:50 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 18:42 UTC · re: Johan Herland · lore

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

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

Johan Herland· Jun 10, 2007, 19:04 UTC · re: Johannes Schindelin · lore

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

On Sunday 10 June 2007, Johannes Schindelin wrote:
Show 8 quoted lines
> 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.

Show 7 quoted lines
> 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
Junio C Hamano· Jun 10, 2007, 21:43 UTC · re: Johan Herland · lore

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

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.

Johan Herland· Jun 10, 2007, 23:16 UTC · re: Junio C Hamano · lore

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

On Sunday 10 June 2007, Junio C Hamano wrote:
Show 6 quoted lines
> 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...

Show 9 quoted lines
> 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.

Show 15 quoted lines
> 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
Junio C Hamano· Jun 11, 2007, 01:01 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 10 quoted lines
> 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/.

Johan Herland· Jun 10, 2007, 11:50 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 10, 2007, 11:50 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 18:35 UTC · re: Johan Herland · lore

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

Hi,
On Sun, 10 Jun 2007, Johan Herland wrote:
Show 5 quoted lines
> 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

Johan Herland· Jun 9, 2007, 00:13 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 9, 2007, 02:54 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 17 quoted lines
> 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?
Show 8 quoted lines
> -{
> -	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.

Show 11 quoted lines
> -	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).

Show 9 quoted lines
>  	/* 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?
Show 5 quoted lines
> @@ -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.

Show 6 quoted lines
> -        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.
Show 6 quoted lines
> @@ -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

Johan Herland· Jun 9, 2007, 10:49 UTC · re: Johannes Schindelin · lore

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

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.

Show 7 quoted lines
> 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.

Show 6 quoted lines
> > -	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.

Show 11 quoted lines
> >  	/* 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.

Show 12 quoted lines
> > @@ -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.

Show 7 quoted lines
> > -	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
Johan Herland· Jun 9, 2007, 00:14 UTC · re: Johan Herland · lore

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

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
Junio C Hamano· Jun 9, 2007, 18:01 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 22 quoted lines
> 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.

Show 18 quoted lines
> 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].
Johannes Schindelin· Jun 10, 2007, 07:49 UTC · re: Junio C Hamano · lore

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

Hi,
On Sat, 9 Jun 2007, Junio C Hamano wrote:
Show 5 quoted lines
> 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

Johan Herland· Jun 9, 2007, 00:14 UTC · re: Johan Herland · lore

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

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
Junio C Hamano· Jun 9, 2007, 18:01 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 27 quoted lines
> 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?

Johan Herland· Jun 10, 2007, 00:45 UTC · re: Junio C Hamano · lore

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

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:
Show 22 quoted lines
> 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
Johannes Schindelin· Jun 10, 2007, 08:06 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 49 quoted lines
> -	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

Johan Herland· Jun 9, 2007, 00:15 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:26 UTC · re: Johan Herland · lore

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

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.
Johan Herland· Jun 9, 2007, 21:34 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 11 quoted lines
> 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
Johannes Schindelin· Jun 10, 2007, 08:14 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 14 quoted lines
> 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

Johannes Schindelin· Jun 10, 2007, 09:01 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 23 quoted lines
> 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

Johan Herland· Jun 9, 2007, 00:15 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:31 UTC · re: Johan Herland · lore

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

On 6/9/07, Johan Herland <johan@herland.net> wrote:
Show 9 quoted lines
> +               /* 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?!

Johan Herland· Jun 9, 2007, 21:39 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 15 quoted lines
> 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
Johannes Schindelin· Jun 10, 2007, 08:22 UTC · re: Johan Herland · lore

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

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.

Johan Herland· Jun 9, 2007, 00:15 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:16 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:39 UTC · re: Johan Herland · lore

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

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?

Johan Herland· Jun 9, 2007, 21:42 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 8 quoted lines
> 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
Johan Herland· Jun 9, 2007, 00:16 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:37 UTC · re: Johan Herland · lore

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

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.

Johan Herland· Jun 9, 2007, 21:46 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 9 quoted lines
> 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
Alex Riesen· Jun 9, 2007, 22:00 UTC · re: Johan Herland · lore

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

On 6/9/07, Johan Herland <johan@herland.net> wrote:
Show 13 quoted lines
> 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.

Johan Herland· Jun 9, 2007, 22:05 UTC · re: Alex Riesen · lore

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

On Sunday 10 June 2007, Alex Riesen wrote:
Show 15 quoted lines
> 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.

Show 7 quoted lines
> > 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
Johannes Schindelin· Jun 10, 2007, 08:38 UTC · re: Johan Herland · lore

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

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

Johan Herland· Jun 9, 2007, 00:17 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 08:38 UTC · re: Johan Herland · lore

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

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

Johan Herland· Jun 9, 2007, 00:17 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:42 UTC · re: Johan Herland · lore

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

On 6/9/07, Johan Herland <johan@herland.net> wrote:
Show 9 quoted lines
> +/*
> + * 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.

Johan Herland· Jun 9, 2007, 21:47 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 13 quoted lines
> 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
Johannes Schindelin· Jun 10, 2007, 08:41 UTC · re: Johan Herland · lore

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

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

Johan Herland· Jun 9, 2007, 00:18 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 08:45 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 17 quoted lines
> +	/*
> +	 * 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

Johan Herland· Jun 9, 2007, 00:18 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 08:52 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 5 quoted lines
> 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

Johannes Schindelin· Jun 10, 2007, 08:58 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 5 quoted lines
> -	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.
Show 9 quoted lines
> @@ -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.

Show 8 quoted lines
> -	/* 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

Johan Herland· Jun 9, 2007, 00:19 UTC · re: Johan Herland · lore

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

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
Johannes Schindelin· Jun 10, 2007, 09:07 UTC · re: Johan Herland · lore

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

Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Show 9 quoted lines
> 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

Johan Herland· Jun 9, 2007, 00:19 UTC · re: Johan Herland · lore

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

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
Alex Riesen· Jun 9, 2007, 21:52 UTC · re: Johan Herland · lore

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

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?
Show 9 quoted lines
> +                       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?
Johan Herland· Jun 9, 2007, 22:00 UTC · re: Alex Riesen · lore

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

On Saturday 09 June 2007, Alex Riesen wrote:
Show 6 quoted lines
> 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?

Show 11 quoted lines
> > +                       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
Johan Herland· Jun 9, 2007, 22:36 UTC · re: Alex Riesen · lore

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

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
Junio C Hamano· Jun 10, 2007, 00:05 UTC · re: Alex Riesen · lore

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

"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 18 quoted lines
> 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".

Johan Herland· Jun 10, 2007, 00:35 UTC · re: Junio C Hamano · lore

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

Signed-off-by: Johan Herland <johan@herland.net>
---
On Sunday 10 June 2007, Junio C Hamano wrote:
Show 5 quoted lines
> "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

	
Junio C Hamano· Jun 10, 2007, 01:33 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 11 quoted lines
> 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.

Johan Herland· Jun 9, 2007, 00:20 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:20 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:20 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:21 UTC · re: Johan Herland · lore

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

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
Johan Herland· Jun 9, 2007, 00:21 UTC · re: Johan Herland · lore

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

On Monday 28 May 2007, Junio C Hamano wrote:
Show 13 quoted lines
> 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
Johannes Schindelin· Jun 7, 2007, 23:11 UTC · re: Johannes Schindelin · lore

Re: error: char103: premature end of data

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

← back to recent threads