Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2020, 16:41 UTC
Message-ID: <xmqqwo88f0do.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAN7CjDDwgR=y8gyYmDzmuTW3AKvb1N=EdCtH-8Tr7T=b6cG5gQ@mail.gmail.com>

"Miriam R." <mirucam@gmail.com> writes:

Show 23 quoted lines

>> It would be surprising if this code were correct.  It may be that it
>> happens to (appear to) work because parents of the commit hasn't
>> been painted and calling the helper only clears the mark from the
>> commit (so we won't see repeated "painting down to the root commit")
>> in which case this might be an extremely expensive looking variant
>> of
>>
>>         commit->object.flags &= ~ALL_REV_FLAGS;
>>
>> that only confuses the readers.
>>
>> Even then, I think by clearing bits like SEEN from commit, it breaks
>> the revision traversal machinery---for example, doesn't this mean
>> that the commit we just processed can be re-visited by
>> get_revision() without deduping in a history with forks and merges?
>>
>> Has this been shown to any of your mentors before sending it to the
>> list?
>
> Adding clear_commit_marks() was a suggestion of a previous review of this patch:
> https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/
>
> And of course, my mentor always reviews my patch series before sending.

OK, I just didn't know how you and your mentors work. Some leave their door open for mentees and help them when asked but otherwise act as an ordinary reviewer who somehow prioritises reviewing the work by their mentees. So your mentors may be a better source of information why this piece of code, which I still do not know how it could be correct, is supposed to work. Good.

After reading the above URL, I think you may have misread the suggestion you were given. Resetting using clear_commit_marks() may be necessary, but you do so when you finished walking so that you can do unrelated revision walk later. For that, you clear the flag bits after the while() loop that asks get_revision() to yield commits are done, using the initial set of commits that you used to start iteration.

That is how bisect.c::check_ancestors() work, that is

 - it initializes a rev_info 'revs' from an array of commit rev[]

 - it lets bisect_common() use the 'revs', which is allowed to
   smudge the flag bits of commit objects.

 - it uses clear_commit_marks_many() to clear the flags of the
   commits whose flag bits may have been smudged and their
   ancestors, recursively.  In order to use as the starting points,
   the original array of commit rev[] that started the revision
   traversal is used.