# [PATCH 0/3] bisect: add support for bisecting bare repositories 12 messages from 2011-08-07 to 2011-08-07. Participants: Jon Seymour, Jonathan Nieder, Stefano Lattarini. Thread: https://gitlist.dev/t/28036 ## Jon Seymour, 2011-08-07 10:50 Subject: [PATCH 0/3] bisect: add support for bisecting bare repositories Message-ID: <1312714240-23647-1-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1312714240-23647-1-git-send-email-jon.seymour%40gmail.com ``` This extension to js/bisect-no-checkout (currently in pu) adds support for bisecting bare repositories. It does this by relaxing the requirement that git bisect is invoked in a repository with a working tree and by defaulting to --no-checkout in the case of a bare repository. Two tests are included to demonstrate this behaviour. Jon Seymour (3): bisect: relax requirement for a working tree. bisect: add tests for bisection on bare repositories bisect: document that --no-checkout is the default for bare repositories Documentation/git-bisect.txt | 2 ++ git-bisect.sh | 8 ++++++-- git.c | 2 +- t/t6030-bisect-porcelain.sh | 28 ++++++++++++++++++++++++++++ 4 files changed, 37 insertions(+), 3 deletions(-) -- 1.7.6.363.g9b380.dirty ``` ## Jon Seymour, 2011-08-07 10:50 Subject: [PATCH 1/3] bisect: relax requirement for a working tree. Message-ID: <1312714240-23647-2-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1312714240-23647-2-git-send-email-jon.seymour%40gmail.com In-Reply-To: <1312714240-23647-1-git-send-email-jon.seymour@gmail.com> ``` Now that bisection does not require checkout, it can work on bare repositories too. In this case, we default the checkout mode to --no-checkout, thereby forcing the creation of BISECT_HEAD on either a manual or automatic start. The require_work_tree check in git-bisect.sh and the NEED_WORK_TREE check in git.c are relaxed. Suggested-by: Junio C Hamano Signed-off-by: Jon Seymour --- git-bisect.sh | 8 ++++++-- git.c | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 22c4da5..436cc07 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -29,7 +29,6 @@ Please use "git help bisect" to get the full man page.' OPTIONS_SPEC= . git-sh-setup . git-sh-i18n -require_work_tree _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" @@ -79,7 +78,12 @@ bisect_start() { orig_args=$(git rev-parse --sq-quote "$@") bad_seen=0 eval='' - mode='' + if test "z$(git rev-parse --is-bare-repository)" != "zfalse" + then + mode='--no-checkout' + else + mode='' + fi while [ $# -gt 0 ]; do arg="$1" case "$arg" in diff --git a/git.c b/git.c index 8828c18..7fdcab2 100644 --- a/git.c +++ b/git.c @@ -320,7 +320,7 @@ static void handle_internal_command(int argc, const char **argv) { "annotate", cmd_annotate, RUN_SETUP }, { "apply", cmd_apply, RUN_SETUP_GENTLY }, { "archive", cmd_archive }, - { "bisect--helper", cmd_bisect__helper, RUN_SETUP | NEED_WORK_TREE }, + { "bisect--helper", cmd_bisect__helper, RUN_SETUP }, { "blame", cmd_blame, RUN_SETUP }, { "branch", cmd_branch, RUN_SETUP }, { "bundle", cmd_bundle, RUN_SETUP_GENTLY }, -- 1.7.6.363.g9b380.dirty ``` ## Jon Seymour, 2011-08-07 10:50 Subject: [PATCH 2/3] bisect: add tests for bisection on bare repositories Message-ID: <1312714240-23647-3-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1312714240-23647-3-git-send-email-jon.seymour%40gmail.com In-Reply-To: <1312714240-23647-1-git-send-email-jon.seymour@gmail.com> ``` We add a test to check that bisection works on bare repositories both when --no-checkout is specified explicitly and when it is defaulted. Signed-off-by: Jon Seymour --- t/t6030-bisect-porcelain.sh | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-) diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 4fb7d11..eb0412c 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -592,6 +592,34 @@ test_expect_success 'erroring out when using bad path parameters' ' grep "bad path parameters" error.txt ' +test_expect_success 'create bare repo' ' + git clone --bare . bare +' + +test_expect_success 'test bisection on bare repo - --no-checkout specified' ' + test_when_finished "cd .." && + cd bare && + git bisect start --no-checkout && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run sh -c \ + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ + >../my_bisect_log.txt && + grep "$HASH3 is the first bad commit" ../my_bisect_log.txt && + git bisect reset' + +test_expect_success 'test bisection on bare repo - --no-checkout defaulted' ' + test_when_finished "cd .." && + cd bare && + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run sh -c \ + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ + >../my_bisect_log.txt && + grep "$HASH3 is the first bad commit" ../my_bisect_log.txt && + git bisect reset' + # # This creates a broken branch which cannot be checked out because # the tree created has been deleted. -- 1.7.6.363.g9b380.dirty ``` ## Jon Seymour, 2011-08-07 10:50 Subject: [PATCH 3/3] bisect: document that --no-checkout is the default for bare repositories Message-ID: <1312714240-23647-4-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1312714240-23647-4-git-send-email-jon.seymour%40gmail.com In-Reply-To: <1312714240-23647-1-git-send-email-jon.seymour@gmail.com> ``` Signed-off-by: Jon Seymour --- Documentation/git-bisect.txt | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt index 41e6ca8..e4f46bc 100644 --- a/Documentation/git-bisect.txt +++ b/Documentation/git-bisect.txt @@ -273,6 +273,8 @@ it point to the commit that should be tested. + This option may be useful when the test you would perform in each step does not require a checked out tree. ++ +If the repository is bare, `--no-checkout` is assumed. EXAMPLES -------- -- 1.7.6.363.g9b380.dirty ``` ## Jonathan Nieder, 2011-08-07 12:41 Subject: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.) Message-ID: <20110807124150.GA20046@elie.gateway.2wire.net> URL: https://gitlist.dev/e/20110807124150.GA20046%40elie.gateway.2wire.net In-Reply-To: <1312714240-23647-2-git-send-email-jon.seymour@gmail.com> ``` Jon Seymour wrote: > Now that bisection does not require checkout, it can work > on bare repositories too. Nice. > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -29,7 +29,6 @@ Please use "git help bisect" to get the full man page.' > OPTIONS_SPEC= > . git-sh-setup > . git-sh-i18n > -require_work_tree > > _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' > _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" > @@ -79,7 +78,12 @@ bisect_start() { > orig_args=$(git rev-parse --sq-quote "$@") > bad_seen=0 > eval='' > - mode='' > + if test "z$(git rev-parse --is-bare-repository)" != "zfalse" > + then > + mode='--no-checkout' > + else > + mode='' > + fi Am I the only one who finds this test "z$foo" = "zbar" style to be impossibly ugly? It means every time someone considers using the "test" utility, they decide "is this expression likely to looks like an operator" and each time someone reads a use of the "test" utility, there is a lingering question of whether that choice was made correctly. By contrast, if one follows the following simple rules, everything works fine with the shells git supports: - _Do_ use the "z$foo" trick when using expr. - Do not use test's '(', '),' -a and -o operators; use && and || instead. The Autoconf manual says Posix also says that ‘test ! "string"’, ‘test -n "string"’ and ‘test -z "string"’ work with any string, but many shells (such as Solaris, AIX 3.2, UNICOS 10.0.0.6, Digital Unix 4, etc.) get confused if string looks like an operator: Notice that none of the mentioned shells is close enough to POSIX even to support $( / ). This is an area in which early POSIX work improved shells immensely (the "-e" primary was introduced around the same time). ``` ## Jon Seymour, 2011-08-07 12:51 Subject: Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.) Message-ID: URL: https://gitlist.dev/e/CAH3Anrr%3DfoOSWumaAhT%2BNKoipPd-r-sCup%3D5-HH5_XmGxnkJ4Q%40mail.gmail.com In-Reply-To: <20110807124150.GA20046@elie.gateway.2wire.net> ``` On Sun, Aug 7, 2011 at 10:41 PM, Jonathan Nieder wrote: > Jon Seymour wrote: +     fi > > Am I the only one who finds this > >        test "z$foo" = "zbar" > Yeah, I do too. The only reason I did it this way was because this is how it is done in git-sh-setup. jon. ``` ## Stefano Lattarini, 2011-08-07 13:04 Subject: Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.) Message-ID: <201108071505.00762.stefano.lattarini@gmail.com> URL: https://gitlist.dev/e/201108071505.00762.stefano.lattarini%40gmail.com In-Reply-To: <20110807124150.GA20046@elie.gateway.2wire.net> ``` On Sunday 07 August 2011, Jonathan Nieder wrote: > Am I the only one who finds this > > test "z$foo" = "zbar" > > style to be impossibly ugly? It means every time someone considers > using the "test" utility, they decide "is this expression likely to > looks like an operator" and each time someone reads a use of the > "test" utility, there is a lingering question of whether that choice > was made correctly. By contrast, if one follows the following simple > rules, everything works fine with the shells git supports: > > - _Do_ use the "z$foo" trick when using expr. > - Do not use test's '(', '),' -a and -o operators; use && and || > instead. > > The Autoconf manual says > > Posix also says that ‘test ! "string"’, ‘test -n "string"’ and > ‘test -z "string"’ work with any string, but many shells (such > as Solaris, AIX 3.2, UNICOS 10.0.0.6, Digital Unix 4, etc.) > get confused if string looks like an operator: > > Notice that none of the mentioned shells is close enough to POSIX even > to support $( / ). This is an area in which early POSIX work improved > shells immensely (the "-e" primary was introduced around the same > time). > While this is true, some problems with the `test' builtin persists also with in more "posixy" shells. For example, with Solaris /bin/ksh and /usr/xpg4/bin/sh (the latter expected to be the default POSIX-compliant shell on the system, since as you've noticed /bin/sh is defintely not compliant there), one sees: $ /usr/xpg4/bin/sh -c 'test -z ")"'; echo $? 0 $ /bin/ksh -c 'test -z ")"'; echo $? 0 This will be documented in the next version of the Autoconf manual BTW. Regards, Stefano ``` ## Jonathan Nieder, 2011-08-07 13:39 Subject: Re: [PATCH 2/3] bisect: add tests for bisection on bare repositories Message-ID: <20110807133939.GA10748@elie.gateway.2wire.net> URL: https://gitlist.dev/e/20110807133939.GA10748%40elie.gateway.2wire.net In-Reply-To: <1312714240-23647-3-git-send-email-jon.seymour@gmail.com> ``` Jon Seymour wrote: > We add a test to check that bisection works on bare repositories > both when --no-checkout is specified explicitly and when it > is defaulted. Yay! > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -592,6 +592,34 @@ test_expect_success 'erroring out when using bad path parameters' ' > grep "bad path parameters" error.txt > ' > > +test_expect_success 'create bare repo' ' > + git clone --bare . bare > +' I'd prefer to see separate clones for the two tests, so if one catastrophically fails it does not affect the other. > + > +test_expect_success 'test bisection on bare repo - --no-checkout specified' ' > + test_when_finished "cd .." && > + cd bare && > + git bisect start --no-checkout && > + git bisect good $HASH1 && > + git bisect bad $HASH4 && > + git bisect run sh -c \ > + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ > + >../my_bisect_log.txt && > + grep "$HASH3 is the first bad commit" ../my_bisect_log.txt && > + git bisect reset' I worry that "sh" might not actually be a POSIX shell on some systems. Maybe a subshell could also simplify this. Like so: git clone --bare . bare.nocheckout && ( cd bare && git bisect start --no-checkout && git bisect good $HASH1 && git bisect bad $HASH4 && git bisect run \ eval "test \$(... " \ >../log.nocheckout && git bisect reset ) && grep "$HASH3 is the first bad commit" log.nocheckout > +test_expect_success 'test bisection on bare repo - --no-checkout defaulted' ' Perhaps these could share code by using a function defined at the top of the file. With or without the changes described above, I like it. Thanks. ``` ## Jonathan Nieder, 2011-08-07 13:43 Subject: Re: [PATCH 3/3] bisect: document that --no-checkout is the default for bare repositories Message-ID: <20110807134347.GB10748@elie.gateway.2wire.net> URL: https://gitlist.dev/e/20110807134347.GB10748%40elie.gateway.2wire.net In-Reply-To: <1312714240-23647-4-git-send-email-jon.seymour@gmail.com> ``` Jon Seymour wrote: > Signed-off-by: Jon Seymour > --- > Documentation/git-bisect.txt | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt > index 41e6ca8..e4f46bc 100644 > --- a/Documentation/git-bisect.txt > +++ b/Documentation/git-bisect.txt > @@ -273,6 +273,8 @@ it point to the commit that should be tested. > + > This option may be useful when the test you would perform in each step > does not require a checked out tree. > ++ > +If the repository is bare, `--no-checkout` is assumed. Thanks. I think this should be squashed with patches 1 and 2, though I don't mind it having been split out for review. The whole series looks good, even given the tiny nitpicks in patch 2 I mentioned. :) ``` ## Jonathan Nieder, 2011-08-07 14:10 Subject: Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.) Message-ID: <20110807141035.GC10748@elie.gateway.2wire.net> URL: https://gitlist.dev/e/20110807141035.GC10748%40elie.gateway.2wire.net In-Reply-To: <201108071505.00762.stefano.lattarini@gmail.com> ``` Stefano Lattarini wrote: > $ /usr/xpg4/bin/sh -c 'test -z ")"'; echo $? > 0 Yikes, thanks for pointing it out. I wonder if we should introduce a sane_test function, like this. empty_string () { case $* in ?*) return 1 ;; *) return 0 ;; esac } # This is like "test", but: # - it only implements a minimal subset of the functionality # - it works around shells that cannot cope with parentheses as # parameters to some operators, like ksh before 2008: # # $ /usr/xpg4/bin/sh -c 'test -z ")"'; echo $? # 0 sane_test () { # implied -n test "$#" = 0 && set -- -n "" test "$#" = 1 && set -- -n "$1" test "$#" = 2 && test "z$1" = z! && set -- -z "$2" test "$#" -le 4 || case $# in 2) case $1 in -z) empty_string "$2" ;; -n) ! empty_string "$2" ;; -*) test "$@" ;; *) die "sane_test: unrecognized unary operator $1" ;; esac ;; 3) case $2 in = | !=) test "z$1" "$2" "z$3" ;; -eq | -lt | -gt | -le | -ge) test "$@" ;; *) die "sane_test: unrecognized binary operator $2" ;; esac ;; 4) test "z$1" = z! || die "sane_test: unrecognized 4-argument test (not a negation)" shift ! sane_test "$@" ;; *) die 'sane_test: too many arguments' ;; esac } ``` ## Jon Seymour, 2011-08-07 14:39 Subject: Re: [PATCH 2/3] bisect: add tests for bisection on bare repositories Message-ID: URL: https://gitlist.dev/e/CAH3AnrohGgvu%2BB-p2-4kNcx68UV4tAbirz7gf6Qi9%3DWpGqE%2B3Q%40mail.gmail.com In-Reply-To: <20110807133939.GA10748@elie.gateway.2wire.net> ``` On Sun, Aug 7, 2011 at 11:39 PM, Jonathan Nieder wrote: > Jon Seymour wrote: > >> We add a test to check that bisection works on bare repositories >> both when --no-checkout is specified explicitly and when it >> is defaulted. > > Yay! > >> --- a/t/t6030-bisect-porcelain.sh >> +++ b/t/t6030-bisect-porcelain.sh >> @@ -592,6 +592,34 @@ test_expect_success 'erroring out when using bad path parameters' ' >>       grep "bad path parameters" error.txt >>  ' >> >> +test_expect_success 'create bare repo' ' >> +     git clone --bare . bare >> +' > > I'd prefer to see separate clones for the two tests, so if one > catastrophically fails it does not affect the other. > >> + >> +test_expect_success 'test bisection on bare repo - --no-checkout specified' ' >> +     test_when_finished "cd .." && >> +     cd bare && >> +     git bisect start --no-checkout && >> +     git bisect good $HASH1 && >> +     git bisect bad $HASH4 && >> +     git bisect run sh -c \ >> +             "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ >> +             >../my_bisect_log.txt && >> +     grep "$HASH3 is the first bad commit" ../my_bisect_log.txt && >> +     git bisect reset' > > I worry that "sh" might not actually be a POSIX shell on some systems. > Maybe a subshell could also simplify this.  Like so: Thanks. Am assuming Junio is ok with use of eval, given these reasons. The subshell is a good idea. So... +test_expect_success 'test bisection on bare repo - --no-checkout specified' ' + git clone --bare . bare.nocheckout && + ( + cd bare.nocheckout && + git bisect start --no-checkout && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run eval \ + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ + >../nocheckout.log && + git bisect reset + ) && + grep "$HASH3 is the first bad commit" nocheckout.log +' + + +test_expect_success 'test bisection on bare repo - --no-checkout defaulted' ' + git clone --bare . bare.defaulted && + ( + cd bare.defaulted && + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run eval \ + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \ + >../defaulted.log && + git bisect reset + ) && + grep "$HASH3 is the first bad commit" defaulted.log +' + jon. ``` ## Jon Seymour, 2011-08-07 14:40 Subject: Re: [PATCH 1/3] bisect: relax requirement for a working tree. Message-ID: URL: https://gitlist.dev/e/CAH3AnrowF7z9wdKqQthxkcAPQCCJkgJSkHDAr6CCjo_aoWzfqA%40mail.gmail.com In-Reply-To: <1312714240-23647-2-git-send-email-jon.seymour@gmail.com> ``` On Sun, Aug 7, 2011 at 8:50 PM, Jon Seymour wrote: > +       if test "z$(git rev-parse --is-bare-repository)" != "zfalse" Next iteration will remove double quotes from around zfalse. jon. ```