Tiago Botelho <tiagonbotelho@xxxxxxxxx> writes:
> -static int count_interesting_parents(struct commit *commit)
> +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
> {
> struct commit_list *p;
> int count;
>
> for (count = 0, p = commit->parents; p; p = p->next) {
> - if (p->item->object.flags & UNINTERESTING)
> - continue;
> - count++;
> + if (!(p->item->object.flags & UNINTERESTING))
> + count++;
> + if (bisect_flags & BISECT_FIRST_PARENT)
> + break;
> }
> return count;
> }
Since this change makes the function never return anything more than 1,...
> @@ -310,7 +315,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> continue;
> if (weight(p) != -2)
> continue;
> - weight_set(p, count_distance(p));
> + weight_set(p, count_distance(p, bisect_flags));
> clear_distance(list);
... this code will not be reached when in the first-parent mode,
where we count the weight for merge commits the hard way.
Am I reading the code correctly? Not pointing out a problem; just
double checking how the code works.
> @@ -329,9 +334,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> if (0 <= weight(p))
> continue;
> for (q = p->item->parents; q; q = q->next) {
> - if (q->item->object.flags & UNINTERESTING)
> - continue;
> - if (0 <= weight(q))
> + if (!(q->item->object.flags & UNINTERESTING))
> + if (0 <= weight(q))
> + break;
> + if (bisect_flags & BISECT_FIRST_PARENT)
> break;
> }
> if (!q)
> continue;
This loop propagates the known weight of an ancestor through a
single strand of pearls.
When this loop is entered, any commit that has non-negative weight
has only one parent of interest, as we counted weight for all merges
the hard way and also weight for root and boundary commits, in the
previous loop. The original goes through p's parents and see if an
interesting parent has non-negative weight (i.e. weight is known for
that parent), at which point break out of the loop, with q that is
non-NULL. Immediately after the loop, q != NULL means we can now
compute weight for p based on q's weight.
I think this patch breaks the logic. When we are looking at a
commit 'p' whose weight is not known yet, we grab its first parent
in 'q'. Imagine that it is not an UNINTERESTING commit (i.e. a
commit whose weight matters when deciding the bisection) whose
weight is not yet known. With the updated code under the
first-parent mode, we break out of the loop, 'q' pointing at the
commit whose weight is not yet known. The computation done by the
code that immediately follows the above part is now broken, as it
will call weight(q) to get -1 and propagate it to p (or add 1 and
set 0 to p), no?
Perhaps something along this line may be closer to what we want:
if (0 <= weight(p))
continue; /* already known */
for (q = p->item->parents; q; q = q->next) {
if ((bisect_flags & BISECT_FIRST_PARENT)) {
if (weight(q) < 0)
q = NULL;
break;
}
if (q->item->object.flags & UNINTERESTING)
continue;
if (0 <= weight(q))
break;
}
if (!q)
continue; /* parent's weight not yet usable */
That is, under the first-parent mode, we would pay attention only to
the first parent of 'p' and break out of this loop without even
looking at q->next. If the weight of that parent is not known yet,
we mark that fact by clearing q to NULL and break out of the loop.
If the weight is known, we do not clear q, so we compute weight of p
based on weight(q).
I am not offhand certain what should happen when p's first parent is
uninteresting. The updated count_interesting_parents() would have
returned 0 for p in such a case, so at this point of the code where
p's weight is unknown, its first parent can never be UNINTERESTING,
I would think.
