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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2015, 21:16 UTC
Message-ID: <xmqqk2vbi7rf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433962918-6536-3-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr>

Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:

> -USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
> +USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'

I think this patch makes the whole series go in the right direction.

I wonder if you can skip the "we only support new/old if you are not doing bog-standard bad/good" step and start from this "bisect terms" one, though.

Then you do not even have to treat new/old any specially, and do not even have to list them in the above list.

Show 11 quoted lines

> @@ -79,9 +81,16 @@ bisect_start() {
>  	orig_args=$(git rev-parse --sq-quote "$@")
>  	bad_seen=0
>  	eval=''
> -	# start_bad_good is used to detect if we did a 
> -	# 'git bisect start bad_rev good_rev'
> -	start_bad_good=0
> +	# terms_defined is used to detect if we did a
> +	# 'git bisect start bad_rev good_rev' or if the user
> +	# defined his own terms with git bisect terms
> +	terms_defined=0

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.

Thanks.