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

[Jeff King <peff@peff.net>]

On Fri, Nov 18, 2022 at 01:36:23AM +0100, Ævar Arnfjörð Bjarmason wrote:

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

Just that it was one more round of refactoring than I did. :)

I agree that it's much more readable. It looks like the original hit
'next', so I'll send a patch on top.

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

Right. It's hitting the exact same code path as all of the other object
types now. You suggested adding to the test here, but I'd prefer not to
do that. Noticing that we have a type mismatch is what is fixed, and it
now does that just like all the other object types. Dealing with the
message reversal is orthogonal.

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

I did double-take on the "TODO" just because that is not our usual
pattern, but that was easily fixed. What I really don't like about the
"switch failure to success" pattern is that it requires rewriting the
test to expect the wrong thing! So when somebody later fixes the bug,
they get a confusing failure, but must also rewrite the test back to
what it originally should have been.

That was not too hard here, where it was just replacing a
test_must_fail, but that earlier hunk in cf10c5b4cf that actualy adds in
expected output (that we know is the wrong thing to be printing!) seems
a bit over the top to me. Anybody who encounters it has to dig into the
history to understand what is going on.

-Peff