Re: [PATCH 2/2] parse_object(): check on-disk type of suspected blob
- From
Jeff King <peff@peff.net>
- Date
- Nov 21, 2022, 19:21 UTC
- Message-ID
- <Y3vP16OJSP19VMMy@coredump.intra.peff.net>
- In-Reply-To
- <221118.86cz9lgjxu.gmgdl@evledraar.gmail.com>
On Fri, Nov 18, 2022 at 01:36:23AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 18 quoted lines
> > - 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.
Show 23 quoted lines
> 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.
Show 6 quoted lines
> > 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