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

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> ... some extra checks here, especially because we don't want to
> allow missing commit's tree and parent to be parsed.

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

I didn't even try to understand what is going on, so there may be a
very good reason that the change must be there for do_traverse() to
work correctly.  I dunno.

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

Thanks.