Re: [PATCH v17 1/7] bisect: move argument parsing before state modification.

From: Christian Couder <chriscool@tuxfamily.org>
Date: Sep 7, 2011, 06:16 UTC
Message-ID: <201109070816.16655.chriscool@tuxfamily.org>
In-Reply-To: <1312459263-16911-2-git-send-email-jon.seymour@gmail.com>

Hi,

On Thursday 04 August 2011 14:00:57 Jon Seymour wrote:

Show 6 quoted lines

> Currently 'git bisect start' modifies some state prior to checking
> that its arguments are valid.
> 
> This change moves argument validation before state modification
> with the effect that state modification does not occur
> unless argument validations succeeds.

This thread:

http://thread.gmane.org/gmane.comp.version-control.git/180733/

made me wonder if we introduced a bug with this patch.

If we start bisecting like this:

$ git bisect start HEAD HEAD~20

and then we decide that it was not optimum and we want to start again like this:

$ git bisect start HEAD HEAD~6

then issuing the latter command might not work as it did before this patch.
 
Before this patch the latter command would do a "git checkout $start_head" 
before the repeated rev=$(git rev-parse -q --verify "$arg^{commit}") to 
convert arguments into sha1. And after this patch the order is reversed.

This means that before this patch "HEAD" in the arguments to "git bisect start" would refer to $start_head because the "git checkout $start_head" changes HEAD. After this patch "HEAD" in the arguments to "git bisect start" would refer to the current HEAD.

For example before this patch, if I issue "git bisect start HEAD HEAD~8" twice I get:

$ git bisect start HEAD HEAD~8
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[67c116bb26b4ee31889e5ee15d6a9d3b7e972b7b] Merge branch 'jk/pager-with-
external-command'
$ git bisect start HEAD HEAD~8
Previous HEAD position was 67c116b... Merge branch 'jk/pager-with-external-
command'
Switched to branch 'master'
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[67c116bb26b4ee31889e5ee15d6a9d3b7e972b7b] Merge branch 'jk/pager-with-
external-command'

so the same commit to test is checked out.

After this patch I get:

$ git bisect start HEAD HEAD~8
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[67c116bb26b4ee31889e5ee15d6a9d3b7e972b7b] Merge branch 'jk/pager-with-
external-command'
$ git bisect start HEAD HEAD~8
Previous HEAD position was 67c116b... Merge branch 'jk/pager-with-external-
command'
Switched to branch 'master'
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[e5cfcb04e0acc5f3b51e6d69487028315b33e4c9] Merge branch 'mh/attr'

so a different commit is checked out after the second "git bisect start HEAD HEAD~8".

Best regards, Christian.