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

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

[Johannes Schindelin]

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.

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

[Stephen Haberman]

> 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

[PATCH 0/6] Simplifications of some 'rebase' tests

[Johannes Schindelin]

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

[PATCH 1/6] t3404 & t3411: undo copy&paste

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH 2/6] lib-rebase.sh: Document what set_fake_editor() does

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	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

[PATCH 3/6] lib-rebase.sh: introduce test_commit() and test_merge() helpers

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	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

[PATCH 4/6] Simplify t3410

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	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

[PATCH 5/6] Simplify t3411

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH 6/6] Simplify t3412

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	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

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

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:

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

> > [*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

Re: [PATCH 1/6] t3404 & t3411: undo copy&paste

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

I've learned to be cautious whenever I see "while at it".

> 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

It is unclear why you would want to do this.  It was unclear what "typo"
you were referring to in your commit log message, either.

The tests are supposed to run under the shell the user specified, so if
there is a case you found that $SHELL_PATH is unset, that is a bug we
would want to fix, and ${SHELL_PATH-/bin/sh} is sweeping the problem under
the rug to make it harder to fix, isn't it?

I would understand if it were

	${SHELL_PATH?"SHELL_PATH Not Set --- bug in tests?"}

though.

Besides, it's /bin/sh, not /bin_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"

I looked at the output from this and wondered what these "sed -n" shown in
the "-v" output were about last night.  I do think it is a good idea to
show what edit was done to the insn stream, but I suspect it may be easier
to read the output if you did this instead:

> +		sed -n "${line}p" < "$1".tmp
> +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> +		sed -n "${line}s/^pick/$action/p" < "$1".tmp

Re: [PATCH 2/6] lib-rebase.sh: Document what set_fake_editor() does

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> rnyn
> Make it easy for other authors to use rebase tests' fake-editor.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Perhaps a very welcome addition, except that I did not find rnyn in my
dictionary, and the patch textually depends on /bin_sh bug ;-)

Re: [PATCH 3/6] lib-rebase.sh: introduce test_commit() and test_merge() helpers

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Yeah, I tend to agree.

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

	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z')}

might be more consistent with this:

> +	echo ${3-$1} > $file &&

and may be easier to read.

I'd suggest dquoting argument to echo above, i.e. "${3-$1}", and all the
references to positional arguments in the remainder of the patch, though.

> +	git add $file &&

as well as "$file" here.

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

Re: [PATCH 1/6] t3404 & t3411: undo copy&paste

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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.
> 
> I've learned to be cautious whenever I see "while at it".

Heh.

> > 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
> 
> It is unclear why you would want to do this.  It was unclear what "typo"
> you were referring to in your commit log message, either.
> 
> The tests are supposed to run under the shell the user specified, so if
> there is a case you found that $SHELL_PATH is unset, that is a bug we
> would want to fix, and ${SHELL_PATH-/bin/sh} is sweeping the problem under
> the rug to make it harder to fix, isn't it?

I call the scripts directly, and I do not think it would be a good idea to 
force the user to use GIT_TEST_OPTS and make when calling the script 
directly is so much easier.  Plus, this way I can pass "sh -x $SCRIPT" 
easily.

I am really puzzled that it works, BTW.  With an empty SHELL_PATH, 
apparently.

> Besides, it's /bin/sh, not /bin_sh ;-)

Right.  The commit message was right, at least!

> > +	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"
> 
> I looked at the output from this and wondered what these "sed -n" shown 
> in the "-v" output were about last night.  I do think it is a good idea 
> to show what edit was done to the insn stream, but I suspect it may be 
> easier to read the output if you did this instead:
> 
> > +		sed -n "${line}p" < "$1".tmp
> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp


Probably.  It is for debugging, anyway.  As everything you only see with 
-v.

Ciao,
Dscho

Re: [PATCH 2/6] lib-rebase.sh: Document what set_fake_editor() does

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > rnyn
> > Make it easy for other authors to use rebase tests' fake-editor.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Perhaps a very welcome addition, except that I did not find rnyn in my
> dictionary, and the patch textually depends on /bin_sh bug ;-)

Oh, that? It is perfectly *snarf* normal.  Just my *pucker* Tourette 
syndrome kicking in.

Ciao,
Dscho

[PATCH v2 0/6] rebase simplifications

[Johannes Schindelin]

Changes vs  v1:

removed the "rnyn" blurt (which probably marsk me as Alpine user...)

removed the SHELL_PATH handling; it is a miracle to me why it works, but 
I'd rather not meddle with the magic now that you pointed it out

Moved test_commit and test_merge into test-lib.sh

Fixed the quoting in test_commit and test_merge

AFAIR that's all...

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

 t/README                                  |   18 ++++
 t/lib-rebase.sh                           |   48 +++++++++++
 t/t3404-rebase-interactive.sh             |   37 +--------
 t/t3410-rebase-preserve-dropped-merges.sh |  124 ++++++++---------------------
 t/t3411-rebase-preserve-around-merges.sh  |  103 +++++-------------------
 t/t3412-rebase-root.sh                    |   28 ++-----
 t/test-lib.sh                             |   26 ++++++
 7 files changed, 159 insertions(+), 225 deletions(-)
 create mode 100644 t/lib-rebase.sh

[PATCH v2 1/6] t3404 & t3411: undo copy&paste

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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..762ffcf
--- /dev/null
+++ b/t/lib-rebase.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+set_fake_editor () {
+	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
+}
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

[PATCH v2 2/6] lib-rebase.sh: Document what set_fake_editor() does

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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 762ffcf..260a231 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" >fake-editor.sh
 	cat >> fake-editor.sh <<\EOF
-- 
1.6.1.482.g7d54be

[PATCH v2 3/6] test-lib.sh: introduce test_commit() and test_merge() helpers

[Johannes Schindelin]

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 whose name is the lower case version of the commit
message (to avoid a warning about ambiguous names) will be committed, with
the corresponding commit messages 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 <johannes.schindelin@gmx.de>
---
 t/README      |   18 ++++++++++++++++++
 t/test-lib.sh |   25 +++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 8f12d48..f208cf1 100644
--- a/t/README
+++ b/t/README
@@ -212,6 +212,24 @@ library for your script to use.
    is to summarize successes and failures in the test script and
    exit with an appropriate error code.
 
+ - test_tick
+
+   Make commit and tag names consistent by setting the author and
+   committer times to defined stated.  Subsequent calls will
+   advance the times by a fixed amount.
+
+ - test_commit <message> [<filename> [<contents>]]
+
+   Creates a commit with the given message, committing the given
+   file with the given contents (default for both is to reuse the
+   message string), and adds a tag (again reusing the message
+   string as name).  Calls test_tick to make the SHA-1s
+   reproducible.
+
+ - test_merge <message> <commit-or-tag>
+
+   Merges the given rev using the given message.  Like test_commit,
+   creates a tag and calls test_tick before committing.
 
 Tips for Writing Tests
 ----------------------
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 41d5a59..c1839f7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -193,6 +193,31 @@ test_tick () {
 	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
 }
 
+# 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:-$(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"
+}
+
 # You are not expected to call test_ok_ and test_failure_ directly, use
 # the text_expect_* functions instead.
 
-- 
1.6.1.482.g7d54be

[PATCH v2 4/6] Simplify t3410

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3410-rebase-preserve-dropped-merges.sh |  124 ++++++++---------------------
 1 files changed, 35 insertions(+), 89 deletions(-)

diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
index 5816415..c49143a 100755
--- a/t/t3410-rebase-preserve-dropped-merges.sh
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -22,47 +22,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 +42,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

[PATCH v2 5/6] Simplify t3411

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH v2 6/6] Simplify t3412

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3412-rebase-root.sh |   28 +++++++---------------------
 1 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 3d8ff67..9fc528f 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,23 +7,13 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 . ./test-lib.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 +93,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 +120,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

Re: [PATCH 1/6] t3404 & t3411: undo copy&paste

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > +		sed -n "${line}p" < "$1".tmp
>> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
>> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp
>
>
> Probably.  It is for debugging, anyway.  As everything you only see with 
> -v.

Exactly.  That is why I'd rather want to see what exact insn sequence is
being fed to the "rebase -i".  Because I'd be debugging my new test or
changes to "rebase -i", not debugging fake-editor's use of sed.

Re: [PATCH v2 0/6] rebase simplifications

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Changes vs  v1:
>
> removed the "rnyn" blurt (which probably marsk me as Alpine user...)
>
> removed the SHELL_PATH handling; it is a miracle to me why it works, but 
> I'd rather not meddle with the magic now that you pointed it out
>
> Moved test_commit and test_merge into test-lib.sh
>
> Fixed the quoting in test_commit and test_merge
>
> AFAIR that's all...

Thanks; looks much nicer (not just relative to v1 but compared to the
original).

Re: [PATCH 1/6] t3404 & t3411: undo copy&paste

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +		sed -n "${line}p" < "$1".tmp
> >> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> >> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp
> >
> > Probably.  It is for debugging, anyway.  As everything you only see with 
> > -v.
> 
> Exactly.  That is why I'd rather want to see what exact insn sequence is 
> being fed to the "rebase -i".  Because I'd be debugging my new test or 
> changes to "rebase -i", not debugging fake-editor's use of sed.

If you are really after seeing the constructed rebase script, then

			tail -n 1 "$1"

would make tons more sense, no?

Ciao,
Dscho

Re: [PATCH v2 0/6] rebase simplifications

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Changes vs  v1:
> >
> > removed the "rnyn" blurt (which probably marsk me as Alpine user...)
> >
> > removed the SHELL_PATH handling; it is a miracle to me why it works, but 
> > I'd rather not meddle with the magic now that you pointed it out
> >
> > Moved test_commit and test_merge into test-lib.sh
> >
> > Fixed the quoting in test_commit and test_merge
> >
> > AFAIR that's all...

Oh, I forgot the ${2:-...} thing...

> Thanks; looks much nicer (not just relative to v1 but compared to the 
> original).

Thanks!

Ciao,
Dscho

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2797 bytes --]

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:
> 
> Dscho wroteÖ
> 
> > 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 <Johannes.Schindelin@gmx.de>
---
 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);

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

[Stephen Haberman]

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

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

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

[Johannes Schindelin]

Hi,

On Tue, 27 Jan 2009, Stephen Haberman wrote:

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

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

[Stephen Haberman]

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

Ah, cool, that makes sense.

Thanks,
Stephen