Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 16, 2023, 19:01 UTC
Message-ID: <CAOLa=ZT4EuoB8GHMe+a2nfq8Pfg5x7xzrEa_qV39U1HqUyS+Eg@mail.gmail.com>
In-Reply-To: <xmqqbkcyo7ro.fsf@gitster.g>

On Mon, Oct 16, 2023 at 6:24 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Good changes relative to the previous round.
>
> A bad news is that with this series (especially [PATCH 3/3]) applied
> on top of Patrick's "always make sure the commit we found from the
> commit-graph at least exists" change, t5310 and t5320 seem to
> consistently fail for me.  They pass with the first two steps
> applied without [3/3] but that is to be expected as they are both
> no-ops.
>
> The change in 3/3 to list-objects.c::do_traverse() seems to be the
> culprit.  Reverting that single hunk makes t5310 and t5320 pass
> again.

It seems that the new chunk introduced now causes collision because of the bit field of the MISSING flag being the same as the pre-existing NEEDS_BITMAP flag. Earlier this wasn't a concern, but now, their paths have converged at this change.

So changing the bit field for the MISSING flag from `22` to an unused `28` fixes the issue. I should have run the whole test suite again. Apologies.

Show 5 quoted lines

>  What I find alarming is that two tests that are touched by
> this series, t5318 and t6022, do not seem to fail with the hunk
> reverted, which means the hunk has no meaningful test coverage for
> the purpose of this series.
>

Well, actually the newly introduced tests t6022 require this block, but this is specific to when commit graph is enabled. So what you did see was correct, but the test would also fail with `GIT_TEST_COMMIT_GRAPH=1` if the block was removed. That's exactly why in this series I increased the number of commits in the test block from 2->3.

Show 15 quoted lines

> >     +-        if (commit->object.flags & ADDED)
> >     ++        if (commit->object.flags & ADDED || commit->object.flags & MISSING)
>
> This and others are syntactically correct C, but some folks may find
> it more readable to see each of the bitwise operations enclosed in a
> pair of parentheses when combined by logical operations, i.e.,
>
>         if ((commit->object.flags & ADDED) || (commit->object.flags & MISSING))
>
> In this particular case, we can even say
>
>                 if (commit->object.flags & (ADDED | MISSING))
>
> meaning, "if we have either the ADDED or the MISSING bit set", which
> may make it even clearer.

I agree with this, I'll add it in the next block.

Thanks for the quick review, I'll wait a day/two and send v3 with the changes to fix tests and cleaner code.