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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Mar 6, 2020 at 8:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Miriam R." <mirucam@gmail.com> writes:
>
> > To my understanding, it looks like calling reset_revision_walk() after
> > the while() loop should be enough. Am I right or am I missing
> > something?
>
> I suspect that reset_revision_walk() may be too-big a hammer, as it
> clears everything, regardless of the reason why the flag bits were
> set.  On the other hand, the clearly strategy that uses
> clear_commit_marks() is to clear only the flag bits that were set
> during the previous revision walk from only the commits that were
> walked during the previous revision walk.
>
> I offhand do not know what flag bits on what objects that were not
> involved in the previous revision walk are still necessary at the
> point of the call made by the caller (that's a question for your
> mentors who volunteered their expertise on the program in question),
> so if there isn't any, reset_revision_walk() may be an easy way out.
> I just do not know if it clears too much to break the code that
> comes after the function returns.

process_skipped_commits(), the function that does this revision walk,
is called by bisect_skipped_commits() to print the possible first bad
commits when there are only skipped commits left to test and we
therefore cannot bisect more. This can be seen in bisect_next() which
does basically the following:

bisect_next()
{
       ...

       /* Perform all bisection computation, display and checkout */
       res = bisect_next_all(the_repository, prefix, no_checkout);

       if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
               ...
       } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
               res = bisect_skipped_commits(terms);
               return res ? res : BISECT_ONLY_SKIPPED_LEFT;
       }
       return res;
}

BISECT_ONLY_SKIPPED_LEFT is an error code (-2) so bisect_next() will
always return an error in this case.

This means that the revision walk in process_skipped_commits() is very
likely to be the last revision walk performed by the command. So my
opinion is that not clearing anything at the end of that revision walk
is fine.

If we are worried about what could happen one day, when people might
be interested in actually doing another revision walk after this one,
then as we don't know what they will want to do and might be
interested in, cleaning everything with reset_revision_walk() might be
the safest thing to do and is probably cheap enough that it's ok to
use it right now.

Thanks for your review,
Christian.