Re: [PATCH v2 09/10] mktag: use fsck instead of custom verify_tag()

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

Change the validation logic in "mktag" to use fsck's fsck_tag()
instead of its own custom parser. Curiously the logic for both dates
back to the same commit[1]. Let's unify them so we're not maintaining
two sets functions to verify that a tag is OK.

Moving to fsck_tag() required teaching it to optionally use some
validations that only the old mktag code could perform. That was done
in an earlier commit, the "extraHeaderEntry" and
"extraHeaderBodyNewline" tests being added here make use of that
logic.

There was other "mktag" validation logic that I think makes sense to
just remove. Namely:

I may not agree with the specifics in this list, but I agree that
the validation should be consistent between mktag and fsck.

+	switch (msg_type) {
+	case FSCK_WARN:
+	case FSCK_ERROR:
+	case FSCK_EXTRA:
+		/*
+		 * We treat both warnings and errors as errors, things
+		 * like missing "tagger" lines are "only" warnings
+		 * under fsck, we've always considered them an error.
+		 */

This is a good comment and design decision.  

As far as I am concerned, the only reason we warn on a missing
tagger line without erroring out is because very early versions of
Git did allow creating such tag objects, and there are annotated
tags in the wild that lack the tagger line.

+		fprintf_ln(stderr, "error: tag input does not pass fsck: %s", message);
+		return 1;
+	default:
+		BUG("%d (FSCK_IGNORE?) should never trigger this callback",
+		    msg_type);
 	}
-	return ret;
 }
 
-static int verify_tag(char *buffer, unsigned long size)
...
-	/* timezone, 5 digits [+-]hhmm, max. 1400 */
-	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
-	      strspn(tagger_line+1, "0123456789") == 4 &&
-	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
-		return error("char%"PRIuMAX": malformed tag timezone",
-			(uintmax_t) (tagger_line - buffer));
-	tagger_line += 6;
-
-	/* Verify the blank line separating the header from the body */
-	if (*tagger_line != '\n')
-		return error("char%"PRIuMAX": trailing garbage in tag header",
-			(uintmax_t) (tagger_line - buffer));
 
-	/* The actual stuff afterwards we don't care about.. */
-	return 0;

I do not see the "we do not want more than one blank line after the
header" in the original, which was one thing I was looking for after
seeing what 08/10 did.

+	fsck_options.extra = 1;
+	fsck_options.error_func = mktag_fsck_error_func;

I wonder if we really need the new .extra bit member in the
fsck_options.  Wouldn't it be sufficient to instead teach
fsck_error_func() that the extra level is a non-event?