[PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Christian Couder]

This subcommand should be used when you have a branch or a part of a
branch that isn't easily bisectable because of a bug that has been
fixed latter.

We suppose that a bug as been introduced at some point, say A, and
that it has been fixed latter at another point, say B, but that
between these points the code is not easily testable because of the
bug, so it's not easy to bisect between these points.

In this case you can create a branch starting at the parent of A, say
O, that has a fixed history. In this fixed history for example, there
could be first a commit C that is the result of squashing A and B
together and then all the commits between A and B that have been
cherry picked.

For example, let's say the commits between A and B are X1, X2, ... Xn
and they have been cherry picked after C as Y1, Y2, ... Yn:

        C--Y1--Y2--...--Yn
      /
...--O--A--X1--X2--...--Xn--B--...

By design, the last cherry picked commit (Yn) should point to the same
tree as commit B.

So in this case you can say:

$ git bisect replace B Yn

and a branch will be created that points to commit Yn and that has a
special name like: "bisect-replace-B"

When bisecting, the branch names will be scanned and each branch named
"bisect-replace-X" and pointing to commit Y will be grafted so that
X will only have Y as parent.

In the example above, that means that instead of the above graph, the
following graph will be bisected:

        C--Y1--Y2--...--Yn
      /                   \
...--O                     B--...

This means that the bisections on this branch will be much easier
because the bug introduced by commit A and fixed by commit B will not
annoy you anymore.

As the branches created by "git bisect replace" can be shared between
developers, this feature might be especially usefull on big projects
where many people often bisect the same code base.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-rev-list.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh      |   37 +++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 3 deletions(-)

	Changes since the previous series (v3) are the following:

	- move 2 lines of code in patch 1, 
	- added tests in patch 7,
	- added --no-replace option (patches 8 and 9) 

	I still wonder about the following:

	- perhaps "git bisect replace" should have a "NAME" argument
	so that the branch names could be like "bisect-replace-X-NAME"
	which could be more descriptive,
	- maybe a way to check that the replace branches all point to
	existing commits would be useful,
	- perhaps bisect log/replay could be improved to handle
	replace branches

	But I think it's already possible to do many things in the
	current state, especially by using "git branch -m" (to rename
	branches). For example to give a more explicit name:

	$ git branch -m bisect-replace-X bisect-replace-X-explicit-name

	and to not replace a branch any more (but keep it in case you may
	use it again latter):

	$ git branch -m bisect-replace-Y bisect-do-not-replace-Y

	...

	So as I don't want to over-engineer this and I am a bit lazy too,
	I will wait for some comments. Thanks in advance for them!

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 06cdeb7..c0717f6 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -574,6 +574,72 @@ static struct commit_list *find_bisection(struct commit_list *list,
 	return best;
 }
 
+static void replace_parents(struct commit *commit,
+			    const char *refname,
+			    const unsigned char *sha1)
+{
+	struct commit *new_parent = lookup_commit(sha1);
+	if (!new_parent) {
+		warning("branch '%s' points to unknown commit '%s'",
+			refname, sha1_to_hex(sha1));
+		return;
+	}
+
+	free_commit_list(commit->parents);
+	commit->parents = NULL;
+	commit_list_insert(new_parent, &commit->parents);
+}
+
+static int bisect_replace(const char *refname, const unsigned char *sha1,
+			  int flag, void *cb_data)
+{
+	unsigned char child[20];
+	struct object *obj;
+	struct commit_graft *graft;
+
+	if (prefixcmp(refname, "bisect-replace-"))
+		return 0;
+
+	if (get_sha1_hex(refname + 15, child)) {
+		warning("bad sha1 in branch name '%s'", refname);
+		return 0;
+	}
+
+	/* Check if child commit exist and is already parsed */
+
+	obj = lookup_object(child);
+	if (obj) {
+		struct commit *commit;
+		if (obj->type != OBJ_COMMIT) {
+			warning("branch name '%s' refers to non commit '%s'",
+				refname, refname + 15);
+			return 0;
+		}
+		commit = (struct commit *) obj;
+		if (commit->object.parsed) {
+			replace_parents(commit, refname, sha1);
+			return 0;
+		}
+	}
+
+	/* Create a graft to replace child commit parents */
+
+	graft = xmalloc(sizeof(*graft) + 20);
+
+	hashcpy(graft->sha1, child);
+	graft->nr_parent = 1;
+	hashcpy(graft->parent[0], sha1);
+
+	register_commit_graft(graft, 1);
+
+	return 0;
+}
+
+static void bisect_replace_all(void)
+{
+	for_each_branch_ref(bisect_replace, NULL);
+}
+
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
@@ -646,8 +712,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = revs.verbose_header ||
 		revs.grep_filter.pattern_list;
-	if (bisect_list)
+
+	if (bisect_list) {
+		bisect_replace_all();
 		revs.limited = 1;
+	}
 
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
diff --git a/git-bisect.sh b/git-bisect.sh
index 0d0e278..b0139d5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|replace|run]'
 LONG_USAGE='git bisect help
         print this long help message.
 git bisect start [<bad> [<good>...]] [--] [<pathspec>...]
@@ -21,6 +21,8 @@ git bisect replay <logfile>
         replay bisection log.
 git bisect log
         show bisect log.
+git bisect replace <rev> [<rev>]
+        use another branch for bisection.
 git bisect run <cmd>...
         use <cmd>... to automatically bisect.
 
@@ -566,6 +568,36 @@ bisect_replay () {
 	bisect_auto_next
 }
 
+bisect_replace() {
+    test "$#" -ge 1 -a "$#" -le 2 ||
+        die "'git bisect replace' accept one or two arguments"
+
+    source="$1"
+    target="${2:-HEAD}"
+
+    # Check arguments
+    src_commit=$(git rev-parse --verify "$source^{commit}") ||
+        die "Bad rev input: $source"
+    tgt_commit=$(git rev-parse --verify "$target^{commit}") ||
+        die "Bad rev input: $target"
+
+    test "$src_commit" != "tgt_commit" ||
+        die "source and target should be different commits"
+
+    # Check that trees from source and target are identical
+    src_tree=$(git rev-parse --verify "$src_commit^{tree}") ||
+        die "Could not get tree for source: $source"
+    tgt_tree=$(git rev-parse --verify "$tgt_commit^{tree}") ||
+        die "Could not get tree for target: $target"
+
+    test "$src_tree" = "$tgt_tree" ||
+        die "source and target should point to the same tree"
+
+    # Create branch for the target commit
+    tgt_branch="bisect-replace-$src_commit"
+    git branch "$tgt_branch" "$tgt_commit"
+}
+
 bisect_run () {
     bisect_next_check fail
 
@@ -618,7 +650,6 @@ bisect_run () {
     done
 }
 
-
 case "$#" in
 0)
     usage ;;
@@ -643,6 +674,8 @@ case "$#" in
 	bisect_replay "$@" ;;
     log)
 	cat "$GIT_DIR/BISECT_LOG" ;;
+    replace)
+        bisect_replace "$@" ;;
     run)
         bisect_run "$@" ;;
     *)
-- 
1.6.0.3.614.g0f3b9

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This subcommand should be used when you have a branch or a part of a
> branch that isn't easily bisectable because of a bug that has been
> fixed latter.

While I acknowledge your effort to make bisect easier to use, I do
not think this is going in the right direction, from the point of view of
the workflow.

I do agree that the issue it tries to solve is a problem in real life.
When you want to hunt for a bug, it is certainly possible that your tests
fail for a bug that is unrelated to what you are hunting for for a range
of commits.  Borrowing from your picture:

    ...--O--A--X1--X2--...--Xn--B--...

non of the commit marked as Xi may not be testable.

But at that point, will you really spend time to rebuild history between A
and B by fixing an unrelated bug that hinders your bisect, so that you can
have a parallel history that is bisectable?  I doubt anybody would.

Even if we assume that somebody wants to adopt the workflow to first fix
an unrelated bug (that may be totally uninteresting for the purpose of
solving the original issue he set out to figure out) to rewrite the
history, what he first needs to do is to find out what part of the history
to rewrite.  IOW, he needs to know A and B (and in general, the history is
not even linear).  Maybe he guesses what A and B is.  But for one thing,
after making the guess, he would certainly test A and B to see if the
original issue exists at these commits.  The sequence of commits Xi become
irrelevant if A turns out to be bad or B turns out to be good.

And if A is good and B is bad, then the _original bug_ is in the very
sequence of Xi you are going to rewrite.  By the time you made a rewritten
history with sequence of commits Yi to be grafted like this:


         C--Y1--Y2--...--Yn
       /
 ...--O--A--X1--X2--...--Xn--B--...

to make it bisectable, it is very likely that you would have already seen
the original bug.

In such a case where you need to figure out what an unrelated bug is, and
which commit A and B are involved while bisecting, I think you are much
better off using bisect skip, as Johannes mentioned earlier.

On the other hand, if you already have a well-known bug that was
introduced at A whose fix at B is also very well-known, you would not even
need a separate "bisect replace" command nor replace_parents() machinery
only for the purpose of bisection, would you?  In such a case I think you
can just use a usual graft.


I have a separate idea make 'grafts' easier on object transfer, that is
somewhat related to this one, by the way.  Instead of making the grafts
completely a local matter as we do now, we can reserve refs/replace/
namespace, and record a new commit object to replace an existing commit
whose object name is $sha1 as refs/replace/$sha1.  We make almost all the
commands except object enumeration (fsck, receive-pack, send-pack, prune,
etc.  Roughly speaking, anything that involves "rev-list --objects") honor
this commit replacement, so that any time you ask for commit $sha1, the
object layer gives you the replacement commit object back.  In this way,
you can clone or fetch from such a repository (along with refs in
refs/replace/ hierarchy) and fsck/prune won't lose the original parents
(because it does not see replacements).  Things like paranoid update hook
needs to become very careful about refs/replace/ for security reasons, but
I think this would make the grafts much easier to use.

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Christian Couder]

Le mercredi 12 novembre 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > This subcommand should be used when you have a branch or a part of a
> > branch that isn't easily bisectable because of a bug that has been
> > fixed latter.
>
> While I acknowledge your effort to make bisect easier to use, I do
> not think this is going in the right direction, from the point of view of
> the workflow.
>
> I do agree that the issue it tries to solve is a problem in real life.
> When you want to hunt for a bug, it is certainly possible that your tests
> fail for a bug that is unrelated to what you are hunting for for a range
> of commits.  Borrowing from your picture:
>
>     ...--O--A--X1--X2--...--Xn--B--...
>
> non of the commit marked as Xi may not be testable.
>
> But at that point, will you really spend time to rebuild history between
> A and B by fixing an unrelated bug that hinders your bisect, so that you
> can have a parallel history that is bisectable?  I doubt anybody would.

I think kernel developers and perhaps others do that somehow. I mean, there 
is the following text in the git-bisect(1) documentation:

"
You may often find that during bisect you want to have near-constant tweaks 
(e.g., s/#define DEBUG 0/#define DEBUG 1/ in a header file, or "revision 
that does not have this commit needs this patch applied to work around 
other problem this bisection is not interested in") applied to the revision 
being tested.

To cope with such a situation, after the inner git-bisect finds the next 
revision to test, with the "run" script, you can apply that tweak before 
compiling, run the real test, and after the test decides if the revision 
(possibly with the needed tweaks) passed the test, rewind the tree to the 
pristine state. Finally the "run" script can exit with the status of the 
real test to let the "git bisect run" command loop to determine the 
outcome.
"

So we suggest that people patch at bisect time in case of problems. But I 
think creating a parallel branch should be better in the long run, because 
you can easily keep the work you did to make things easier to bisect and 
you can easily share it with other people working with you. Also using "git 
rebase -i" to create the fixed branch might be easier and more reliable 
than trying to patch at each step.

Of course it also depends on how often people use "git bisect", but it seems 
that there are people out there bisecting very frequently and that these 
people care very much about bisectability of the tree.

> Even if we assume that somebody wants to adopt the workflow to first fix
> an unrelated bug (that may be totally uninteresting for the purpose of
> solving the original issue he set out to figure out) to rewrite the
> history, what he first needs to do is to find out what part of the
> history to rewrite.

Not necessarily. People may decide to adopt a workflow that consists in 
creating a fixed branch as soon as they know about a fix that may prevent 
their code from being tested. This way they know that they will be able to 
bisect at each commit of their full history.

> IOW, he needs to know A and B (and in general, the 
> history is not even linear).  Maybe he guesses what A and B is.  But for
> one thing, after making the guess, he would certainly test A and B to see
> if the original issue exists at these commits.  The sequence of commits
> Xi become irrelevant if A turns out to be bad or B turns out to be good.

Yes that's possible. But in some cases people might find it simpler (for 
example because it might take a long time or a lot of manpower to fully 
test one commit) to just create a fixed branch as soon as they find 
something that prevents testing which might happen during a bisection or 
not.

> And if A is good and B is bad, then the _original bug_ is in the very
> sequence of Xi you are going to rewrite.  By the time you made a
> rewritten history with sequence of commits Yi to be grafted like this:
>
>
>          C--Y1--Y2--...--Yn
>        /
>  ...--O--A--X1--X2--...--Xn--B--...
>
> to make it bisectable, it is very likely that you would have already seen
> the original bug.

I am not sure I understand/agree with that, because I think it may be quite 
easy to make such a fixed branch, especially if the commit message of B is 
a good one like "fix build problem on platform 'foo' introduced by A".

> In such a case where you need to figure out what an unrelated bug is, and
> which commit A and B are involved while bisecting, I think you are much
> better off using bisect skip, as Johannes mentioned earlier.

Even if there was a bisect skip command that could skip a range of commit, 
you might find in the end that the original bug you are looking for was 
introduced in a range you skipped.

> On the other hand, if you already have a well-known bug that was
> introduced at A whose fix at B is also very well-known, you would not
> even need a separate "bisect replace" command nor replace_parents()
> machinery only for the purpose of bisection, would you?  In such a case I
> think you can just use a usual graft.

I don't know very much usual grafts but I think that the problem is that 
usual grafts rewrite history for (nearly) all the commands.

It seems to me that with usual grafts for this purpose, people may for 
example have problems refering to some commits, because it might happen 
that the sha1 of a commit might be in a branch that has been grafted out 
the current branch for easier bisecting.

For example in my patch series, the 2 last patches add a --no-replace option 
to "git bisect", so that it's easy not to use "bisect-replace" branches if 
one does not want too. And I fear that if graft use is generalized, it may 
be necessary to add such options to many other commands (instead of just 
one). And even then, people may want to use some of the grafts but not 
others (for example grafts to see some very old commits imported after the 
initial repo was created but not grafts to fix history) for some purposes 
and a different set (for example all the grafts) for other purposes (like 
bisecting).

> I have a separate idea make 'grafts' easier on object transfer, that is
> somewhat related to this one, by the way.  Instead of making the grafts
> completely a local matter as we do now, we can reserve refs/replace/
> namespace, and record a new commit object to replace an existing commit
> whose object name is $sha1 as refs/replace/$sha1.  We make almost all the
> commands except object enumeration (fsck, receive-pack, send-pack, prune,
> etc.  Roughly speaking, anything that involves "rev-list --objects")
> honor this commit replacement, so that any time you ask for commit $sha1,
> the object layer gives you the replacement commit object back.  In this
> way, you can clone or fetch from such a repository (along with refs in
> refs/replace/ hierarchy) and fsck/prune won't lose the original parents
> (because it does not see replacements).  Things like paranoid update hook
> needs to become very careful about refs/replace/ for security reasons,
> but I think this would make the grafts much easier to use.

I agree that it would make grafts much easier to use (and would be very 
security sensitive).

But except for grafts used to connect to old histories, do you know many 
special commands other than "git bisect" that may use the refs/replace/ 
namespace?

I mean, it may be a good idea to use a special namespace with sha1 named 
refs like "refs/xxx/$sha1" instead of branches named 
like "bisect-replace-SHA1", but I think that it would be better if grafts 
used to fix history for bisecting purpose would be separated from other 
kind of grafts, for example perhaps in "refs/replace/bisect/$sha1".

Regards,
Christian.

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Le mercredi 12 novembre 2008, Junio C Hamano a écrit :
> ...
>> When you want to hunt for a bug, it is certainly possible that your tests
>> fail for a bug that is unrelated to what you are hunting for for a range
>> of commits.  Borrowing from your picture:
>>
>>     ...--O--A--X1--X2--...--Xn--B--...
>>
>> non of the commit marked as Xi may not be testable.
>>
>> But at that point, will you really spend time to rebuild history between
>> A and B by fixing an unrelated bug that hinders your bisect, so that you
>> can have a parallel history that is bisectable?  I doubt anybody would.
>
> I think kernel developers and perhaps others do that somehow. I mean, there 
> is the following text in the git-bisect(1) documentation:
>
> "
> You may often find that during bisect you want to have near-constant tweaks 
> (e.g., s/#define DEBUG 0/#define DEBUG 1/ in a header file, or "revision 
> that does not have this commit needs this patch applied to work around 
> other problem this bisection is not interested in") applied to the revision 
> being tested.
>
> To cope with such a situation, after the inner git-bisect finds the next 
> revision to test, with the "run" script, you can apply that tweak before 
> compiling,...
> "
>
> So we suggest that people patch at bisect time in case of problems. But I 
> think creating a parallel branch should be better in the long run, because 
> you can easily keep the work you did to make things easier to bisect and 
> you can easily share it with other people working with you.

I strongly disagree.

Maybe you hit X2 which does have a breakage, and you would need to patch
up that one before being able to test for your bug, but after you say good
or bad on that one, the next pick will be far away from that Xi segment.
You will test many more revisions before you come back to X3 or X5.  Why
should we force the users to fix all the commits in the segment "just in
case" somebody's bisect falls into the range before that actually happens?

In other words, unless the breakage you are hunting for exists between
point A and B that you cannot bisect for that other breakage, you won't
need to patch-up _every single revision_ in the range for the breakage.
Doing so beforehand is wasteful.

And if you know the range of A..B and the fix, the procedure to follow the
suggestion you quoted above from the doc can even be automated relatively
easily.  Your "run" script would need to do two merge-base to see if the
version to be tested falls inside the range, and if so apply the known fix
before testing (and clean it up afterwards).

Come to think of it, you do not even need to have a custom run script.
How about an approach illustrated by this patch?

I think it also needs to call unapply_fixup when "bisect reset" is given
to clean up, but this is more about a possible approach and not about a
adding a polished and finished shiny new toy, so...

-- >8 --
bisect: use canned "fixup patch"

This allows you to have $GIT_DIR/bisect-fixup directory, populated with
"fixup" patch files.  When revisions between A and B (inclusive) cannot be
tested for your bisection purpose because they have other unrelated
breakage whose fix is already known, you can store a fix-up patch in the
file whose name is A-B (the commit object name of A, a dash, then the
commit object name of B).  When "git bisect" suggests you to test a
revision that falls in that range, the patch will automatically be
applied to the working tree, so that you can test with the known fix
already in place.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-bisect.sh           |   36 ++++++++++++++++++++++++++
 t/t6035-bisect-fixup.sh |   64 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 0 deletions(-)

diff --git c/git-bisect.sh w/git-bisect.sh
index 0d0e278..600d9aa 100755
--- c/git-bisect.sh
+++ w/git-bisect.sh
@@ -338,13 +338,48 @@ exit_if_skipped_commits () {
 	fi
 }
 
+apply_fixup() {
+	>"$GIT_DIR/BISECT_FIXUP"
+	(
+		cd "$GIT_DIR" 2>/dev/null &&
+		find bisect-fixup -type f -print
+	) |
+	while read _fixup
+	do
+		_btm=$(expr "$_fixup" : 'bisect-fixup/\([0-9a-f]*\)-[0-9a-f]*$') &&
+		_btm=$(git rev-parse --quiet --verify "$_btm" 2>/dev/null) &&
+		_top=$(expr "$_fixup" : 'bisect-fixup/[0-9a-f]*-\([0-9a-f]*\)$') &&
+		_top=$(git rev-parse --quiet --verify "$_top" 2>/dev/null) ||
+		continue
+		if	test "z$(git merge-base $_btm "$1")" = z$_btm &&
+			test "z$(git merge-base "$1" $_top)" = "z$1"
+		then
+			echo "Applying $_fixup..."
+			git apply "$GIT_DIR/$_fixup" || exit 1
+			echo >>"$GIT_DIR/BISECT_FIXUP" "$_fixup"
+		fi
+	done
+}
+
+unapply_fixup() {
+	test -r "$GIT_DIR/BISECT_FIXUP" || return 0
+	@@PERL@@ -e "print reverse <>" "$GIT_DIR/BISECT_FIXUP" |
+	while read _fixup
+	do
+		echo "Unapplying $_fixup..."
+		git apply -R "$GIT_DIR/$_fixup" || exit
+	done
+}
+
 bisect_checkout() {
+	unapply_fixup || exit ;# should not happen but try to be careful
 	_rev="$1"
 	_msg="$2"
 	echo "Bisecting: $_msg"
 	mark_expected_rev "$_rev"
 	git checkout -q "$_rev" || exit
 	git show-branch "$_rev"
+	apply_fixup "$_rev" || exit
 }
 
 is_among() {
@@ -532,6 +567,7 @@ bisect_clean_state() {
 	do
 		git update-ref -d $ref $hash || exit
 	done
+	rm -f "$GIT_DIR/BISECT_FIXUP" &&
 	rm -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
 	rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
 	rm -f "$GIT_DIR/BISECT_LOG" &&
diff --git c/t/t6035-bisect-fixup.sh w/t/t6035-bisect-fixup.sh
new file mode 100755
index 0000000..4745b73
--- /dev/null
+++ w/t/t6035-bisect-fixup.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+# Copyright (c) 2008, Junio C Hamano
+
+test_description='bisect fixup'
+
+. ./test-lib.sh
+
+cat >test-fixup <<\EOF
+diff a/elif b/elif
+--- a/elif
++++ b/elif
+@@ -1 +1 @@
+-0
++Z
+EOF
+
+test_expect_success setup '
+	echo 0 >elif
+	git add elif
+	for i in 1 2 3 4 5 6 7 8 9
+	do
+		test_tick &&
+		echo $i >file &&
+		git add file &&
+		git commit -m $i &&
+		git tag t$i
+	done &&
+
+	mkdir ".git/bisect-fixup" &&
+
+	btm=$(git rev-parse --short t3) &&
+	top=$(git rev-parse --short t5) &&
+	cat test-fixup >".git/bisect-fixup/$btm-$top" &&
+
+	btm=$(git rev-parse --short t7) &&
+	top=$(git rev-parse --short t8) &&
+	sed -e "s|Z|A|" test-fixup >".git/bisect-fixup/$btm-$top"
+'
+
+test_expect_success 'bisect fixup' '
+	t1=$(git rev-parse t1) &&
+	t5=$(git rev-parse t5) &&
+	t6=$(git rev-parse t6) &&
+	t7=$(git rev-parse t7) &&
+	t9=$(git rev-parse t9) &&
+
+	git bisect start &&
+	test $(git rev-parse HEAD) = $t9 &&
+	git bisect bad t9 &&
+	test $(git rev-parse HEAD) = $t9 &&
+	git bisect good t1 &&
+	test $(git rev-parse HEAD) = $t5 &&
+	grep Z elif &&
+
+	git bisect good &&
+	test $(git rev-parse HEAD) = $t7 &&
+	grep A elif &&
+
+	git bisect bad &&
+	test $(git rev-parse HEAD) = $t6 &&
+	grep 0 elif
+'
+
+test_done

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Paolo Bonzini]

> Of course it also depends on how often people use "git bisect", but it seems 
> that there are people out there bisecting very frequently and that these 
> people care very much about bisectability of the tree.

What about "git bisect cherry-pick COMMIT" which would do a "cherry-pick
-n" of COMMIT after every bisection unless COMMIT is in the ancestry of
the current revision?  So if you have to bisect between A and B, and you
know that a bug present between A and B was fixed by C, you could do

  git bisect good A
  git bisect bad B
  git bisect cherry-pick C

This would subsume Junio's proposal: users could also set up a few
special bisect/set-debug-to-1, bisect/remove-cflags-o2 and so on patches
that you could use for purposes other than ensuring known bugs are fixed.

Finally, you could have a [bisect] configuration section with entries
like "cherry-pick = BROKEN-SHA1 FIX-SHA1" and "git bisect" would apply
FIX-SHA1 automatically if the bisect tip were in BROKEN-SHA1..FIX-SHA1.

>> Things like paranoid update hook
>> needs to become very careful about refs/replace/ for security reasons,
>> but I think this would make the grafts much easier to use.
> 
> I agree that it would make grafts much easier to use (and would be very 
> security sensitive).

Not so much.  People with public servers could create refs/replace in
the server and give it 400 permissions, and/or git would refuse to
create the refs/replace directory, forcing users to create it manually
unless it is already in the server.

Paolo

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Christian Couder]

Le jeudi 13 novembre 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Le mercredi 12 novembre 2008, Junio C Hamano a écrit :
> > ...
> >
> >> When you want to hunt for a bug, it is certainly possible that your
> >> tests fail for a bug that is unrelated to what you are hunting for for
> >> a range of commits.  Borrowing from your picture:
> >>
> >>     ...--O--A--X1--X2--...--Xn--B--...
> >>
> >> non of the commit marked as Xi may not be testable.
> >>
> >> But at that point, will you really spend time to rebuild history
> >> between A and B by fixing an unrelated bug that hinders your bisect,
> >> so that you can have a parallel history that is bisectable?  I doubt
> >> anybody would.
> >
> > I think kernel developers and perhaps others do that somehow. I mean,
> > there is the following text in the git-bisect(1) documentation:
> >
> > "
> > You may often find that during bisect you want to have near-constant
> > tweaks (e.g., s/#define DEBUG 0/#define DEBUG 1/ in a header file, or
> > "revision that does not have this commit needs this patch applied to
> > work around other problem this bisection is not interested in") applied
> > to the revision being tested.
> >
> > To cope with such a situation, after the inner git-bisect finds the
> > next revision to test, with the "run" script, you can apply that tweak
> > before compiling,...
> > "
> >
> > So we suggest that people patch at bisect time in case of problems. But
> > I think creating a parallel branch should be better in the long run,
> > because you can easily keep the work you did to make things easier to
> > bisect and you can easily share it with other people working with you.
>
> I strongly disagree.
>
> Maybe you hit X2 which does have a breakage, and you would need to patch
> up that one before being able to test for your bug, but after you say
> good or bad on that one, the next pick will be far away from that Xi
> segment. You will test many more revisions before you come back to X3 or
> X5.  Why should we force the users to fix all the commits in the segment
> "just in case" somebody's bisect falls into the range before that
> actually happens?

The users would not be _forced_ at all to use "git bisect replace", they can 
ignore it if they want. And even if other coworkers use it, they are not 
forced at all to use it themself. If they get none of 
the "bisect-replace-*" branches, the behavior of git bisect will not change 
at all.

> In other words, unless the breakage you are hunting for exists between
> point A and B that you cannot bisect for that other breakage, you won't
> need to patch-up _every single revision_ in the range for the breakage.
> Doing so beforehand is wasteful.

I think that it depends on many factors. More precisely, it depends on how 
easy it is to fully test (because to make sure that the bug your are 
bisecting is between A and B you need to test A and B, so you waste some 
testing) vs how easy it is to patch up every single revision between A and 
B.

And I think it can be really easy to patch up every revision between A and 
B, you might need only something like:

$ git checkout -b patch-up B
$ git rebase -i A^

and then squash the last commit into the first one, in the list "git 
rebase -i" gives you. It may even be easy to automate this with code like 
this (completely untested, and it assumes git rebase -i accept a script 
from stdin which may not work right now):

create_replace_branch() {
	_a="$1"
	_b="$2"
	git checkout -b bisect-replace-$_b "$_b" || exit
	git rev-list ^"$_a"^ "$_b" | {
		while read sha1
		do
			case $sha1 in
				$_a) echo "pick $_a"; echo "squash $_b" ;;
				$_b) ;;
				*) echo "pick $sha1 ;;
			esac
		done
	} | git rebase -i "$_a"^
}

> And if you know the range of A..B and the fix, the procedure to follow
> the suggestion you quoted above from the doc can even be automated
> relatively easily.  Your "run" script would need to do two merge-base to
> see if the version to be tested falls inside the range, and if so apply
> the known fix before testing (and clean it up afterwards).
>
> Come to think of it, you do not even need to have a custom run script.
> How about an approach illustrated by this patch?

This is interesting, but the fix up patch might not apply cleanly on all the 
commits in the range, and there is no simple way to share these patches 
(and changes to them) in a team. Perhaps more importantly, there is also no 
simple way to look at the result from applying the patch or to manipulate 
it with other git commands.

I mean it was decided to store changes in Git as blob, trees and commits, 
not patches, so why would we store these changes as patches?

Regards,
Christian.

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Christian Couder]

Le jeudi 13 novembre 2008, Paolo Bonzini a écrit :
> > Of course it also depends on how often people use "git bisect", but it
> > seems that there are people out there bisecting very frequently and
> > that these people care very much about bisectability of the tree.
>
> What about "git bisect cherry-pick COMMIT" which would do a "cherry-pick
> -n" of COMMIT after every bisection unless COMMIT is in the ancestry of
> the current revision?  So if you have to bisect between A and B, and you
> know that a bug present between A and B was fixed by C, you could do
>
>   git bisect good A
>   git bisect bad B
>   git bisect cherry-pick C
>
> This would subsume Junio's proposal:

Yes, it looks better than Junio's, except when there is no existing commit 
that perfectly works.

> users could also set up a few 
> special bisect/set-debug-to-1, bisect/remove-cflags-o2 and so on patches
> that you could use for purposes other than ensuring known bugs are fixed.

In this case it is similar to Junio's proposal. But I think that for changes 
like set-debug-to-1 and remove-cflags-o2, using the right make command 
should be enough.

> Finally, you could have a [bisect] configuration section with entries
> like "cherry-pick = BROKEN-SHA1 FIX-SHA1" and "git bisect" would apply
> FIX-SHA1 automatically if the bisect tip were in BROKEN-SHA1..FIX-SHA1.

Yes, but how do you share this between members of a team?

Your proposal and Junio's proposal also don't use real sha1 names for 
commits that are tested, so it's more difficult to talk about them, share 
them, use them, ...

Regards,
Christian.

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Paolo Bonzini]

>> users could also set up a few 
>> special bisect/set-debug-to-1, bisect/remove-cflags-o2 and so on patches
>> that you could use for purposes other than ensuring known bugs are fixed.
> 
> In this case it is similar to Junio's proposal. But I think that for changes 
> like set-debug-to-1 and remove-cflags-o2, using the right make command 
> should be enough.

Yeah, I couldn't think of a better usecase, but you got the idea.

>> Finally, you could have a [bisect] configuration section with entries
>> like "cherry-pick = BROKEN-SHA1 FIX-SHA1" and "git bisect" would apply
>> FIX-SHA1 automatically if the bisect tip were in BROKEN-SHA1..FIX-SHA1.
> 
> Yes, but how do you share this between members of a team?

That's a common problem with stuff that goes in .gitconfig.  It does not
belong in the repository, though...

Paolo

Re: [PATCH 1/9 v4] bisect: add "git bisect replace" subcommand

[Christian Couder]

Le vendredi 14 novembre 2008, Paolo Bonzini a écrit :
> >> users could also set up a few
> >> special bisect/set-debug-to-1, bisect/remove-cflags-o2 and so on
> >> patches that you could use for purposes other than ensuring known bugs
> >> are fixed.
> >
> > In this case it is similar to Junio's proposal. But I think that for
> > changes like set-debug-to-1 and remove-cflags-o2, using the right make
> > command should be enough.
>
> Yeah, I couldn't think of a better usecase, but you got the idea.
>
> >> Finally, you could have a [bisect] configuration section with entries
> >> like "cherry-pick = BROKEN-SHA1 FIX-SHA1" and "git bisect" would apply
> >> FIX-SHA1 automatically if the bisect tip were in
> >> BROKEN-SHA1..FIX-SHA1.
> >
> > Yes, but how do you share this between members of a team?
>
> That's a common problem with stuff that goes in .gitconfig.  It does not
> belong in the repository, though...

Why?

Git encourages people to develop by creating many branches of commits, 
working on them and sharing them, and that seems to work very well. So I 
really don't understand why _the hell_ people using bisect could not also 
benefit of from creating patched up branches, sharing them, bisecting on 
them.

Especially as, in my patch series, it does not in _any way_ change anything 
for people who just don't want to use patched up branches. They just need 
to not download any "bisect-replace-*" branch, or we can even implement a 
config option "bisect.useReplaceBranches" that default to "no" if that is 
such a big threat to them.

I agree that the name of the branches "bisect-replace-*" may not be the best 
or that using special refs in "refs/replace/" might be better (except that 
these refs should have in my opinion a special namespace to be easily 
distinguished from others), but I don't think _at all_ that this should be 
enough to discard the whole idea (and implementation but that's another 
story).

Or is there a god command I don't know about that says:

Thou shall not use patched up branches to bisect!
Thou shall bisect painfully!

Regards,
Christian.