patch, 3 partsbisect: add support for bisecting bare repositories

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

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

Jon Seymour wrote:

Nice.

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

On Sun, Aug 7, 2011 at 10:41 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

+     fi

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.

On Sunday 07 August 2011, Jonathan Nieder wrote:

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

Stefano Lattarini wrote:

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
 }

On Sun, Aug 7, 2011 at 8:50 PM, Jon Seymour <jon.seymour@gmail.com> wrote:

Next iteration will remove double quotes from around zfalse.

jon.

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

Jon Seymour wrote:

Yay!

I'd prefer to see separate clones for the two tests, so if one catastrophically fails it does not affect the other.

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

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.

On Sun, Aug 7, 2011 at 11:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 Documentation/git-bisect.txt |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Jon Seymour wrote:

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