| Message ID | Y3a4jKzsHSooYFqj@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8db2dad7a045e376b9c4f51ddd33da43c962e3a4 |
| Headers | show |
| Series | fixing parse_object() check for type mismatch | expand |
On Thu, Nov 17 2022, Jeff King wrote: > 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 &&". > [...] > 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. > 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. > 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.
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
diff --git a/object.c b/object.c index 16eb944e98..fad1a5af4a 100644 --- a/object.c +++ b/object.c @@ -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; 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 ' test_expect_success 'traverse unexpected non-blob tag (seen)' '
In parse_object(), we try to handle blobs by streaming rather than loading them entirely into memory. The most common case here will be that we haven't seen the object yet and check oid_object_info(), which tells us we have a blob. But we trigger this code on one other case: when we have an in-memory object struct with type OBJ_BLOB (and without its "parsed" flag set, since otherwise we'd return early from the function). This indicates that some other part of the code suspected we have a blob (e.g., it was mentioned by a tree or tag) but we haven't yet looked at the on-disk copy. In this case before hitting the streaming path, we check if we have the object on-disk at all. This is mostly pointless extra work, as the streaming path would complain if it couldn't open the object (albeit with the message "hash mismatch", which is a little misleading). But it's also insufficient to catch all problems. The streaming code will only tell us "yes, the on-disk object matches the oid". But it doesn't actually confirm that what we found was indeed a blob, and neither does repo_has_object_file(). One way to improve this would be to teach stream_object_signature() to check the type (either by returning it to us to check, or taking an "expected" type). But there's an even simpler fix here: if we suspect the object is a blob, just call oid_object_info() to confirm that we have it on-disk, and that it really is a blob. This is slightly less efficient than teaching stream_object_signature() to do it (since it has to open the object already). But this case very rarely comes up. In practice, we usually don't have any clue what the type is, in which case we already call oid_object_info(). This "suspected" case happens only when some other code created an object struct but didn't actually parse the blob, which is actually tricky to trigger at all (see the discussion of the test below). 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) This is shorter, but also reflects what we really want say, which is "have we ruled out this being a blob; if not, check it on-disk". In either case, if oid_object_info() fails to tell us it's a blob, we'll skip the streaming code path and call repo_read_object_file(), just as before. And if we really do have a mismatch with the existing object struct, we'll eventually call lookup_commit(), etc, via parse_object_buffer(), which will complain that it doesn't match our existing obj->type. 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. Signed-off-by: Jeff King <peff@peff.net> --- 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. 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. object.c | 4 ++-- t/t6102-rev-list-unexpected-objects.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)