[PATCH 0/5] bisect: Add support for a --no-checkout option.

[PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

For some bisection tasks, checking out the commit at each stage of the bisection process is unecessary or undesirable.

This series adds support for a --no-checkout[=<ref>] option to git-bisect.

If specified, this option causes git-bisect to update the specified reference at each stage of the bisection process instead of checking out the commit at that point. If <ref> is not specified, HEAD is assumed.

One application of this option is to find, within a partially damaged repository, the earliest commit such that the graph of commits, trees and blobs reachable from the parents of that commit is completely reachable. 

For example:

    git bisect start HEAD <known-good-commit> [ <damaged-or-missing-commit> ... ]
    git bisect run eval '
    	git rev-list --objects HEAD > /dev/null && 
	git rev-list --objects HEAD | git pack-objects --stdout >/dev/null || 
	false'

Assuming this git bisect run completes successfully, bisect/bad will refer to the first commit whose parents have completely reachable graphs.

This series is a reworking of an earlier series "bisect: allow git bisect to be used with repos containing damaged trees." that incorporates Junio's suggestion to add a --no-checkout option instead of an --ignore-checkout-failure option.

Jon Seymour (5):
  bisect: add tests to document expected behaviour in presence of
    broken trees.
  bisect: introduce support for --no-checkout=<ref> option.
  bisect: introduce --no-checkout[=<ref>] support into porcelain.
  bisect: add tests for the --no-checkout option.
  bisect: add documentation for --no-checkout[=<ref>] option.

 Documentation/git-bisect.txt |   26 ++++++++-
 bisect.c                     |   26 +++++++-
 bisect.h                     |    2 +-
 builtin/bisect--helper.c     |    7 ++-
 git-bisect.sh                |   45 ++++++++++++--
 t/t6030-bisect-porcelain.sh  |  132 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 223 insertions(+), 15 deletions(-)

-- 
1.7.6.387.g991c2

[PATCH 1/5] bisect: add tests to document expected behaviour in presence of broken trees.

[Jon Seymour]

If the repo is broken, we expect bisect to fail.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t6030-bisect-porcelain.sh |   48 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index b5063b6..c28da2d 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -573,5 +573,53 @@ test_expect_success 'erroring out when using bad path parameters' '
 '
 
 #
+# This creates a broken branch which cannot be checked out because
+# the tree created has been deleted.
 #
+# H1-H2-H3-H4-H5-H6-H7  <--other
+#            \
+#             S5-S6'-S7'-S8'-S9  <--broken
+#
+# Commits marked with ' have a missing tree.
+#
+test_expect_success 'broken branch creation' '
+	git bisect reset &&
+	git checkout -b broken $HASH4 &&
+	git tag BROKEN_HASH4 $HASH4 &&
+	add_line_into_file "5(broken): first line on a broken branch" hello2 &&
+	git tag BROKEN_HASH5 &&
+	mkdir missing &&
+	:> missing/MISSING &&
+	git add missing/MISSING &&
+	git commit -m "6(broken): Added file that will be deleted"
+	git tag BROKEN_HASH6 &&
+	add_line_into_file "7(broken): second line on a broken branch" hello2 &&
+	git tag BROKEN_HASH7 &&
+	add_line_into_file "8(broken): third line on a broken branch" hello2 &&
+	git tag BROKEN_HASH8 &&
+	git rm missing/MISSING &&
+	git commit -m "9(broken): Remove missing file"
+	git tag BROKEN_HASH9 &&
+	rm .git/objects/39/f7e61a724187ab767d2e08442d9b6b9dab587d
+'
+
+echo "" > expected.ok
+cat > expected.missing-tree.default <<EOF
+fatal: unable to read tree 39f7e61a724187ab767d2e08442d9b6b9dab587d
+EOF
+
+test_expect_success 'bisect fails if tree is broken on start commit' '
+	git bisect reset &&
+	test_must_fail git bisect start BROKEN_HASH7 BROKEN_HASH4 2>error.txt &&
+	test_cmp expected.missing-tree.default error.txt
+'
+
+test_expect_success 'bisect fails if tree is broken on trial commit' '
+	git bisect reset &&
+	test_must_fail git bisect start BROKEN_HASH9 BROKEN_HASH4 2>error.txt &&
+	git reset --hard broken &&
+	git checkout broken &&
+	test_cmp expected.missing-tree.default error.txt
+'
+
 test_done
-- 
1.7.6.387.g991c2

[PATCH 2/5] bisect: introduce support for --no-checkout=<ref> option.

[Jon Seymour]

If --no-checkout=<ref> is specified, then the bisection process uses

	git update-ref --no-deref <ref> <trial>

at each trial instead of:

	git checkout <trial>

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 bisect.c                 |   26 ++++++++++++++++++++++----
 bisect.h                 |    2 +-
 builtin/bisect--helper.c |    7 +++++--
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/bisect.c b/bisect.c
index dd7e8ed..e6c99a0 100644
--- a/bisect.c
+++ b/bisect.c
@@ -24,6 +24,8 @@ struct argv_array {
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
+static const char *argv_update_ref[] = {"update-ref", "--no-deref", NULL, NULL, NULL};
+static const char *cursor_ref = 0;
 
 /* bits #0-15 in revision.h */
 
@@ -714,9 +716,19 @@ static int bisect_checkout(char *bisect_rev_hex)
 	mark_expected_rev(bisect_rev_hex);
 
 	argv_checkout[2] = bisect_rev_hex;
-	res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
-	if (res)
-		exit(res);
+	if (!cursor_ref) {
+	  res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
+	  if (res) {
+	    exit(res);
+	  }
+	} else {
+	    argv_update_ref[2] = cursor_ref;
+	    argv_update_ref[3] = bisect_rev_hex;
+	    res = run_command_v_opt(argv_update_ref, RUN_GIT_CMD);
+	    if (res) {
+	      die("update-ref --no-deref %s failed on %s", cursor_ref, bisect_rev_hex);
+	    }
+	}
 
 	argv_show_branch[1] = bisect_rev_hex;
 	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
@@ -908,8 +920,12 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
  * We use the convention that exiting with an exit code 10 means that
  * the bisection process finished successfully.
  * In this case the calling shell script should exit 0.
+ *
+ * If cursor_ref_param is not null, the bisection process does not
+ * checkout the trial commit but instead simply updates the specified
+ * cursor reference.
  */
-int bisect_next_all(const char *prefix)
+int bisect_next_all(const char *prefix, const char *cursor_ref_param)
 {
 	struct rev_info revs;
 	struct commit_list *tried;
@@ -917,6 +933,8 @@ int bisect_next_all(const char *prefix)
 	const unsigned char *bisect_rev;
 	char bisect_rev_hex[41];
 
+	cursor_ref = cursor_ref_param;
+
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
diff --git a/bisect.h b/bisect.h
index 0862ce5..4fd4565 100644
--- a/bisect.h
+++ b/bisect.h
@@ -27,7 +27,7 @@ struct rev_list_info {
 	const char *header_prefix;
 };
 
-extern int bisect_next_all(const char *prefix);
+extern int bisect_next_all(const char *prefix, const char *no_checkout);
 
 extern int estimate_bisect_steps(int all);
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5b22639..b3d9263 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,16 +4,19 @@
 #include "bisect.h"
 
 static const char * const git_bisect_helper_usage[] = {
-	"git bisect--helper --next-all",
+	"git bisect--helper --next-all [ --no-checkout=<ref> ]",
 	NULL
 };
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int next_all = 0;
+	char * no_checkout = NULL;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "next-all", &next_all,
 			    "perform 'git bisect next'"),
+		OPT_STRING(0, "no-checkout", &no_checkout, "ref",
+			    "don't checkout but update <ref> instead."),
 		OPT_END()
 	};
 
@@ -24,5 +27,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	/* next-all */
-	return bisect_next_all(prefix);
+	return bisect_next_all(prefix, no_checkout);
 }
-- 
1.7.6.387.g991c2

[PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Jon Seymour]

git-bisect can now perform bisection of a history without performing
a checkout at each stage of the bisection process.

Instead, the reference specified by <ref> is updated. If <ref> is not
specified, HEAD is used instead.

One use-case for this function is allow git bisect to be used with
damaged repositories where git checkout would fail because the tree
referenced by the commit is damaged.

It can also be used in other cases where actual checkout of the tree
is not required to progress the bisection.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-bisect.sh |   45 ++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index b2186a8..6c4e853 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -3,7 +3,7 @@
 USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
         print this long help message.
-git bisect start [<bad> [<good>...]] [--] [<pathspec>...]
+git bisect start [--no=checkout[=<ref>]] [<bad> [<good>...]] [--] [<pathspec>...]
         reset bisect state and start bisection.
 git bisect bad [<rev>]
         mark <rev> a known-bad revision.
@@ -34,6 +34,8 @@ 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"
 
+BISECT_NOCHECKOUT=$(test -f "${GIT_DIR}/BISECT_NOCHECKOUT" && cat "${GIT_DIR}/BISECT_NOCHECKOUT")
+
 bisect_autostart() {
 	test -s "$GIT_DIR/BISECT_START" || {
 		(
@@ -59,6 +61,18 @@ bisect_autostart() {
 }
 
 bisect_start() {
+	BISECT_NOCHECKOUT=
+	for arg in "$@"; do
+		case "$arg" in
+		--no-checkout)
+			BISECT_NOCHECKOUT=--no-checkout=HEAD ;;
+		--no-checkout=*)
+			BISECT_NOCHECKOUT=$arg ;;
+		--)
+			break; ;;
+		esac
+	done
+
 	#
 	# Verify HEAD.
 	#
@@ -74,7 +88,11 @@ bisect_start() {
 	then
 		# Reset to the rev from where we started.
 		start_head=$(cat "$GIT_DIR/BISECT_START")
-		git checkout "$start_head" -- || exit
+		if test -z "${BISECT_NOCHECKOUT}"; then
+		    git checkout "$start_head" --
+		else
+		    git update-ref --no-deref "${BISECT_NOCHECKOUT#--no-checkout=}" "$start_head"
+		fi
 	else
 		# Get rev from where we start.
 		case "$head" in
@@ -114,6 +132,13 @@ bisect_start() {
 		shift
 		break
 		;;
+	    --no-checkout*)
+		echo "${BISECT_NOCHECKOUT}" > "$GIT_DIR/BISECT_NOCHECKOUT"
+		shift
+		;;
+	    --*)
+		die "unrecognised option: $arg"
+		;;
 	    *)
 		rev=$(git rev-parse -q --verify "$arg^{commit}") || {
 		    test $has_double_dash -eq 1 &&
@@ -291,7 +316,7 @@ bisect_next() {
 	bisect_next_check good
 
 	# Perform all bisection computation, display and checkout
-	git bisect--helper --next-all
+	git bisect--helper --next-all "${BISECT_NOCHECKOUT}"
 	res=$?
 
         # Check if we should exit because bisection is finished
@@ -340,11 +365,16 @@ bisect_reset() {
 	*)
 	    usage ;;
 	esac
-	if git checkout "$branch" -- ; then
-		bisect_clean_state
-	else
-		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
+	if test -z "${BISECT_NOCHECKOUT}"; then
+		if git checkout "$branch" --; then
+			bisect_clean_state
+		else
+			die "$(eval_gettext "Could not check out original HEAD '\$branch'.
 Try 'git bisect reset <commit>'.")"
+		fi
+	else
+	    ref=${BISECT_NOCHECKOUT#--no-checkout=}
+	    git symbolic-ref "$ref" $(git rev-parse --symbolic-full-name "${branch}")
 	fi
 }
 
@@ -360,6 +390,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_LOG" &&
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
+	rm -f "$GIT_DIR/BISECT_NOCHECKOUT" &&
 	# Cleanup head-name if it got left by an old version of git-bisect
 	rm -f "$GIT_DIR/head-name" &&
 
-- 
1.7.6.387.g991c2

[PATCH 4/5] bisect: add tests for the --no-checkout option.

[Jon Seymour]

We expect to git-bisect --no-checkout to tolerate various kinds of repository damage.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t6030-bisect-porcelain.sh |   84 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index c28da2d..a9e86d2 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -608,6 +608,14 @@ cat > expected.missing-tree.default <<EOF
 fatal: unable to read tree 39f7e61a724187ab767d2e08442d9b6b9dab587d
 EOF
 
+check_same()
+{
+    echo "Checking $1 is the same as $2" &&
+    git rev-parse "$1" > expected.same &&
+    git rev-parse "$2" > expected.actual &&
+    test_cmp expected.same expected.actual
+}
+
 test_expect_success 'bisect fails if tree is broken on start commit' '
 	git bisect reset &&
 	test_must_fail git bisect start BROKEN_HASH7 BROKEN_HASH4 2>error.txt &&
@@ -622,4 +630,80 @@ test_expect_success 'bisect fails if tree is broken on trial commit' '
 	test_cmp expected.missing-tree.default error.txt
 '
 
+test_expect_success 'bisect: --no-checkout - start commit bad' '
+	git bisect reset &&
+	git bisect start BROKEN_HASH7 BROKEN_HASH4 --no-checkout &&
+	check_same BROKEN_HASH6 HEAD &&
+	git bisect reset
+'
+
+test_expect_success 'bisect: --no-checkout - trial commit bad' '
+	git bisect reset &&
+	git bisect start broken BROKEN_HASH4 --no-checkout &&
+	check_same BROKEN_HASH6 HEAD &&
+	git bisect reset
+'
+
+test_expect_success 'bisect: --no-checkout - target before breakage' '
+	git bisect reset &&
+	git bisect start broken BROKEN_HASH4 --no-checkout &&
+	check_same BROKEN_HASH6 HEAD &&
+	git bisect bad HEAD &&
+	check_same BROKEN_HASH5 HEAD &&
+	git bisect bad HEAD &&
+	check_same BROKEN_HASH5 bisect/bad &&
+	git bisect reset
+'
+
+test_expect_success 'bisect: --no-checkout - target in breakage' '
+	git bisect reset &&
+	git bisect start broken BROKEN_HASH4 --no-checkout &&
+	check_same BROKEN_HASH6 HEAD &&
+	git bisect bad HEAD &&
+	check_same BROKEN_HASH5 HEAD &&
+	git bisect good HEAD &&
+	check_same BROKEN_HASH6 bisect/bad &&
+	git bisect reset
+'
+
+test_expect_success 'bisect: --no-checkout - target after breakage' '
+	git bisect reset &&
+	git bisect start broken BROKEN_HASH4 --no-checkout &&
+	check_same BROKEN_HASH6 HEAD &&
+	git bisect good HEAD &&
+	check_same BROKEN_HASH8 HEAD &&
+	git bisect good HEAD &&
+	check_same BROKEN_HASH9 bisect/bad &&
+	git bisect reset
+'
+
+test_expect_success 'bisect: --no-checkout=CURSOR - check HEAD is unmodified' '
+	git bisect reset &&
+	git checkout broken &&
+	BROKEN=$(git rev-parse broken) &&
+	git bisect start broken BROKEN_HASH4 --no-checkout=CURSOR &&
+	check_same CURSOR BROKEN_HASH6 &&
+	test "refs/heads/broken" = "$(git rev-parse --symbolic-full-name HEAD)" &&
+	git bisect bad CURSOR &&
+	test "refs/heads/broken" = "$(git rev-parse --symbolic-full-name HEAD)" &&
+	check_same CURSOR BROKEN_HASH5 &&
+	test "refs/heads/broken" = "$(git rev-parse --symbolic-full-name HEAD)" &&
+	git bisect good CURSOR &&
+	check_same BROKEN_HASH6 bisect/bad &&
+	test "refs/heads/broken" = "$(git rev-parse --symbolic-full-name HEAD)" &&
+	git bisect reset &&
+	test "refs/heads/broken" = "$(git rev-parse --symbolic-full-name HEAD)" &&
+	check_same HEAD broken &&
+	check_same broken $BROKEN
+'
+
+test_expect_success 'bisect: demonstrate identification of damage boundary' '
+	git bisect reset &&
+	git checkout broken &&
+	git bisect start broken master --no-checkout &&
+	git bisect run eval "git rev-list --objects HEAD && git rev-list --objects HEAD | git pack-objects --stdout >/dev/null || false" &&
+	check_same BROKEN_HASH6 bisect/bad &&
+	git bisect reset
+'
+
 test_done
-- 
1.7.6.387.g991c2

[PATCH 5/5] bisect: add documentation for --no-checkout[=<ref>] option.

[Jon Seymour]

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

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index ab60a18..0ad3b01 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect help
- git bisect start [<bad> [<good>...]] [--] [<paths>...]
+ git bisect start [--no-checkout[=<ref>]] [<bad> [<good>...]] [--] [<paths>...]
  git bisect bad [<rev>]
  git bisect good [<rev>...]
  git bisect skip [(<rev>|<range>)...]
@@ -263,6 +263,17 @@ rewind the tree to the pristine state.  Finally the script should exit
 with the status of the real test to let the "git bisect run" command loop
 determine the eventual outcome of the bisect session.
 
+OPTIONS
+-------
+--no-checkout[=<ref>]::
++
+If this option is specified, git bisect does not update the working tree
+or index but instead simply updates the specified reference. <ref> defaults
+to HEAD if not specified.
++
+This option is useful in circumstances in which checkout is either not
+possible (because of a damaged respository) or otherwise not required.
+
 EXAMPLES
 --------
 
@@ -343,6 +354,19 @@ $ git bisect run sh -c "make || exit 125; ~/check_test_case.sh"
 This shows that you can do without a run script if you write the test
 on a single line.
 
+* Locate the earliest damaged commit in a repository
++
+------------
+$ git bisect start HEAD <known-good-commit> [ <missing-or-damaged-commit> ... ] --no-checkout
+$ git bisect run eval "
+git rev-list --objects HEAD >/dev/null && 
+git rev-list --objects HEAD | git pack-objects --stdout >/dev/null || false"
+------------
++
+This shows how to use `git bisect --no-checkout` with a repository containing damaged commits 
+to find the earliest commit reachable from the HEAD commit such that the graph rooted at the 
+parents of that commit is undamaged.
+
 SEE ALSO
 --------
 link:git-bisect-lk2009.html[Fighting regressions with git bisect],
-- 
1.7.6.387.g991c2

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Sat, Jul 30, 2011 at 6:28 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> For some bisection tasks, checking out the commit at each stage of the bisection process is unecessary or undesirable.
>
> This series adds support for a --no-checkout[=<ref>] option to git-bisect.
>
> If specified, this option causes git-bisect to update the specified reference at each stage of the bisection process instead of checking out the commit at that point. If <ref> is not specified, HEAD is assumed.
>
> One application of this option is to find, within a partially damaged repository, the earliest commit such that the graph of commits, trees and blobs reachable from the parents of that commit is completely reachable.
>
> For example:
>
>    git bisect start HEAD <known-good-commit> [ <damaged-or-missing-commit> ... ]

Sorry, this example should include --no-checkout.

    git bisect start --no-checkout HEAD <known-good-commit> [
<damaged-or-missing-commit> ... ]

Example in documentation is correct.

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Christian Couder]

Hi,

On Saturday 30 July 2011 10:28:26 Jon Seymour wrote:
> For some bisection tasks, checking out the commit at each stage of the
> bisection process is unecessary or undesirable.
> 
> This series adds support for a --no-checkout[=<ref>] option to git-bisect.

Sorry but I didn't reply to your previous email when you asked about a "--no-
checkout[=<ref>]" compromise. I thought that Junio would reply and then I 
forgot about it.

My opinion is that if you really want to be able to use another ref, then 
there should be a special "--update-ref=<ref>" or "--use-ref=<ref>" option 
that is different from "--no-checkout".

"--no-checkout" looks like a boolean argument. And "--no-checkout[=<ref>]" may 
make the user think that this option will not checkout <ref>, and then it 
leads to the confusing question "but why would it checkout this f&#@ing ref in 
the first place?".

Thanks,
Christian.

Re: [PATCH 2/5] bisect: introduce support for --no-checkout=<ref> option.

[Christian Couder]

On Saturday 30 July 2011 10:28:28 Jon Seymour wrote:
> diff --git a/bisect.c b/bisect.c
> index dd7e8ed..e6c99a0 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -24,6 +24,8 @@ struct argv_array {
> 
>  static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
>  static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> +static const char *argv_update_ref[] = {"update-ref", "--no-deref",
>                                                           NULL, NULL, NULL};
> +static const char *cursor_ref = 0;

Please use "NULL" instead of "0" above.
And is this global really needed? I would prefer if you passed one more 
argument to some functions.

>  /* bits #0-15 in revision.h */
> 
> @@ -714,9 +716,19 @@ static int bisect_checkout(char *bisect_rev_hex)
>  	mark_expected_rev(bisect_rev_hex);
> 
>  	argv_checkout[2] = bisect_rev_hex;
> -	res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
> -	if (res)
> -		exit(res);
> +	if (!cursor_ref) {
> +	  res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
> +	  if (res) {
> +	    exit(res);
> +	  }

Style nit: braces are superfluous, please use:

if (res)
        exit(res); 

> +	} else {
> +	    argv_update_ref[2] = cursor_ref;
> +	    argv_update_ref[3] = bisect_rev_hex;
> +	    res = run_command_v_opt(argv_update_ref, RUN_GIT_CMD);
> +	    if (res) {
> +	      die("update-ref --no-deref %s failed on %s", cursor_ref,
> bisect_rev_hex);
> +	    }

Style nit: braces are superfluous.

> +	}

Thanks,
Christian.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Sat, Jul 30, 2011 at 11:48 PM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> Hi,
>
> On Saturday 30 July 2011 10:28:26 Jon Seymour wrote:
>> For some bisection tasks, checking out the commit at each stage of the
>> bisection process is unecessary or undesirable.
>>
>> This series adds support for a --no-checkout[=<ref>] option to git-bisect.
>
> Sorry but I didn't reply to your previous email when you asked about a "--no-
> checkout[=<ref>]" compromise. I thought that Junio would reply and then I
> forgot about it.
>
> My opinion is that if you really want to be able to use another ref, then
> there should be a special "--update-ref=<ref>" or "--use-ref=<ref>" option
> that is different from "--no-checkout".
>
> "--no-checkout" looks like a boolean argument. And "--no-checkout[=<ref>]" may
> make the user think that this option will not checkout <ref>, and then it
> leads to the confusing question "but why would it checkout this f&#@ing ref in
> the first place?".

Good suggestions.

So, to confirm that I understand:

    use --no-checkout to control (no-)checkout behaviour and
--update-ref to specify a ref other than HEAD?

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Christian Couder]

On Saturday 30 July 2011 15:58:16 Jon Seymour wrote:
> On Sat, Jul 30, 2011 at 11:48 PM, Christian Couder wrote:
> > 
> > Sorry but I didn't reply to your previous email when you asked about a
> > "--no- checkout[=<ref>]" compromise. I thought that Junio would reply
> > and then I forgot about it.
> > 
> > My opinion is that if you really want to be able to use another ref, then
> > there should be a special "--update-ref=<ref>" or "--use-ref=<ref>"
> > option that is different from "--no-checkout".
> > 
> > "--no-checkout" looks like a boolean argument. And
> > "--no-checkout[=<ref>]" may make the user think that this option will
> > not checkout <ref>, and then it leads to the confusing question "but why
> > would it checkout this f&#@ing ref in the first place?".
> 
> Good suggestions.
> 
> So, to confirm that I understand:
> 
>     use --no-checkout to control (no-)checkout behaviour and
> --update-ref to specify a ref other than HEAD?

Yeah, I think it would be less confusing like this.

Thanks,
Christian.

Re: [PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Christian Couder]

On Saturday 30 July 2011 10:28:29 Jon Seymour wrote:
> diff --git a/git-bisect.sh b/git-bisect.sh
> index b2186a8..6c4e853 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -3,7 +3,7 @@
>  USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
>  LONG_USAGE='git bisect help
>          print this long help message.
> -git bisect start [<bad> [<good>...]] [--] [<pathspec>...]
> +git bisect start [--no=checkout[=<ref>]] [<bad> [<good>...]] [--]

s/--no=checkout/--no-checkout/

> [<pathspec>...] reset bisect state and start bisection.
>  git bisect bad [<rev>]
>          mark <rev> a known-bad revision.
> @@ -34,6 +34,8 @@ 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"
> 
> +BISECT_NOCHECKOUT=$(test -f "${GIT_DIR}/BISECT_NOCHECKOUT" && cat
> "${GIT_DIR}/BISECT_NOCHECKOUT")

Is there a reason you use "${GIT_DIR}" instead of "$GIT_DIR" that is used 
everywhere else?

Thanks,
Christian.

Re: [PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Jon Seymour]

On Sun, Jul 31, 2011 at 12:34 AM, Christian Couder
<chriscool@tuxfamily.org> wrote:
>
> Is there a reason you use "${GIT_DIR}" instead of "$GIT_DIR" that is used
> everywhere else?
>

No, there is no good reason.

I've pushed a revision that addresses your comments to github  -
https://github.com/jonseymour/git/commits/no-checkout-v2
(git@github.com:jonseymour/git.git)

I'll post the revised series to the list once Junio and others have
had a chance to comment.

jon.

Re: [PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Christian Couder]

On Saturday 30 July 2011 19:00:22 Jon Seymour wrote:
> On Sun, Jul 31, 2011 at 12:34 AM, Christian Couder
> 
> <chriscool@tuxfamily.org> wrote:
> > Is there a reason you use "${GIT_DIR}" instead of "$GIT_DIR" that is used
> > everywhere else?
> 
> No, there is no good reason.
> 
> I've pushed a revision that addresses your comments to github  -
> https://github.com/jonseymour/git/commits/no-checkout-v2
> (git@github.com:jonseymour/git.git)

I commented on some of the patches there.

> I'll post the revised series to the list once Junio and others have
> had a chance to comment.

Thanks,
Christian.

Re: [PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> I've pushed a revision that addresses your comments to github  -
>> https://github.com/jonseymour/git/commits/no-checkout-v2
>> (git@github.com:jonseymour/git.git)
>
> I commented on some of the patches there.

Please do not do _there_ hiding from others who expect to see the topic
getting polished _here_.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> On Saturday 30 July 2011 15:58:16 Jon Seymour wrote:
>> On Sat, Jul 30, 2011 at 11:48 PM, Christian Couder wrote:
>> > 
>> > Sorry but I didn't reply to your previous email when you asked about a
>> > "--no- checkout[=<ref>]" compromise. I thought that Junio would reply
>> > and then I forgot about it.
>> > 
>> > My opinion is that if you really want to be able to use another ref, then
>> > there should be a special "--update-ref=<ref>" or "--use-ref=<ref>"
>> > option that is different from "--no-checkout".
>> > 
>> > "--no-checkout" looks like a boolean argument. And
>> > "--no-checkout[=<ref>]" may make the user think that this option will
>> > not checkout <ref>, and then it leads to the confusing question "but why
>> > would it checkout this f&#@ing ref in the first place?".
>> 
>> Good suggestions.
>> 
>> So, to confirm that I understand:
>> 
>>     use --no-checkout to control (no-)checkout behaviour and
>> --update-ref to specify a ref other than HEAD?
>
> Yeah, I think it would be less confusing like this.

When used without "--no-checkout" option, "bisect" need to check-out the
candidate version. What good would it do if it does _not_ update HEAD when
it does so?

While you are correct to point out --[no-]checkout is a boolean option,
this "we do not update HEAD but update this other thing" is not orthogonal
to the option. It does not make sense when we actually touch the working
tree.

My preference is not to play games with "we can specify a ref other than
HEAD" until somebody can demonstrate why it is a feature "because we need
to be able to do so in such and such times", not merely "because we can".

Re: [PATCH 3/5] bisect: introduce --no-checkout[=<ref>] support into porcelain.

[Christian Couder]

On Monday 01 August 2011 02:53:04 Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> >> I've pushed a revision that addresses your comments to github  -
> >> https://github.com/jonseymour/git/commits/no-checkout-v2
> >> (git@github.com:jonseymour/git.git)
> > 
> > I commented on some of the patches there.
> 
> Please do not do _there_ hiding from others who expect to see the topic
> getting polished _here_.

Ok, I will not comment on github any more.

Jon, please post your latest series on the list.

Thanks,
Christian.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Christian Couder]

On Monday 01 August 2011 03:00:10 Junio C Hamano wrote:
>
> When used without "--no-checkout" option, "bisect" need to check-out the
> candidate version. What good would it do if it does _not_ update HEAD when
> it does so?
> 
> While you are correct to point out --[no-]checkout is a boolean option,
> this "we do not update HEAD but update this other thing" is not orthogonal
> to the option. It does not make sense when we actually touch the working
> tree.

Yeah.

> My preference is not to play games with "we can specify a ref other than
> HEAD" until somebody can demonstrate why it is a feature "because we need
> to be able to do so in such and such times", not merely "because we can".

So you want Jon to post a patch series with only a "--no-checkout" option. Ok.

Thanks,
Christian.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Mon, Aug 1, 2011 at 11:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> On Saturday 30 July 2011 15:58:16 Jon Seymour wrote:
>>> On Sat, Jul 30, 2011 at 11:48 PM, Christian Couder wrote:
>>> >
>>> > Sorry but I didn't reply to your previous email when you asked about a
>>> > "--no- checkout[=<ref>]" compromise. I thought that Junio would reply
>>> > and then I forgot about it.
>>> >
>>> > My opinion is that if you really want to be able to use another ref, then
>>> > there should be a special "--update-ref=<ref>" or "--use-ref=<ref>"
>>> > option that is different from "--no-checkout".
>>> >
>>> > "--no-checkout" looks like a boolean argument. And
>>> > "--no-checkout[=<ref>]" may make the user think that this option will
>>> > not checkout <ref>, and then it leads to the confusing question "but why
>>> > would it checkout this f&#@ing ref in the first place?".
>>>
>>> Good suggestions.
>>>
>>> So, to confirm that I understand:
>>>
>>>     use --no-checkout to control (no-)checkout behaviour and
>>> --update-ref to specify a ref other than HEAD?
>>
>> Yeah, I think it would be less confusing like this.
>
> When used without "--no-checkout" option, "bisect" need to check-out the
> candidate version. What good would it do if it does _not_ update HEAD when
> it does so?
>
> While you are correct to point out --[no-]checkout is a boolean option,
> this "we do not update HEAD but update this other thing" is not orthogonal
> to the option. It does not make sense when we actually touch the working
> tree.
>
> My preference is not to play games with "we can specify a ref other than
> HEAD" until somebody can demonstrate why it is a feature "because we need
> to be able to do so in such and such times", not merely "because we can".
>

Conceptually, I do think HEAD and "state of current bisection" are
different concepts and so it wouldn't hurt to be able to use  a
different reference for this purpose.

The main argument I have is the one that you raised earlier regarding
potential user confusion that would result from the somewhat
"spurious" (from the point of view of the naive user) differences that
might arise between the working tree, index and HEAD during the course
of the bisection process.

Of course, we can explain this in the documentation of the
--no-checkout option, so this needn't be a huge concern.

It might become more important if someone ever writes a tool that does
a bisection on the user's behalf. In this case, aborting the tool
might leave the HEAD in, what appears to the user, a confused state.
It would probably simplify error handling and abort processing in this
case if the tool never had to touch the HEAD reference at all.

That said, such concerns are hypothetical for now and for my primary
use-case for tree-less bisection (e.g. inspecting a damaged repo) I
can live with using HEAD for this purpose.

So, I will re-roll with references to --update-ref omitted.

Expect a v11 to the list, in the next half day or so.

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> It might become more important if someone ever writes a tool that does
> a bisection on the user's behalf. In this case, aborting the tool
> might leave the HEAD in, what appears to the user, a confused state.

Yes, I would prefer a series without --use-this-ref-for-bisect-status and
then a follow-up series on top of it to add that as a separate feature.

Thanks.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> It might become more important if someone ever writes a tool that does
>> a bisection on the user's behalf. In this case, aborting the tool
>> might leave the HEAD in, what appears to the user, a confused state.
>
> Yes, I would prefer a series without --use-this-ref-for-bisect-status and
> then a follow-up series on top of it to add that as a separate feature.
>
> Thanks.
>

Ok, I understand that you are not convinced that such a series is
necessary and so may not integrate it at this point.

However, I will prepare one, just for the record.

In this hypothetical additional series, are you happy for
--no-checkout to become a synonym for --update-ref=HEAD in the manner
of v8? From a technical perspective, it doesn't seem necessary to
duplicate the state variables and parameters.

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jon Seymour <jon.seymour@gmail.com> writes:
>>
>>> It might become more important if someone ever writes a tool that does
>>> a bisection on the user's behalf. In this case, aborting the tool
>>> might leave the HEAD in, what appears to the user, a confused state.
> ...
> In this hypothetical additional series, are you happy for
> --no-checkout to become a synonym for --update-ref=HEAD in the manner
> of v8? From a technical perspective, it doesn't seem necessary to
> duplicate the state variables and parameters.

Well, from a technical perspective, which ref is to be updated is an
option that is valid _only_ under no-checkout mode, and no-checkout mode
could be using HEAD, so I think you would need two variables. One for
"what mode are we running", and the other that is only valid under "we are
in no-checkout mode" that says "we update BISECT_HEAD".

If no-checkout mode _never_ updates HEAD, because the normal mode _always_
updates HEAD, you could reduce them into a single variable (i.e. if we are
updating HEAD then we are in normal mode, otherwise we are in no-checkout
mode).  I actually have a mild suspicion that the no-checkout mode may
turn out to be too confusing for people if it by default updates HEAD, and
we may want to have it update something that is not HEAD by default (or
forbid --update-ref from specifying HEAD), so if that turns out to be the
approach we are going to proceed, we _might_ end up needing only one
variable, but I do not think we know it just yet.  At least I don't.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Tue, Aug 2, 2011 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jon Seymour <jon.seymour@gmail.com> writes:
>>>
>>>> It might become more important if someone ever writes a tool that does
>>>> a bisection on the user's behalf. In this case, aborting the tool
>>>> might leave the HEAD in, what appears to the user, a confused state.
>> ...
>> In this hypothetical additional series, are you happy for
>> --no-checkout to become a synonym for --update-ref=HEAD in the manner
>> of v8? From a technical perspective, it doesn't seem necessary to
>> duplicate the state variables and parameters.
>
> Well, from a technical perspective, which ref is to be updated is an
> option that is valid _only_ under no-checkout mode, and no-checkout mode
> could be using HEAD, so I think you would need two variables. One for
> "what mode are we running", and the other that is only valid under "we are
> in no-checkout mode" that says "we update BISECT_HEAD".
>
> If no-checkout mode _never_ updates HEAD, because the normal mode _always_
> updates HEAD, you could reduce them into a single variable (i.e. if we are
> updating HEAD then we are in normal mode, otherwise we are in no-checkout
> mode).  I actually have a mild suspicion that the no-checkout mode may
> turn out to be too confusing for people if it by default updates HEAD, and
> we may want to have it update something that is not HEAD by default (or
> forbid --update-ref from specifying HEAD), so if that turns out to be the
> approach we are going to proceed, we _might_ end up needing only one
> variable, but I do not think we know it just yet.  At least I don't.
>

How about in the current series, I change the option parsed by
bisect--helper.c to be:

    --bisect-mode=checkout|update-ref

The porcelain option can still be --no-checkout (or should it be
--bisect-mode too?). The porcelain will maintain a state variable
called (rather than BISECT_NO_CHECKOUT).

     BISECT_MODE

Then, the next series can add a --bisect-head option (instead of
--update-ref) to allow the head to be varied (defaulting to
BISECT_HEAD) if not specified.

Sound ok?

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Tue, Aug 2, 2011 at 11:15 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Tue, Aug 2, 2011 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> How about in the current series, I change the option parsed by
> bisect--helper.c to be:
>
>    --bisect-mode=checkout|update-ref
>
> The porcelain option can still be --no-checkout (or should it be
> --bisect-mode too?). The porcelain will maintain a state variable
> called (rather than BISECT_NO_CHECKOUT).
>
>     BISECT_MODE
>
> Then, the next series can add a --bisect-head option (instead of
> --update-ref) to allow the head to be varied (defaulting to
> BISECT_HEAD) if not specified.

To clarify: --bisect-head would default to HEAD if mode is checkout,
to BISECT_HEAD if the mode is update-ref.

jon.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

>> Then, the next series can add a --bisect-head option (instead of
>> --update-ref) to allow the head to be varied (defaulting to
>> BISECT_HEAD) if not specified.
>
> To clarify: --bisect-head would default to HEAD if mode is checkout,
> to BISECT_HEAD if the mode is update-ref.

I am not sure if we ever want to allow updating anything but HEAD under
checkout mode, so I would rather think --bisect-head is _always_ invalid
under that mode. IOW, as I said in the previous message, these two are not
independent options.

Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.

[Jon Seymour]

On Wed, Aug 3, 2011 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>>> Then, the next series can add a --bisect-head option (instead of
>>> --update-ref) to allow the head to be varied (defaulting to
>>> BISECT_HEAD) if not specified.
>>
>> To clarify: --bisect-head would default to HEAD if mode is checkout,
>> to BISECT_HEAD if the mode is update-ref.
>
> I am not sure if we ever want to allow updating anything but HEAD under
> checkout mode, so I would rather think --bisect-head is _always_ invalid
> under that mode. IOW, as I said in the previous message, these two are not
> independent options.
>

I agree. In fact, I like your idea of a BISECT_HEAD for --no-checkout,
and I don't see any need (even in the future)
to allow this to be configurable [ except perhaps if someone thinks
doing a --no-checkout with HEAD is actually useful ].

jon.