Junio C Hamano [off-list ref] writes:
Jon Seymour [off-list ref] writes:
...
quoted
+ 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
quoted
- 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?
On Wednesday 03 August 2011 01:16:54 Junio C Hamano wrote:
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?
I agree that it would be nice if it worked in a bare repo. I did not look at
that yet.
Christian, do you think of anything else?
No, I agree that it looks almost ready.
The few improvements I would like are:
- squashing the last patch into the previous ones,
- using a bisect_mode() function when needed.
Also do you see flaws in our
reasoning that updating only BISECT_HEAD and doing nothing else is a good
way to do this?
No, I agree that --no-checkout is a good idea and that using only BISECT_HEAD
is good way to do this.
Thanks,
Christian.
On Wed, Aug 3, 2011 at 9:16 AM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
Jon Seymour [off-list ref] writes:
...
quoted
+ 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
quoted
- 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.
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.
On Wed, Aug 3, 2011 at 11:16 PM, Jon Seymour [off-list ref] wrote:
On Wed, Aug 3, 2011 at 9:16 AM, Junio C Hamano [off-list ref] wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
Jon Seymour [off-list ref] writes:
quoted
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.
I had a quick go at doing this, but haven't been able to test it
fully. At a minimum it will require that we relax the barriers in
git-bisect.sh and git.c that prevent git-bisect
and bisect--helper running without a working tree. Other paths in
these modules will need to be checked to see that they don't have an
implicit assumption that a working tree is available.
I won't have time to tackle this properly until the weekend.
jon.