Re: [PATCH v2 7/7] bisect: allows any terms set by user

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2015, 22:19 UTC
Message-ID: <xmqq7frbi4uv.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqk2vbi7rf.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> I like this change very much; it removes the mysteriously misnamed
> start-bad-good variable (because you do not really _care_ that
> 'start' was what implicitly decided that good/bad pair is the term
> we use in this session; what you care is that the terms are already
> known or not).
>
> That is another reason why I think it would be a better organization
> for the patch series to do without the intermediate "we now add new/old
> as another hardcoded values on top of the traditional bad/good".
>
> That is, I would think a reasonable progression of the series would
> look more like these three steps:
>
>  - preliminary clean-up steps (e.g. "correct 'mistook'");
>
>  - use $name_new and $name_old throughout the code, giving them
>    'bad' and 'good' as hardcoded values; finally
>
>  - add 'bisect terms' support.

Just in case I confused readers with a message that apparently conflicts with what I said in the ancient thread:

  http://thread.gmane.org/gmane.comp.version-control.git/199758/focus=200025

I am admitting that I was wrong. Or perhaps I was right back then, but the world has changed ;-).

We have been hearing "bisect bad/good" is hard to use for the last 3 years since that thread discussed this topic, and that made me realize that addition of single new/old may not be good enough, and we should bite the bullet to support 'bisect terms' properly, making bad/good and new/old even less special (contrary to what I said back then in that thread "we only need these two pairs"), following the suggestion by Phil Hord in that thread.

And the suggested three-step approach above reflects that new world order in my mind. We admit that the machinery should have been built around a value-agnostic "old vs new" in the first place, but unfortunately it wasn't. So we belatedly update the system to use these two terms internally to express the logic by naming the variables name_old and name_new after these two value-agnostic concepts, and then support the traditional "good vs bad" as a mere default values for the names of old and new states.

One thing I forgot to say in the review of this patch:

Show 15 quoted lines

> +bisect_terms () {
> +	test $# -eq 2 ||
> +	die "You need to give me at least two arguments"
> +
> +	if ! test -s "$GIT_DIR/BISECT_START"
> +	then
> +		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
> +		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
> +		echo "1" > "$GIT_DIR/TERMS_DEFINED"
> +	else
> +		die "A bisection has already started, please use "\
> +		"'git bisect reset' to restart and change the terms"
> +	fi
> +}
> +

I think "git bisect terms" is a good way to help a user to recall what two names s/he decided to use for the current session. So dying 'already started' with suggestion for 'reset' is OK, but at the same time, helping the user to continue the current bisection by giving a message along the lines of "You are hunting for a commit that is at the boundary of the old state (you are calling it '$NAME_OLD') and the new state ('$NAME_NEW')" would be a good idea.

Thanks.