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

From: Jon Seymour <jon.seymour@gmail.com>
Date: Aug 3, 2011, 13:16 UTC
Message-ID: <CAH3AnrrtfhU1r1DWO8ski5Jd=cLXcj3Wq-MowB8QKv+r1-BT_A@mail.gmail.com>
In-Reply-To: <7vk4avcsk9.fsf@alter.siamese.dyndns.org>

On Wed, Aug 3, 2011 at 9:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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).

That seems reasonable. In fact, none of these series properly cleaned up the reset state properly, so I'll fix that and a test for it.

Show 9 quoted lines

>
> 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?
>

I agree, that would be useful. Haven't tried it yet but I'll see what happens.I may issue changes for this as separate commit that can be squashed later, if required, once it has been reviewed.

BTW: I'll squash v14 8/8 into the other commits, per Christian's suggestion.

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

Seems reasonable.

jon.