Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

Tiago Botelho <tiagonbotelho@gmail.com> writes:

We either say 0 ("the first parent is uninteresting") or 1 ("it is") under BISECT_FIRST_PARENT mode. OK.

An unrelated tangent, but I think I just spotted a bug in the existing code on the line immediately before this hunk, which reads

		if (commit->util)
			fprintf(stderr, "%3d", weight(p));

I think this was a bug introduced at bb408ac9 ("bisect.c: use commit-slab for commit weight instead of commit->util", 2018-05-19) where the internal implementation of weight() was changed not to touch commit->util but instead to use a separate commit-slab storage

Looking at the code before that conversion, it seems that we were using ->util to store a pointer to an integer, so we had the ability to differenciate non-negative weight (i.e. weight already computed for the commit), negative weight (i.e. not computed yet, but will be), and commits to which the concept of weight is not applicable. When we went to the commit-slab with the change, we lost the ability to represent the third case. I am offhand not sure what the best remedy would be. Perhaps stuff a so-far unused value like -3 to the weight() and use weight(p) == -3 instead of the old !commit->util or something like that?

(Duy CC'ed to help checking my sanity on this point).

In any case, this is an existing bug in a debug helper, and the focus of this patch is not about fixing that bug, so you can and should leave it as-is, until this patch successfully adds the "bisection following only the first parent" feature.

I am not sure if we want to do this. This is a debugging aid that allows us to peek into the internal state of the commit history graph being worked on; don't we rather want to make sure that tips of merged side branches are not participating (e.g. commit->util that records "weight" is used differently between the first and other parent commits)?

Because count_interesting_parents() returns either 0 or 1 under BISECT_FIRST_PARENT mode, we will never set -2 in this switch() statement. We either paint the bottom boundary with the correct weight, or -1 (i.e. "its weight is one plus its first parent's weight" marker). OK.

The loop before this part (not shown) is about counting weight for merges the hard way, which does not apply to us under the new BISECT_FIRST_PARENT mode. IOW, "initialize" and "count_distance" should show exactly the same output from show_list().

If we know the weight of 'p' already, then we are already done with it. In the normal code, that means we are done with all the merges, so the loop immediately below deals only with a single strand of pearls. However, under BISECT_FIRST_PARENT mode, that is different. Let's see.

Under first-parent mode, we uncondtionally break out of the loop after inspecting the first parent, even if it is a merge. The code after this loop (outside the context) says

			if (!q)
				continue;

which is to say "we are not ready to set the weight of p based on the weight of q, because the latter is unknown". So that is done by setting q to NULL, which makes sort of sense.

Post context of this hunk is "break;", meaning that if we find an interesting parent

What the above original tells us is that when we are doing a full bisection, we do not look at weight of uninteresting parent and continue. In that case we know p is not a merge so we will exit this for() loop with q set to NULL.

Shouldn't we keep that property even under BISECT_FIRST_PARENT mode? What I am wondering is the block added to the top of this for() loop.

Shouldn't it be more like this?

                                if ((bisect_flags & BISECT_FIRST_PARENT)) {
					if ((q->item->object.flags & UNINTERESTING) ||
					    (weight(q) < 0))
						q = NULL;
					break;
				}

I do not even recall if weight(q) is defined for an uninteresting q offhand; your patch requires weight(q) is defined and is negative for such a 'q' to be correct. Code would be more trivially correct with an explicit check for UNINTERESTING, I would think.

In any case, after finding a parent (or not finding, signalled by being NULL) whose weight is either the same or one less than our weight, we leave the above for() loop and then ...

... we set our weight based on its weight.  OK.

The remainder of the change looked OK (I didn't look at the the test).

Thanks.