Re: [PATCH 2/2] parse_object(): check on-disk type of suspected blob

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 18, 2022, 00:36 UTC
Message-ID: <221118.86cz9lgjxu.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y3a4jKzsHSooYFqj@coredump.intra.peff.net>

On Thu, Nov 17 2022, Jeff King wrote:

Show 20 quoted lines

> I reworked the conditional a bit so that instead of:
>
>   if ((suspected_blob && oid_object_info() == OBJ_BLOB)
>       (no_clue && oid_object_info() == OBJ_BLOB)
>
> we have the simpler:
>
>   if ((suspected_blob || no_clue) && oid_object_info() == OBJ_BLOB)
> [...]
> @@ -286,8 +286,8 @@ struct object *parse_object_with_flags(struct repository *r,
>  			return &commit->object;
>  	}
>  
> -	if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||
> -	    (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {
> +	if ((!obj || (obj && obj->type == OBJ_BLOB)) &&
> +	    oid_object_info(r, oid, NULL) == OBJ_BLOB) {
>  		if (!skip_hash && stream_object_signature(r, repl) < 0) {
>  			error(_("hash mismatch %s"), oid_to_hex(oid));
>  			return NULL;

But why:

	if ((!x || (x && x->m)) && ...)

Instead of:

	if ((!x || x->m)) && ...)

If "!obj" is false then "obj" must be non-NULL, so you don't need to check it again and can lose the "obj &&".

Show 8 quoted lines

> [...]
> So this fixes one of the lingering expect_failure cases from 0616617c7e
> (t: introduce tests for unexpected object types, 2019-04-09).  That test
> works by peeling a tag that claims to point to a blob (triggering us to
> create the struct), but really points to something else, which we later
> discover when we call parse_object() as part of the actual traversal).
> Prior to this commit, we'd quietly check the sha1 and mark the blob as
> "parsed". Now we correctly complain about the mismatch.

I applied this on top of "master", and adjusted your test to be this instead:

	test_expect_success 'traverse unexpected non-blob tag (lone)' '
		cat >expect <<-EOF &&
		error: object $commit is a blob, not a commit
		fatal: bad object $commit
		EOF
		test_must_fail git rev-list --objects $tag >out 2>actual &&
		test_must_be_empty out &&
		test_cmp expect actual
	'

Which passes, showing that we're still not correctly identifying it, but we are doing it for the purposes of erroring out, but the incorrect type persists.

Now, this all does seem quite familiar... :) : https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/

I.e. that's the rest of the fix for this issue. I applied this change on my local branch with that, and they combine nicely. the "test_must_fail" here works as intended, *and* we'll correctly report & store the type.

Show 8 quoted lines

> As an aside, I found the "this test is marked as success but testing the
> wrong thing" pattern here confusing to deal with (since I had to dig in
> history to understand what was going on and what the test was _supposed_
> to say).
>
> It comes from cf10c5b4cf (rev-list tests: don't hide abort() in
> "test_expect_failure", 2022-03-07). I'm skeptical that it was worth
> switching those tests for leak detection purposes.

It's not just for leak detection purposes that it was a good idea to switch it away from test_expect_failure, but also that we've been ensuring that this didn't turn into a segfault all this time by not using "test_expect_failure".

> But more importantly, it looks like pw/test-todo would provide us with a
> much nicer pattern there. It seems to be stalled on review, so let's see
> if we can get that moving again.

The "TODO (should fail!)" didn't stand out? But yeah, having a "todo" or "test_expect_todo" or "test_expect_failure" not suck would be nice.

FWIW I think https://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/ outlines a good way forward for it that I think should make everyone happy.

Show 13 quoted lines

> diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
> index 4a9a4436e2..9350b5fd2c 100755
> --- a/t/t6102-rev-list-unexpected-objects.sh
> +++ b/t/t6102-rev-list-unexpected-objects.sh
> @@ -121,8 +121,8 @@ test_expect_success 'setup unexpected non-blob tag' '
>  	tag=$(git hash-object -w --literally -t tag broken-tag)
>  '
>  
> -test_expect_success 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '
> -	git rev-list --objects $tag
> +test_expect_success 'traverse unexpected non-blob tag (lone)' '
> +	test_must_fail git rev-list --objects $tag
>  '

I think you'll either want the test_cmp I noted above, or to do that in a subsequent test_expect_failure.

I know that your stance is that you prefer not to "test the bad behavior" as it were. Personally I thought an all-caps TODO comment might make it less confusing, but anyway.

In this case your commit message claims you're happy with the end result, so I think you'd want to test what we actually emit on stderr, as it's quite ... unintuative.

Or, which I think probably makes more sense, add that as a subsequent test_expect_failure or whatever. FWIW this somewhat un-idiomatic pattern will get around the current caveats with it:

	test_expect_success .... '
		... >actual.non-blob-tag
	'
	test_lazy_prereq HAVE_NON_BLOB_TAG 'test -e actual.non-blob-tag'
	test_expect_failure HAVE_NON_BLOB_TAG '...' '
		cat >expect.non-blob-tag <<-\EOF &&
		...
		EOF
		test_cmp expect.non-blob-tag actual.non-blob-tag
	'

I.e. peel off the 'test_cmp" that should have a known-good state from the already-good status code.