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:

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

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

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

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

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

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

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

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

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

Hmpf.

Ciao,
Dscho