Heads up: rebase -i -p will be made sane again

Dear list,

I am progressing to a point where I am almost comfortable to send the 
patch series; I want to use the thing myself first, and I want to fix a 
design bug.

As always, my code is public, but will be rebased frequently.  You have 
been warned.

BTW I am really sorry for the state I left the --preserve-merges code for 
a long time.  Originally, it was never meant to be used interactively, and 
that shows sorely.

As for the design bug I want to fix: imagine this history:

  ------A
 /     /
/     /
---- B
\     \
 \     \
  C-----D-----E = HEAD

A, C and D touch the same file, and A and D agree on the contents.

Now, rebase -p A does the following at the moment:

  ------A-----E' = HEAD
 /     /
/     /
---- B

In other words, C is truly forgotten, and it is pretended that D never 
happened, either.  That is exactly what test case 2 in t3410 tests for 
[*1*].

This is insane.

So after my rebase -i -p revamp, this will happen instead: in the 
interactive version you will get the script

	pick C
	merge parents B' original D
	pick E

In the non-interactive version -- or if you change nothing, in the 
interactive version, too -- this will lead to a conflict while picking C.

As it should.

Ciao,
Dscho

[*1*] The code in t3410 was not really easy to read, even if there was an 
explanation what it tried to do, but the test code was inconsitent, 
sometimes tagging, sometimes not, sometimes committing with -a, sometimes 
"git add"ing first, yet almost repetitive.

In my endeavor not only to understand it, and either fix my code or the 
code in t3410, I refactored it so that others should have a much easier 
time to understand what it actually does.

Dear list,

Thanks for keeping me on the cc list--several of the later stages of
cruft are my fault, so I don't know that I'll be able to help any more
than commentary on the use cases I was trying to fulfill.

As for the design bug I want to fix: imagine this history:

  ------A
 /     /
/     /
---- B
\     \
 \     \
  C-----D-----E = HEAD

A, C and D touch the same file, and A and D agree on the contents.

Now, rebase -p A does the following at the moment:

  ------A-----E' = HEAD
 /     /
/     /
---- B

In other words, C is truly forgotten, and it is pretended that D never 
happened, either.  That is exactly what test case 2 in t3410 tests for 
[*1*].

This is insane.

Agreed.

Does this mean you're just getting rid of the code that calls "rev list
--cherry-pick"?

If so, I'd be all for that--I did not introduce it, nor fully understand
its nuances, and t3410 was just a hack to get the behavior of a rebase
with a dropped/cherry picked commit from the previous behavior of being
a no-op to instead do "something".

A few times I've pondered just removing the --cherry-pick/drop commit
part of rebase-p, but assumed it was there for a reason.

Also, yeah, don't treat the test cases in t3410 as "the result should be
this exact DAG" but "the result should be something that is not a
noop/sane".

[*1*] The code in t3410 was not really easy to read, even if there was an 
explanation what it tried to do, but the test code was inconsitent, 
sometimes tagging, sometimes not, sometimes committing with -a, sometimes 
"git add"ing first, yet almost repetitive.

In my endeavor not only to understand it, and either fix my code or the 
code in t3410, I refactored it so that others should have a much easier 
time to understand what it actually does.

Thanks for cleaning it up.

I recently saw a test of yours use a `test_commit` bash function that I
really like. My last patch submission debacle had a patch cleaning up
t3411 by introducing `test_commit`--I can brave `git send-email` again
if you have any interest in me resending it.

Thanks,
Stephen

While working on the rebase revamp, I had to fix a few tests (the design
bug I described earlier, and fallout from the new "goto" and "merge"
functions).

These are just the cleanups, they should not change any functionality,
but make everything more readable by providing simple test_commit() and
test_merge() wrappers.

Note: the test_commit() and test_merge() wrappers might be generic enough
to put them into test-lib.sh for a wider audience.

Johannes Schindelin (6):
  t3404 & t3411: undo copy&paste
  lib-rebase.sh: Document what set_fake_editor() does
  lib-rebase.sh: introduce test_commit() and test_merge() helpers
  Simplify t3410
  Simplify t3411
  Simplify t3412

 t/lib-rebase.sh                           |   74 +++++++++++++++++
 t/t3404-rebase-interactive.sh             |   37 +--------
 t/t3410-rebase-preserve-dropped-merges.sh |  126 +++++++++--------------------
 t/t3411-rebase-preserve-around-merges.sh  |  103 +++++-------------------
 t/t3412-rebase-root.sh                    |   30 ++-----
 5 files changed, 145 insertions(+), 225 deletions(-)
 create mode 100644 t/lib-rebase.sh

Rather than copying and pasting, which is prone to lead to fixes
missing in one version, move the fake-editor generator to t/t3404/.

While at it, fix a typo that causes head-scratching: use
${SHELL_PATH-/bin/sh} instead of $SHELL_PATH.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/lib-rebase.sh                          |   36 ++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh            |   37 +++--------------------------
 t/t3411-rebase-preserve-around-merges.sh |   38 +++--------------------------
 3 files changed, 44 insertions(+), 67 deletions(-)
 create mode 100644 t/lib-rebase.sh

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
new file mode 100644
index 0000000..8c8caab
--- /dev/null
+++ b/t/lib-rebase.sh

@@ -0,0 +1,36 @@
+#!/bin/sh
+
+set_fake_editor () {
+	echo "#!${SHELL_PATH-/bin_sh}" >fake-editor.sh
+	cat >> fake-editor.sh <<\EOF
+case "$1" in
+*/COMMIT_EDITMSG)
+	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
+	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
+	exit
+	;;
+esac
+test -z "$EXPECT_COUNT" ||
+	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
+	exit
+test -z "$FAKE_LINES" && exit
+grep -v '^#' < "$1" > "$1".tmp
+rm -f "$1"
+cat "$1".tmp
+action=pick
+for line in $FAKE_LINES; do
+	case $line in
+	squash|edit)
+		action="$line";;
+	*)
+		echo sed -n "${line}s/^pick/$action/p"
+		sed -n "${line}p" < "$1".tmp
+		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
+		action=pick;;
+	esac
+done
+EOF
+
+	test_set_editor "$(pwd)/fake-editor.sh"
+	chmod a+x fake-editor.sh
+}

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 2cc8e7a..3592403 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -10,6 +10,10 @@ that the result still makes sense.
 '
 . ./test-lib.sh
 
+. ../lib-rebase.sh
+
+set_fake_editor
+
 # set up two branches like this:
 #
 # A - B - C - D - E

@@ -61,39 +65,6 @@ test_expect_success 'setup' '
 	git tag I
 '
 
-echo "#!$SHELL_PATH" >fake-editor.sh
-cat >> fake-editor.sh <<\EOF
-case "$1" in
-*/COMMIT_EDITMSG)
-	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
-	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
-	exit
-	;;
-esac
-test -z "$EXPECT_COUNT" ||
-	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
-	exit
-test -z "$FAKE_LINES" && exit
-grep -v '^#' < "$1" > "$1".tmp
-rm -f "$1"
-cat "$1".tmp
-action=pick
-for line in $FAKE_LINES; do
-	case $line in
-	squash|edit)
-		action="$line";;
-	*)
-		echo sed -n "${line}s/^pick/$action/p"
-		sed -n "${line}p" < "$1".tmp
-		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
-		action=pick;;
-	esac
-done
-EOF
-
-test_set_editor "$(pwd)/fake-editor.sh"
-chmod a+x fake-editor.sh
-
 test_expect_success 'no changes are a nop' '
 	git rebase -i F &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&

diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
index aacfaae..6a1586a 100755
--- a/t/t3411-rebase-preserve-around-merges.sh
+++ b/t/t3411-rebase-preserve-around-merges.sh

@@ -5,44 +5,14 @@
 
 test_description='git rebase preserve merges
 
-This test runs git rebase with and tries to squash a commit from after a merge
-to before the merge.
+This test runs git rebase with -p and tries to squash a commit from after
+a merge to before the merge.
 '
 . ./test-lib.sh
 
-# Copy/paste from t3404-rebase-interactive.sh
-echo "#!$SHELL_PATH" >fake-editor.sh
-cat >> fake-editor.sh <<\EOF
-case "$1" in
-*/COMMIT_EDITMSG)
-	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
-	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
-	exit
-	;;
-esac
-test -z "$EXPECT_COUNT" ||
-	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
-	exit
-test -z "$FAKE_LINES" && exit
-grep -v '^#' < "$1" > "$1".tmp
-rm -f "$1"
-cat "$1".tmp
-action=pick
-for line in $FAKE_LINES; do
-	case $line in
-	squash|edit)
-		action="$line";;
-	*)
-		echo sed -n "${line}s/^pick/$action/p"
-		sed -n "${line}p" < "$1".tmp
-		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
-		action=pick;;
-	esac
-done
-EOF
+. ../lib-rebase.sh
 
-test_set_editor "$(pwd)/fake-editor.sh"
-chmod a+x fake-editor.sh
+set_fake_editor
 
 # set up two branches like this:
 #

-- 
1.6.1.482.g7d54be

Often we just need to add a commit with a given (short) name, that will
be tagged with the same name.  Now, relatively complicated graphs can be
constructed easily and in a clear fashion:

	test_commit A &&
	test_commit B &&
	git checkout A &&
	test_commit C &&
	test_merge D B

will construct this graph:

	A - B
	  \   \
	    C - D

For simplicity, files of the same name (but in lower case, to avoid
a warning about ambiguous names) will be committed, with the commit
message as contents.

If you need to provide a different file/different contents, you can use
the more explicit form

	test_commit $MESSAGE $FILENAME $CONTENTS

Signed-off-by: Johannes Schindelin <redacted>
---

	This may want to live in test-lib.sh instead.

 t/lib-rebase.sh |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index cda7778..37430f3 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh

@@ -46,3 +46,29 @@ EOF
 	test_set_editor "$(pwd)/fake-editor.sh"
 	chmod a+x fake-editor.sh
 }
+
+# Call test_commit with the arguments "<message> [<file> [<contents>]]"
+#
+# This will commit a file with the given contents and the given commit
+# message.  It will also add a tag with <message> as name.
+#
+# Both <file> and <contents> default to <message>.
+
+test_commit () {
+	file=$2
+	test -z "$2" && file=$(echo "$1" | tr 'A-Z' 'a-z')
+	echo ${3-$1} > $file &&
+	git add $file &&
+	test_tick &&
+	git commit -m $1 &&
+	git tag $1
+}
+
+# Call test_merge with the arguments "<message> <commit>", where <commit>
+# can be a tag pointing to the commit-to-merge.
+
+test_merge () {
+	test_tick &&
+	git merge -m $1 $2 &&
+	git tag $1
+}

-- 
1.6.1.482.g7d54be

rnyn
Make it easy for other authors to use rebase tests' fake-editor.

Signed-off-by: Johannes Schindelin <redacted>
---

	Separated from 1/6 to make the code move more obvious.

 t/lib-rebase.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 8c8caab..cda7778 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh

@@ -1,5 +1,17 @@
 #!/bin/sh
 
+# After setting the fake editor with this function, you can
+#
+# - override the commit message with $FAKE_COMMIT_MESSAGE,
+# - amend the commit message with $FAKE_COMMIT_AMEND
+# - check that non-commit messages have a certain line count with $EXPECT_COUNT
+# - rewrite a rebase -i script with $FAKE_LINES in the form
+#
+#	"[<lineno1>] [<lineno2>]..."
+#
+#   If a line number is prefixed with "squash" or "edit", the respective line's
+#   command will be replaced with the specified one.
+
 set_fake_editor () {
 	echo "#!${SHELL_PATH-/bin_sh}" >fake-editor.sh
 	cat >> fake-editor.sh <<\EOF

-- 
1.6.1.482.g7d54be

Use test_commit() and test_merge(), reducing the code while making the
intent clearer.

Signed-off-by: Johannes Schindelin <redacted>
---

	Stephen, this and the next one touches your code.

 t/t3410-rebase-preserve-dropped-merges.sh |  126 +++++++++--------------------
 1 files changed, 37 insertions(+), 89 deletions(-)

diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
index 5816415..0669b48 100755
--- a/t/t3410-rebase-preserve-dropped-merges.sh
+++ b/t/t3410-rebase-preserve-dropped-merges.sh

@@ -11,6 +11,8 @@ rewritten.
 '
 . ./test-lib.sh
 
+. ../lib-rebase.sh
+
 # set up two branches like this:
 #
 # A - B - C - D - E

@@ -22,47 +24,17 @@ rewritten.
 # where B, D and G touch the same file.
 
 test_expect_success 'setup' '
-	: > file1 &&
-	git add file1 &&
-	test_tick &&
-	git commit -m A &&
-	git tag A &&
-	echo 1 > file1 &&
-	test_tick &&
-	git commit -m B file1 &&
-	: > file2 &&
-	git add file2 &&
-	test_tick &&
-	git commit -m C &&
-	echo 2 > file1 &&
-	test_tick &&
-	git commit -m D file1 &&
-	: > file3 &&
-	git add file3 &&
-	test_tick &&
-	git commit -m E &&
-	git tag E &&
-	git checkout -b branch1 A &&
-	: > file4 &&
-	git add file4 &&
-	test_tick &&
-	git commit -m F &&
-	git tag F &&
-	echo 3 > file1 &&
-	test_tick &&
-	git commit -m G file1 &&
-	git tag G &&
-	: > file5 &&
-	git add file5 &&
-	test_tick &&
-	git commit -m H &&
-	git tag H &&
-	git checkout -b branch2 F &&
-	: > file6 &&
-	git add file6 &&
-	test_tick &&
-	git commit -m I &&
-	git tag I
+	test_commit A file1 &&
+	test_commit B file1 1 &&
+	test_commit C file2 &&
+	test_commit D file1 2 &&
+	test_commit E file3 &&
+	git checkout A &&
+	test_commit F file4 &&
+	test_commit G file1 3 &&
+	test_commit H file5 &&
+	git checkout F &&
+	test_commit I file6
 '
 
 # A - B - C - D - E

@@ -72,68 +44,44 @@ test_expect_success 'setup' '
 #         I -- G2 -- J -- K           I -- K
 # G2 = same changes as G
 test_expect_success 'skip same-resolution merges with -p' '
-	git checkout branch1 &&
+	git checkout H &&
 	! git merge E &&
-	echo 23 > file1 &&
-	git add file1 &&
-	git commit -m L &&
-	git checkout branch2 &&
-	echo 3 > file1 &&
-	git commit -a -m G2 &&
+	test_commit L file1 23 &&
+	git checkout I &&
+	test_commit G2 file1 3 &&
 	! git merge E &&
-	echo 23 > file1 &&
-	git add file1 &&
-	git commit -m J &&
-	echo file7 > file7 &&
-	git add file7 &&
-	git commit -m K &&
-	GIT_EDITOR=: git rebase -i -p branch1 &&
-	test $(git rev-parse branch2^^) = $(git rev-parse branch1) &&
+	test_commit J file1 23 &&
+	test_commit K file7 file7 &&
+	git rebase -i -p L &&
+	test $(git rev-parse HEAD^^) = $(git rev-parse L) &&
 	test "23" = "$(cat file1)" &&
-	test "" = "$(cat file6)" &&
-	test "file7" = "$(cat file7)" &&
-
-	git checkout branch1 &&
-	git reset --hard H &&
-	git checkout branch2 &&
-	git reset --hard I
+	test "I" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)"
 '
 
 # A - B - C - D - E
 #   \             \ \
-#     F - G - H -- L \        -->   L
-#       \            |               \
-#         I -- G2 -- J -- K           I -- G2 -- K
+#     F - G - H -- L2 \        -->   L2
+#       \             |                \
+#         I -- G3 --- J2 -- K2           I -- G3 -- K2
 # G2 = different changes as G
 test_expect_success 'keep different-resolution merges with -p' '
-	git checkout branch1 &&
+	git checkout H &&
 	! git merge E &&
-	echo 23 > file1 &&
-	git add file1 &&
-	git commit -m L &&
-	git checkout branch2 &&
-	echo 4 > file1 &&
-	git commit -a -m G2 &&
+	test_commit L2 file1 23 &&
+	git checkout I &&
+	test_commit G3 file1 4 &&
 	! git merge E &&
-	echo 24 > file1 &&
-	git add file1 &&
-	git commit -m J &&
-	echo file7 > file7 &&
-	git add file7 &&
-	git commit -m K &&
-	! GIT_EDITOR=: git rebase -i -p branch1 &&
+	test_commit J2 file1 24 &&
+	test_commit K2 file7 file7 &&
+	test_must_fail git rebase -i -p L2 &&
 	echo 234 > file1 &&
 	git add file1 &&
-	GIT_EDITOR=: git rebase --continue &&
-	test $(git rev-parse branch2^^^) = $(git rev-parse branch1) &&
+	git rebase --continue &&
+	test $(git rev-parse HEAD^^^) = $(git rev-parse L2) &&
 	test "234" = "$(cat file1)" &&
-	test "" = "$(cat file6)" &&
-	test "file7" = "$(cat file7)" &&
-
-	git checkout branch1 &&
-	git reset --hard H &&
-	git checkout branch2 &&
-	git reset --hard I
+	test "I" = "$(cat file6)" &&
+	test "file7" = "$(cat file7)"
 '
 
 test_done

-- 
1.6.1.482.g7d54be

Use test_commit() and test_merge().  This way, it is harder to forget to
tag, or to call test_tick before committing.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t3411-rebase-preserve-around-merges.sh |   65 ++++++++----------------------
 1 files changed, 17 insertions(+), 48 deletions(-)

diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
index 6a1586a..6533505 100755
--- a/t/t3411-rebase-preserve-around-merges.sh
+++ b/t/t3411-rebase-preserve-around-merges.sh

@@ -21,27 +21,13 @@ set_fake_editor
 #        -- C1 --
 
 test_expect_success 'setup' '
-	touch a &&
-	touch b &&
-	git add a &&
-	git commit -m A1 &&
-	git tag A1
-	git add b &&
-	git commit -m B1 &&
-	git tag B1 &&
-	git checkout -b branch &&
-	touch c &&
-	git add c &&
-	git commit -m C1 &&
-	git checkout master &&
-	touch d &&
-	git add d &&
-	git commit -m D1 &&
-	git merge branch &&
-	touch f &&
-	git add f &&
-	git commit -m F1 &&
-	git tag F1
+	test_commit A1 &&
+	test_commit B1 &&
+	test_commit C1 &&
+	git reset --hard B1 &&
+	test_commit D1 &&
+	test_merge E1 C1 &&
+	test_commit F1
 '
 
 # Should result in:

@@ -52,7 +38,7 @@ test_expect_success 'setup' '
 #
 test_expect_success 'squash F1 into D1' '
 	FAKE_LINES="1 squash 3 2" git rebase -i -p B1 &&
-	test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" &&
+	test "$(git rev-parse HEAD^2)" = "$(git rev-parse C1)" &&
 	test "$(git rev-parse HEAD~2)" = "$(git rev-parse B1)" &&
 	git tag E2
 '

@@ -70,32 +56,15 @@ test_expect_success 'squash F1 into D1' '
 # And rebase G1..M1 onto E2
 
 test_expect_success 'rebase two levels of merge' '
-	git checkout -b branch2 A1 &&
-	touch g &&
-	git add g &&
-	git commit -m G1 &&
-	git checkout -b branch3 &&
-	touch h
-	git add h &&
-	git commit -m H1 &&
-	git checkout -b branch4 &&
-	touch i &&
-	git add i &&
-	git commit -m I1 &&
-	git tag I1 &&
-	git checkout branch3 &&
-	touch j &&
-	git add j &&
-	git commit -m J1 &&
-	git merge I1 --no-commit &&
-	git commit -m K1 &&
-	git tag K1 &&
-	git checkout branch2 &&
-	touch l &&
-	git add l &&
-	git commit -m L1 &&
-	git merge K1 --no-commit &&
-	git commit -m M1 &&
+	test_commit G1 &&
+	test_commit H1 &&
+	test_commit I1 &&
+	git checkout -b branch3 H1 &&
+	test_commit J1 &&
+	test_merge K1 I1 &&
+	git checkout -b branch2 G1 &&
+	test_commit L1 &&
+	test_merge M1 K1 &&
 	GIT_EDITOR=: git rebase -i -p E2 &&
 	test "$(git rev-parse HEAD~3)" = "$(git rev-parse E2)" &&
 	test "$(git rev-parse HEAD~2)" = "$(git rev-parse HEAD^2^2~2)" &&

-- 
1.6.1.482.g7d54be

Use the newly introduced test_commit() and test_merge() helpers.

Signed-off-by: Johannes Schindelin <redacted>
---

	Thomas, this touches your code.

 t/t3412-rebase-root.sh |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 6359580..39f7768 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh

@@ -6,24 +6,16 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 '
 . ./test-lib.sh
 
+. ../lib-rebase.sh
+
 test_expect_success 'prepare repository' '
-	echo 1 > A &&
-	git add A &&
-	git commit -m 1 &&
-	echo 2 > A &&
-	git add A &&
-	git commit -m 2 &&
+	test_commit 1 A &&
+	test_commit 2 A &&
 	git symbolic-ref HEAD refs/heads/other &&
 	rm .git/index &&
-	echo 3 > B &&
-	git add B &&
-	git commit -m 3 &&
-	echo 1 > A &&
-	git add A &&
-	git commit -m 1b &&
-	echo 4 > B &&
-	git add B &&
-	git commit -m 4
+	test_commit 3 B &&
+	test_commit 1b A 1 &&
+	test_commit 4 B
 '
 
 test_expect_success 'rebase --root expects --onto' '

@@ -103,9 +95,7 @@ test_expect_success 'pre-rebase got correct input (5)' '
 test_expect_success 'set up merge history' '
 	git checkout other^ &&
 	git checkout -b side &&
-	echo 5 > C &&
-	git add C &&
-	git commit -m 5 &&
+	test_commit 5 C &&
 	git checkout other &&
 	git merge side
 '

@@ -132,9 +122,7 @@ test_expect_success 'set up second root and merge' '
 	git symbolic-ref HEAD refs/heads/third &&
 	rm .git/index &&
 	rm A B C &&
-	echo 6 > D &&
-	git add D &&
-	git commit -m 6 &&
+	test_commit 6 D &&
 	git checkout other &&
 	git merge third
 '

-- 
1.6.1.482.g7d54be

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:

quoted

As for the design bug I want to fix: imagine this history:

  ------A
 /     /
/     /
---- B
\     \
 \     \
  C-----D-----E = HEAD

A, C and D touch the same file, and A and D agree on the contents.

Now, rebase -p A does the following at the moment:

  ------A-----E' = HEAD
 /     /
/     /
---- B

In other words, C is truly forgotten, and it is pretended that D never 
happened, either.  That is exactly what test case 2 in t3410 tests for 
[*1*].

This is insane.

Agreed.

Good!  I already feared that you would be disagreeing with me.

Does this mean you're just getting rid of the code that calls "rev list 
--cherry-pick"?

Not exactly.  The idea of rebasing is to stay on top of an upstream.  If 
that upstream has your changes already, you do not want to reapply them -- 
even with --preserve-merges.

Now, a merge cannot be sent as a patch mail, for good reasons.  So 
whatever merge might look like yours, it is not.  So it is your 
responsibility to say that yours is obsolete, and delete it from the 
rebase script.

If your merge is in upstream (because a pull-request was heeded, for 
example), then you will not see the commits anyway.

A few times I've pondered just removing the --cherry-pick/drop commit 
part of rebase-p, but assumed it was there for a reason.

I will find the "dropped" commits using git log -p | git patch-id.

It is still nice to tell the user if she wants to merge a parent that is 
already in upstream, so I would not like to miss out on that information.

quoted

[*1*] The code in t3410 was not really easy to read, even if there was 
an explanation what it tried to do, but the test code was inconsitent, 
sometimes tagging, sometimes not, sometimes committing with -a, 
sometimes "git add"ing first, yet almost repetitive.

In my endeavor not only to understand it, and either fix my code or 
the code in t3410, I refactored it so that others should have a much 
easier time to understand what it actually does.

Thanks for cleaning it up.

I recently saw a test of yours use a `test_commit` bash function that I 
really like. My last patch submission debacle had a patch cleaning up 
t3411 by introducing `test_commit`--I can brave `git send-email` again 
if you have any interest in me resending it.

Heh... so I sent that part of the patches.  Hopefully they will get in 
soon, as they should be rather obvious, and I have a lot more to come...

Ciao,
Dscho

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:

Dscho wroteÖ

quoted

As for the design bug I want to fix: imagine this history:

  ------A
 /     /
/     /
---- B
\     \
 \     \
  C-----D-----E = HEAD

A, C and D touch the same file, and A and D agree on the contents.

Now, rebase -p A does the following at the moment:

  ------A-----E' = HEAD
 /     /
/     /
---- B

In other words, C is truly forgotten, and it is pretended that D never 
happened, either.  That is exactly what test case 2 in t3410 tests for 
[*1*].

This is insane.

Agreed.

Actually, I misread t3410 a great deal.  The situation is as follows:

    ... UPSTREAM
           \
... A - B - C -D

A is a patch the upstream does not have, B is a patch UPSTREAM has,
and "git diff C^!" (i.e. the diff of C to its first parent) is _also_ 
identical to a diff of a merge that is in UPSTREAM.

Basically, t3410 tests that after "git rebase -i -p UPSTREAM" and leaving 
the rebase script as-is, essentially, A and D are cherry-picked on top of 
UPSTREAM.

Does this mean you're just getting rid of the code that calls "rev list 
--cherry-pick"?

Only now do I understand.

I misread the code for --cherry-pick.  For merges, it adds the diff to the 
first parent!

I do not know if it really is desirable to have --cherry-pick handle 
merges at all; I tend to think it is not.

Unfortunately, a short blame session just points to 9c6efa36 done by a 
sloppy programmer: yours truly.

So I adapted my code to find the "dropped" merges in 
git-rebase--interactive, too, for now, but I guess the proper fix is 
something like this:

-- snipsnap --
[PATCH] --cherry-pick: do not skip merges, ever

Currently, --cherry-pick has no problem getting a patch id for merge 
commits: it calculated as the patch id of the patch between the first 
parent and the merge commit.

Of course, this is bogus, as it completely misses the fact that the
merge commit has other parents, too, and therefore a single patch id
would be wrong.

Signed-off-by: Johannes Schindelin <redacted>
---
 patch-ids.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..808a7f0 100644
--- a/patch-ids.c
+++ b/patch-ids.c

@@ -6,9 +6,12 @@
 static int commit_patch_id(struct commit *commit, struct diff_options *options,
 		    unsigned char *sha1)
 {
-	if (commit->parents)
+	if (commit->parents) {
+		if (commit->parents->next)
+			return 0; /* merges do not have a patch id */
 		diff_tree_sha1(commit->parents->item->object.sha1,
 		               commit->object.sha1, "", options);
+	}
 	else
 		diff_root_tree_sha1(commit->object.sha1, "", options);
 	diffcore_std(options);

Actually, I misread t3410 a great deal.  The situation is as follows:

    ... UPSTREAM
           \
... A - B - C -D

A is a patch the upstream does not have, B is a patch UPSTREAM has,
and "git diff C^!" (i.e. the diff of C to its first parent) is _also_ 
identical to a diff of a merge that is in UPSTREAM.

Basically, t3410 tests that after "git rebase -i -p UPSTREAM" and leaving 
the rebase script as-is, essentially, A and D are cherry-picked on top of 
UPSTREAM.

Cool--I "knew" that, but could not have articulated the case as
succinctly.

quoted

Does this mean you're just getting rid of the code that calls "rev list 
--cherry-pick"?

Only now do I understand.

I misread the code for --cherry-pick.  For merges, it adds the diff to the 
first parent!

Ah, so that is how --cherry-pick works--I'd never looked into the
patch-id stuff before. Makes sense, both of how it is leveraged by
rev-list --cherry-pick and also that it doesn't make sense to only be
against the first parent of merges.

So I adapted my code to find the "dropped" merges in
git-rebase--interactive, too, for now, but I guess the proper fix is
something like this:

So, if C, as a merge commit, doesn't get a patch id anymore (right?),
does that mean that C is included with A and D in the cherry-picking
on top of UPSTREAM (because with no patch id it cannot be recognized
as a duplicate)? So then C' is an empty-commit? This would be fine, I
think, or can you detect that C is a noop somehow without patch ids?

Thanks,
Stephen

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:

quoted

So I adapted my code to find the "dropped" merges in 
git-rebase--interactive, too, for now, but I guess the proper fix is 
something like this:

So, if C, as a merge commit, doesn't get a patch id anymore (right?),
does that mean that C is included with A and D in the cherry-picking
on top of UPSTREAM (because with no patch id it cannot be recognized
as a duplicate)?

Yep, it gets into the list.  But not with a "pick" command, as a merge it 
will get a "merge" command.

So then C' is an empty-commit? This would be fine, I think, or can you 
detect that C is a noop somehow without patch ids?

Actually, there are three possible outcomes:

- it tries to merge an ancestor of HEAD or HEAD itself -> noop

- it tries to merge which results in a fast-forward -> fine

- it tries to merge and a proper merge is necessary -> may conflict

Ciao,
Dscho

quoted

quoted

So I adapted my code to find the "dropped" merges in 
git-rebase--interactive, too, for now, but I guess the proper fix is 
something like this:

So, if C, as a merge commit, doesn't get a patch id anymore (right?),
does that mean that C is included with A and D in the cherry-picking
on top of UPSTREAM (because with no patch id it cannot be recognized
as a duplicate)?

Yep, it gets into the list.  But not with a "pick" command, as a merge it 
will get a "merge" command.

quoted

So then C' is an empty-commit? This would be fine, I think, or can you 
detect that C is a noop somehow without patch ids?

Actually, there are three possible outcomes:

- it tries to merge an ancestor of HEAD or HEAD itself -> noop

- it tries to merge which results in a fast-forward -> fine

- it tries to merge and a proper merge is necessary -> may conflict

Ah, cool, that makes sense.

Thanks,
Stephen