[PATCH 0/5] rebase: calculate patches in upstream correctly

[PATCH 0/5] rebase: calculate patches in upstream correctly

[Martin von Zweigbergk]

I worked on these patches last year but never sent them because I
wasn't sure how they impact performance. I was hoping that getting the
patch body from the commit and avoiding the call to mailinfo would
make am-based rebase a little faster, but if I remember correctly, it
got a little slower instead (not by much, though). I was also working
on a way to implement the 'git cherry' functionality in 'git
rev-list', but I wasn't sure how that would work and I ran out of
time. I hope at least some of these patches can still be useful.

Martin von Zweigbergk (5):
  rebase: don't source git-sh-setup twice
  rebase --root: print usage on too many args
  am --rebasing: get patch body from commit, not from mailbox
  am: don't call mailinfo if $rebasing
  rebase [-m]: calculate patches in upstream correctly

 git-am.sh                   |   48 ++++++++++++++++++++++---------------------
 git-rebase--am.sh           |    8 ++------
 git-rebase--interactive.sh  |    4 +---
 git-rebase--merge.sh        |    4 +---
 git-rebase.sh               |   13 ++++++------
 t/t3401-rebase-partial.sh   |   17 +++++++++++++++
 t/t3405-rebase-malformed.sh |   32 +++++++++++++++++++++++++----
 t/t3406-rebase-message.sh   |   14 ++++++-------
 t/t3412-rebase-root.sh      |    8 +++++++-
 9 files changed, 94 insertions(+), 54 deletions(-)

-- 
1.7.9.3.327.g2980b

[PATCH 1/5] rebase: don't source git-sh-setup twice

[Martin von Zweigbergk]

The git-sh-setup script is already sourced in git-rebase.sh before
calling into git-rebase--(am|interactive|merge).sh. There are no other
callers of these scripts. It is therefore unnecessary to source
git-sh-setup again in them.
---
 git-rebase--am.sh          |    2 --
 git-rebase--interactive.sh |    4 +---
 git-rebase--merge.sh       |    2 --
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 04d8941..392ebc9 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -3,8 +3,6 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
-. git-sh-setup
-
 case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" &&
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..a5b018d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -9,9 +9,7 @@
 #
 # The original idea comes from Eric W. Biederman, in
 # http://article.gmane.org/gmane.comp.version-control.git/22407
-
-. git-sh-setup
-
+#
 # The file containing rebase commands, comments, and empty lines.
 # This file is created by "git rebase -i" then edited by the user.  As
 # the lines are processed, they are removed from the front of this
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index dc59907..b10f2cf 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -3,8 +3,6 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
-. git-sh-setup
-
 prec=4
 
 read_state () {
-- 
1.7.9.3.327.g2980b

[PATCH 2/5] rebase --root: print usage on too many args

[Martin von Zweigbergk]

Just like

  git rebase --onto newbase upstream branch error

displays the usage message, so should clearly

  git rebase --onto newbase --root branch error

, but it doesn't. Instead, it ignores both "branch" and "error" and
rebases the current HEAD. This is because we try to match the number
of remainging arguments "$#", which fails to match "1" argument and
matches the "*" that really should have been a "0".

Make sure we display usage information when too many arguments are
given. Also fail-fast in case of similar bugs in the future by
matching on exactly 0 arguments and failing on unknown numbers.
---
 git-rebase.sh          |    6 +++++-
 t/t3412-rebase-root.sh |    8 +++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index e616737..6df06c4 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -400,6 +400,7 @@ else
 	test -z "$onto" && die "You must specify --onto when using --root"
 	unset upstream_name
 	unset upstream
+	test $# -gt 1 && usage
 	upstream_arg=--root
 fi
 
@@ -450,7 +451,7 @@ case "$#" in
 		die "fatal: no such branch: $1"
 	fi
 	;;
-*)
+0)
 	# Do not need to switch branches, we are already on it.
 	if branch_name=`git symbolic-ref -q HEAD`
 	then
@@ -462,6 +463,9 @@ case "$#" in
 	fi
 	orig_head=$(git rev-parse --verify "${branch_name}^0") || exit
 	;;
+*)
+	die "BUG: unexpected number of arguments left to parse"
+	;;
 esac
 
 require_clean_work_tree "rebase" "Please commit or stash them."
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 086c91c..1e9d1a7 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -23,9 +23,15 @@ test_expect_success 'prepare repository' '
 '
 
 test_expect_success 'rebase --root expects --onto' '
+	git checkout -B fail other &&
 	test_must_fail git rebase --root
 '
 
+test_expect_success 'rebase --root fails with too many args' '
+	git checkout -B fail other &&
+	test_must_fail git rebase --onto master --root fail fail
+'
+
 test_expect_success 'setup pre-rebase hook' '
 	mkdir -p .git/hooks &&
 	cat >.git/hooks/pre-rebase <<EOF &&
@@ -42,7 +48,7 @@ cat > expect <<EOF
 EOF
 
 test_expect_success 'rebase --root --onto <newbase>' '
-	git checkout -b work &&
+	git checkout -b work other &&
 	git rebase --root --onto master &&
 	git log --pretty=tformat:"%s" > rebased &&
 	test_cmp expect rebased
-- 
1.7.9.3.327.g2980b

[PATCH 3/5] am --rebasing: get patch body from commit, not from mailbox

[Martin von Zweigbergk]

Rebasing a commit that contains a diff in the commit message results
in a failure with output such as

  First, rewinding head to replay your work on top of it...
  Applying: My cool patch.
  fatal: sha1 information is lacking or useless
  (app/controllers/settings_controller.rb).
  Repository lacks necessary blobs to fall back on 3-way merge.
  Cannot fall back to three-way merge.
  Patch failed at 0001 My cool patch.

The reason is that 'git rebase' without -p/-i/-m internally calls 'git
format-patch' and pipes the output to 'git am --rebasing', which has
no way of knowing what is a real patch and what is a commit message
that contains a patch.

Make 'git am' while in --rebasing mode get the patch body from the
commit object instead of extracting it from the mailbox.

Patch by Junio, test case and commit log message by Martin.

Reported-by: anikey <arty.anikey@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 git-am.sh                   |    1 +
 t/t3405-rebase-malformed.sh |   32 ++++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index f8b7a0c..ec8fde1 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -683,6 +683,7 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
 			echo "$commit" > "$dotest/original-commit"
 			get_author_ident_from_commit "$commit" > "$dotest/author-script"
+			git diff-tree --root --binary "$commit" >"$dotest/patch"
 		else
 			{
 				sed -n '/^Subject/ s/Subject: //p' "$dotest/info"
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index e5ad67c..19eddad 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='rebase should not insist on git message convention'
+test_description='rebase should handle arbitrary git message'
 
 . ./test-lib.sh
 
@@ -12,6 +12,11 @@ It has two paragraphs, but its first paragraph is not friendly
 to oneline summary format.
 EOF
 
+cat >G <<\EOF
+commit log message containing a diff
+EOF
+
+
 test_expect_success setup '
 
 	>file1 &&
@@ -19,8 +24,9 @@ test_expect_success setup '
 	git add file1 file2 &&
 	test_tick &&
 	git commit -m "Initial commit" &&
+	git branch diff-in-message
 
-	git checkout -b side &&
+	git checkout -b multi-line-subject &&
 	cat F >file2 &&
 	git add file2 &&
 	test_tick &&
@@ -28,6 +34,17 @@ test_expect_success setup '
 
 	git cat-file commit HEAD | sed -e "1,/^\$/d" >F0 &&
 
+	git checkout diff-in-message &&
+	echo "commit log message containing a diff" >G &&
+	echo "" >>G
+	cat G >file2 &&
+	git add file2 &&
+	git diff --cached >>G &&
+	test_tick &&
+	git commit -F G &&
+
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >G0 &&
+
 	git checkout master &&
 
 	echo One >file1 &&
@@ -36,13 +53,20 @@ test_expect_success setup '
 	git commit -m "Second commit"
 '
 
-test_expect_success rebase '
+test_expect_success 'rebase commit with multi-line subject' '
 
-	git rebase master side &&
+	git rebase master multi-line-subject &&
 	git cat-file commit HEAD | sed -e "1,/^\$/d" >F1 &&
 
 	test_cmp F0 F1 &&
 	test_cmp F F0
 '
 
+test_expect_success 'rebase commit with diff in message' '
+	git rebase master diff-in-message &&
+	git cat-file commit HEAD | sed -e "1,/^$/d" >G1 &&
+	test_cmp G0 G1 &&
+	test_cmp G G0
+'
+
 test_done
-- 
1.7.9.3.327.g2980b

[PATCH 4/5] am: don't call mailinfo if $rebasing

[Martin von Zweigbergk]

Since 5e835ca (rebase: do not munge commit log message, 2008-04-16),
'git am --rebasing' no longer gets the commit log message from the
patch, but reads it from the commit identified by the "From " header
line. From 43c2325 (am: use get_author_ident_from_commit instead of
mailinfo when rebasing, 2010-06-16), it also gets the author name,
email and date from the commit. Now that the final part of the patch
-- the patch body itself -- is also read from the commit, there is no
longer a need to call 'git mailinfo' to extract any of these parts
while --rebasing.

Sugested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 git-am.sh |   47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index ec8fde1..b6a5300 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -260,7 +260,7 @@ check_patch_format () {
 split_patches () {
 	case "$patch_format" in
 	mbox)
-		if test -n "$rebasing" || test t = "$keepcr"
+		if test t = "$keepcr"
 		then
 		    keep_cr=--keep-cr
 		else
@@ -413,7 +413,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
+		rebasing=t threeway=t ;;
 	-d|--dotest)
 		die "$(gettext "-d option is no longer supported.  Do not use.")"
 		;;
@@ -658,33 +658,34 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
-			<"$dotest/$msgnum" >"$dotest/info" ||
-			stop_here $this
-
-		# skip pine's internal folder data
-		sane_grep '^Author: Mail System Internal Data$' \
-			<"$dotest"/info >/dev/null &&
-			go_next && continue
-
-		test -s "$dotest/patch" || {
-			eval_gettextln "Patch is empty.  Was it split wrong?
-If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
-To restore the original branch and stop patching run \"\$cmdline --abort\"."
-			stop_here $this
-		}
-		rm -f "$dotest/original-commit" "$dotest/author-script"
-		if test -f "$dotest/rebasing" &&
+		if test -f "$dotest/rebasing"
+		then
 			commit=$(sed -e 's/^From \([0-9a-f]*\) .*/\1/' \
 				-e q "$dotest/$msgnum") &&
-			test "$(git cat-file -t "$commit")" = commit
-		then
+			test "$(git cat-file -t "$commit")" = commit ||
+				stop_here $this
 			git cat-file commit "$commit" |
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
-			echo "$commit" > "$dotest/original-commit"
-			get_author_ident_from_commit "$commit" > "$dotest/author-script"
+			echo "$commit" >"$dotest/original-commit"
+			get_author_ident_from_commit "$commit" >"$dotest/author-script"
 			git diff-tree --root --binary "$commit" >"$dotest/patch"
 		else
+			git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
+				<"$dotest/$msgnum" >"$dotest/info" ||
+				stop_here $this
+
+			# skip pine's internal folder data
+			sane_grep '^Author: Mail System Internal Data$' \
+				<"$dotest"/info >/dev/null &&
+				go_next && continue
+
+			test -s "$dotest/patch" || {
+				eval_gettextln "Patch is empty.  Was it split wrong?
+If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
+To restore the original branch and stop patching run \"\$cmdline --abort\"."
+				stop_here $this
+			}
+			rm -f "$dotest/original-commit" "$dotest/author-script"
 			{
 				sed -n '/^Subject/ s/Subject: //p' "$dotest/info"
 				echo
-- 
1.7.9.3.327.g2980b

[PATCH 5/5] rebase [-m]: calculate patches in upstream correctly

[Martin von Zweigbergk]

Plain 'git rebase' (without -m/-i/-p) applies the patches from

  git format-patch --ignore-if-in-upstream $upstream..$orig_head

, while 'git rebase -m' finds the commits using

  git rev-list $upstream..$orig_head

As Knut Franke reported in [1], the fact that there is no
--ignore-if-in-upstream or equivalent when using merge-based rebase
means that unnecessary conflicts can arise due to commits
cherry-picked between $orig_head and $upstream.

There is a second problem with the above method of calculating the
upstream commits. Copying the example history from [1]:

      .-c
     /
a---b---d---e---f
         \
          .-g---E

Commit E is here a cherry-pick of e. If we now run 'git rebase --onto
c f E', the revisions that will be applied onto 'c' are given by 'git
format-patch --ignore-if-in-upstream f..E'. In this case that would be
only 'g' and NOT 'E'.

To solve both of the above problems, we want to find the commits in
$upstream..$orig_head that are not cherry-picked in
$upstream..$onto. There is unfortunately no direct way of finding
these commits using 'git rev-list', so we will have to resort to using
'git cherry' and filter for lines starting with '+'.

To reduce the risk of 'git rebase' and 'git rebase -m' behaving
differently (with respect to the commits chosen) in the future,
perform the calculation already in git-rebase.sh.

As a side-effect, we also avoid the cost of formatting patches.

Test case updates for 'rebase -m' by Knut, the rest by Martin.

 [1] http://thread.gmane.org/gmane.comp.version-control.git/161917

Helped-by: Knut Franke <Knut.Franke@gmx.de>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 git-rebase--am.sh         |    6 ++----
 git-rebase--merge.sh      |    2 +-
 git-rebase.sh             |    7 +------
 t/t3401-rebase-partial.sh |   17 +++++++++++++++++
 t/t3406-rebase-message.sh |   14 +++++++-------
 5 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 392ebc9..89e0ab4 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -16,7 +16,6 @@ skip)
 	;;
 esac
 
-test -n "$rebase_root" && root_flag=--root
 
 if test -n "$keep_empty"
 then
@@ -26,9 +25,8 @@ then
 	# makes this easy
 	git cherry-pick --allow-empty "$revisions"
 else
-	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		--src-prefix=a/ --dst-prefix=b/ \
-		--no-renames $root_flag "$revisions" |
+	echo "$revisions" |
+	sed -e 's/\([0-9a-f]\{40\}\)/From \1 Mon Sep 17 00:00:00 2001/' |
 	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"
 fi && move_to_original_branch
 
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index b10f2cf..7ea33e3 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -131,7 +131,7 @@ echo "$onto_name" > "$state_dir/onto_name"
 write_basic_state
 
 msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
+for cmt in $revisions
 do
 	msgnum=$(($msgnum + 1))
 	echo "$cmt" > "$state_dir/cmt.$msgnum"
diff --git a/git-rebase.sh b/git-rebase.sh
index 6df06c4..47e75cb 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -522,11 +522,6 @@ then
 	exit 0
 fi
 
-if test -n "$rebase_root"
-then
-	revisions="$onto..$orig_head"
-else
-	revisions="$upstream..$orig_head"
-fi
+revisions="$(git cherry $onto $orig_head $upstream | sed -ne 's/^+ //p')"
 
 run_specific_rebase
diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index 7ba1797..ce555fa 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -42,4 +42,21 @@ test_expect_success 'rebase --merge topic branch that was partially merged upstr
 	test_path_is_missing .git/rebase-merge
 '
 
+test_expect_success 'rebase --onto does not re-apply patches in $onto' '
+	git checkout C &&
+	test_commit C2 C.t &&
+	git checkout -B my-topic-branch master &&
+	test_commit D &&
+	git rebase --onto C2 A2 &&
+	test "$(git log --format=%s C2..)" = D
+'
+
+test_expect_success 'rebase --onto does not lose patches in $upstream' '
+	git rebase --onto A2 D &&
+	test "$(git log --format=%s A2..)" = "D
+C2
+C
+B"
+'
+
 test_done
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 6898377..3eecc66 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -5,8 +5,10 @@ test_description='messages from rebase operation'
 . ./test-lib.sh
 
 quick_one () {
-	echo "$1" >"file$1" &&
-	git add "file$1" &&
+	fileno=$2
+	test -z "$fileno" && fileno=$1
+	echo "$1" >"file$fileno" &&
+	git add "file$fileno" &&
 	test_tick &&
 	git commit -m "$1"
 }
@@ -16,21 +18,19 @@ test_expect_success setup '
 	git branch topic &&
 	quick_one X &&
 	quick_one A &&
-	quick_one B &&
+	quick_one B A &&
 	quick_one Y &&
 
 	git checkout topic &&
 	quick_one A &&
-	quick_one B &&
+	quick_one B A &&
 	quick_one Z &&
 	git tag start
 
 '
 
 cat >expect <<\EOF
-Already applied: 0001 A
-Already applied: 0002 B
-Committed: 0003 Z
+Committed: 0001 Z
 EOF
 
 test_expect_success 'rebase -m' '
-- 
1.7.9.3.327.g2980b

Re: [PATCH 5/5] rebase [-m]: calculate patches in upstream correctly

[Junio C Hamano]

Hrm, why does this break 9140.6?  Has the test been expecting a
wrong result?

-- >8 --

expecting success: 
        ( cd g &&
          git svn fetch &&
          git svn rebase &&
          fgrep "mod hidden" hid/hid.txt
        )

Index mismatch: a4b5c203f79112e92d530cb82366ca4d706fa4a8 != 6b5a1244749c8cdb5442eefd0abc3689d666322d
rereading 6f980d1f3188db5082d5bfd2f28b317b6e7b8893
        A       hid/hid.txt
r2 = 9c02870abb807d33b84cc36ab2f2384bf01448f8 (refs/remotes/git-svn)
        M       vis/vis.txt
r3 = 26a2abacc1ca26dde5a0b18a73f0b2d306dc95d1 (refs/remotes/git-svn)
        M       hid/hid.txt
r4 = 9299b4c6227b8efcc64ec5e867f0eeae4d1c96ec (refs/remotes/git-svn)
First, rewinding head to replay your work on top of it...
Applying: create initially hidden files
fatal: unrecognized input
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 create initially hidden files

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase
--skip".
To check out the original branch and stop rebasing run "git rebase
--abort".

rebase refs/remotes/git-svn: command returned error: 1

not ok - 6 refetch succeeds not ignoring any files

Re: [PATCH 5/5] rebase [-m]: calculate patches in upstream correctly

[Martin von Zweigbergk]

On Tue, Jun 26, 2012 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Hrm, why does this break 9140.6?  Has the test been expecting a
> wrong result?

I wish I could say that it was. I guess I never had the subversion
libraries installed while running the test cases :-(. It turns out
that that test case happens to create an empty commit. So when I
changed from git-format-patch to git-cherry for calculating the
revisions to rebase, it started including empty commits. It also turns
out we don't have any test cases specifically for rebasing empty
commits, so I'm glad 9140.6 caught it. I'll send out a patch that adds
a test case for rebasing of empty commit, which should be good
regardless of whether the current patch, in some form, is eventually
accepted.

I got all the tests to pass by special-casing empty patches in
git-am.sh when $rebasing, but I'm now unsure about this patch for a
few reasons:

 1) I suppose the $revisions passed to git-am and may result in too
long a command line. This should be fixable by replacing it by a
function, I guess.

 2) The new --keep-empty stuff is clearly closely related to these
changes and at least the 'if test -n "$keep_empty"' block in
git-rebase--am.sh could possibly be replaced by similar logic in
git-am.sh by passing the --keep-empty to it. Using git-am should make
it faster, which it the whole point of using git-am instead of
git-cherry-pick, IIUC.

I'm not sure when I'll get time to work on these issues. If someone
else feels the urge, please go ahead.

Martin