[PATCH 0/3] bisect: add support for bisecting bare repositories

[PATCH 0/3] bisect: add support for bisecting bare repositories

[Jon Seymour]

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

[PATCH 1/3] bisect: relax requirement for a working tree.

[Jon Seymour]

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 <gitster@pobox.com>
Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 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

test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.)

[Jonathan Nieder]

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).

Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.)

[Jon Seymour]

On Sun, Aug 7, 2011 at 10:41 PM, Jonathan Nieder <jrnieder@gmail.com> 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.

Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.)

[Stefano Lattarini]

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

Re: test z$foo = zbar (and Re: [PATCH 1/3] bisect: relax requirement for a working tree.)

[Jonathan Nieder]

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
 }

Re: [PATCH 1/3] bisect: relax requirement for a working tree.

[Jon Seymour]

On Sun, Aug 7, 2011 at 8:50 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> +       if test "z$(git rev-parse --is-bare-repository)" != "zfalse"

Next iteration will remove double quotes from around zfalse.

jon.

[PATCH 2/3] bisect: add tests for bisection on bare repositories

[Jon Seymour]

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 <jon.seymour@gmail.com>
---
 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

Re: [PATCH 2/3] bisect: add tests for bisection on bare repositories

[Jonathan Nieder]

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.

Re: [PATCH 2/3] bisect: add tests for bisection on bare repositories

[Jon Seymour]

On Sun, Aug 7, 2011 at 11:39 PM, Jonathan Nieder <jrnieder@gmail.com> 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.

[PATCH 3/3] bisect: document that --no-checkout is the default for bare repositories

[Jon Seymour]

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 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

Re: [PATCH 3/3] bisect: document that --no-checkout is the default for bare repositories

[Jonathan Nieder]

Jon Seymour wrote:

> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  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. :)