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

Christian Couder [off-list ref] writes:

Le mercredi 12 novembre 2008, Junio C Hamano a écrit :
...

quoted

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 <redacted>
---
 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

Le jeudi 13 novembre 2008, Junio C Hamano a écrit :

Christian Couder [off-list ref] writes:

quoted

Le mercredi 12 novembre 2008, Junio C Hamano a écrit :
...

quoted

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.