Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
> - const char *type_line, *tag_line, *tagger_line;
> - unsigned long type_len, tag_len;
> + const char *type_line, *tag_line, *tagger_line;
> + unsigned long type_len, tag_len, tagger_len;
> + const char *header_end;
This is ugly. Really ugly. Besides, it breaks the minimal patch paradigm.
> @@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
> if (size < 64)
> return error("Tag object failed preliminary size check");
>
> - /* Verify object line */
> + /* Verify mandatory object line */
> if (prefixcmp(data, "object "))
> return error("Tag object (@ char 0): "
> "Does not start with \"object \"");
Hmph. I think everybody reading C code understands that this is mandatory.
I even think that the comment is useless. It is sort of a
code-in-human-language duplicated code.
> - /* Verify the tagger line */
> + /*
> + * Verify mandatory tagger line, but only when we're checking
> + * thoroughly, i.e. on inserting a new tag, and on fsck.
> + * There are existing tag objects without a tagger line (most
> + * notably the "v0.99" tag in the main git repo), and we don't
> + * want to fail parsing on these.
> + */
I maintain that even with thorough checking, it is _wrong_ to error on a
missing tagger. Since we have to deal with tagger-less tags _anyway_, and
since it is not like you could really do something about it (the tag is
immutable), you should go with a warning.
> - * Calculate lengths of header fields.
> + * Calculate lengths of header fields (0 for fields that are not given).
Does that really make sense? You effectively treat a missing field as if
it were empty. IMHO that is wrong. Besides, this
> + tagger_len = header_end > tagger_line ?
> + (header_end - tagger_line) - 1 : 0;
is really ugly, what with in-line spaces, _and_ special casing.
> - /* Verify the tagger line */
> + /* Verify tagger line */
Hmpf.
Ciao,
Dscho
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
