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

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

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.

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.