Re: [PATCH v14 5/8] bisect: introduce --no-checkout support into porcelain.

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2011, 23:16 UTC
Message-ID: <7vk4avcsk9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7voc07ct9z.fsf@alter.siamese.dyndns.org>

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

Show 28 quoted lines

> Jon Seymour <jon.seymour@gmail.com> writes:
> ...
>> +		if test "$BISECT_MODE" = "--no-checkout"; then
>> +		    git update-ref --no-deref HEAD "$start_head"
>> +		else
>> +		    git checkout "$start_head" --
>> +		fi
>
> Just a minor worry but I would not be surprised if somebody's "test"
> implementation barfs upon:
>
> 	test "--no-checkout" = "--no-checkout"
>
> mistaking the string with a dash at the beginning as an option unknown to
> it. That is why we often have "z$variable" in our comparison, like so:
>
> 	if test "z$BISECT_MODE" = "z--no-checkout"
>         then
>         	git update-ref --no-deref BISECT_HEAD "$start_head"
> 	else
>         	git checkout "$start_head" --
> 	fi
>
>> -	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
>> +	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES"
>> +	echo "$BISECT_MODE" > "$GIT_DIR/BISECT_MODE" &&
>
> &&?

Having said that, other than these minor nits, I think this round is almost ready. I didn't check how it behaves upon "bisect reset", though. It shouldn't touch the index, HEAD nor the working tree (it probably is just the matter of "update-ref -d BISECT_HEAD" and nothing else, but I haven't thought things through thoroughly).

Further polishing we may want to do while it is still in pu/next I can think of off the top of my head are:

 - In this mode, I can bisect the history even inside a bare repository,
   as the whole point of --no-checkout is that the mode does not require a
   working tree. I however suspect "git bisect" requires working tree. Is
   this something we want to fix?

 - Further, perhaps should we default to this mode inside a bare
   repository?

Christian, do you think of anything else? Also do you see flaws in our reasoning that updating only BISECT_HEAD and doing nothing else is a good way to do this?