Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> This patch series introduces detailed checking of tag objects when calling
>> git fsck, and also when transfer.fsckobjects is set to true.
>>
>> To this end, the fsck machinery is reworked to accept the buffer and size
>> of the object to check, and for commit and tag objects, we verify that the
>> buffers contain an end of header (i.e. an empty line) to guarantee that our
>> checks do not run beyond the buffer.
>
> Overall they looked sensible and I am trying to queue them on 'pu'
> for today's pushout.
>
> Thanks.

I was expecting to see interesting interactions with the "oops, fsck
was not exiting with non-zero status in some cases" fix by Peff with
the patch 5/6 of this series that adds two new tests to t1450, but
because the breakage in these two cases are both only warning-events
and not error-events, I didn't prefix "git fsck" with test_must_fail
after all.

Are there some more serious kind of breakages that the new code
actually catches as errors which the new tests are not exercising?

The second test however did need the fix I mentioned in the review
to skip ident validation when we know the buffer does not point at a
potential ident to pass, though (see below, which is what I queued
on the tip of your series as a "SQUASH???" single fix-up patch).

Please do an equivalent in the individual patches that introduce
these buglets in a reroll (i.e. not like I did here, which was done
this way only because I needed to make sure that day's integration
result passes the test suite with minimum effort on my end ;-)).

Thanks.

 fsck.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fsck.c b/fsck.c
index 8341a30..6e6ea25 100644
--- a/fsck.c
+++ b/fsck.c
@@ -248,14 +248,14 @@ static int require_end_of_header(const void *data, unsigned long size,
 		switch (buffer[i]) {
 		case '\0':
 			return error_func(obj, FSCK_ERROR,
-				"invalid message: NUL at offset %d", i);
+				"invalid header: NUL at offset %d", i);
 		case '\n':
 			if (i + 1 < size && buffer[i + 1] == '\n')
 				return 0;
 		}
 	}
 
-	return error_func(obj, FSCK_ERROR, "invalid buffer: missing empty line");
+	return error_func(obj, FSCK_ERROR, "invalid buffer: missing end of header");
 }
 
 static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
@@ -426,14 +426,16 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 		error_func(&tag->object, FSCK_WARN, "invalid 'tag' name: %s", buffer);
 	buffer = eol + 1;
 
-	if (!skip_prefix(buffer, "tagger ", &buffer)) {
+	if (!skip_prefix(buffer, "tagger ", &buffer))
 		/* early tags do not contain 'tagger' lines; warn only */
-		error_func(&tag->object, FSCK_WARN, "invalid format - expected 'tagger' line");
-	}
-	ret = fsck_ident(&buffer, &tag->object, error_func);
+		error_func(&tag->object, FSCK_WARN,
+			   "invalid format - expected 'tagger' line");
+	else
+		ret = fsck_ident(&buffer, &tag->object, error_func);
 
 done:
 	free(to_free);
+	strbuf_release(&sb);
 	return ret;
 }
 
-- 
2.1.0-451-g8daadf2