git rebase -i -p broken?

git rebase -i -p broken?

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

Consider this scenario:
- commit some patch (named 'a')
- merge a random branch
- commit a fix to patch a
- since I haven't pushed yet, I want to squash a and a-fix together, to 
prevent bisect problems
- fire up 'git rebase -i -p a^'

Now the problems begin:
- the todo list shows up the branch's commits as well as my current 
branch.  But I don't want to commit the branch's commits in my own 
branch.  Replaying the merge should be enough.  Looks like a missing 
--first-parent somewhere.
- removing the spurious commit from the todo, and moving a-fix after a 
and marking it as a squash action, I get
   Refusing to squash a merge: 51ca22d7afb7433332ae41d0c2e3bab598048c21
  even though that's not a merge
- using git commit --amend instead of squash confuses git in some other way

Attached is a script that generates a test case.  With some $EDITOR 
hacks it can even be convinced to be an automated test case.

All this using 1.6.0.2.

-- 
error compiling committee.c: too many arguments to function


[-- Attachment #2: rebase-merge --]
[-- Type: text/plain, Size: 385 bytes --]

!#/bin/sh -e

git --version
mkdir repo
cd repo
git init
touch a
touch 0
git add 0
git commit -m 'zeroth commit'
git add a
git commit -m 'first commit'
git checkout -b branch
touch b
git add b
git commit -m 'second commit (branch)'
git checkout master
touch c
git add c
git commit -m 'third commit'
git merge branch
touch d
git add d
git commit -m 'fifth commit'
git rebase -i -p HEAD~4

[PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Stephen Haberman]

This commit fixes Avi Kivity's use case of squashing two commits on either side
of a merge together.

Changes include:

- Delaying storing the rewrite information if the current commit we are
  applying is being squashed. This means storing multiple lines in the
  current-commit file and recording each of them as rewritten to the
  same HEAD on the next commit.

- Move the "no squashing merges" check into the case statement for
  merges as previously it was catching catching even single-parent
  squashes.

- Conditionally pass "--first-parent" to `git rev-list` based on whether
  this is a rebase is preserving merges or not.

- At the end, just take the current head for the new branch ref instead
  of trying to look up what the OLDHEAD was rewritten to. This fails
  because the OLDHEAD was squashed/moved up earlier in the timeline, so
  even if we can find its rewritten HEAD, that is no longer what we
  ended up at.

Signed-off-by: Stephen Haberman <stephen@exigencecorp.com>
---

I agree with Avi on what the rebase -i -p behavior should be for his
scenario. This patch makes it so. However, the bane of my existence,
t3404 is failing ~12 tests in, which is a real PITA to debug, so please
let me know if this is a worthwhile tangent to continue on.

(That last change of dropping the OLDHEAD->NEWHEAD guessing is probably
what is causing t3404 to fail, but I can't reason why it'd need to do
that rather than just use HEAD.)

I've read in the archives about the git-sequencer stuff, which sounds
cool, my thought is that, if anything, this will clarify git rebase -i -p
behavior and add tests that can later be ensured to still pass when
git-sequencer is dropped in.

 git-rebase--interactive.sh               |   53 ++++++++++---------
 t/t3411-rebase-preserve-around-merges.sh |   83 ++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 25 deletions(-)
 create mode 100644 t/t3411-rebase-preserve-around-merges.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..9914111 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -159,13 +159,18 @@ pick_one_preserving_merges () {
 
 	if test -f "$DOTEST"/current-commit
 	then
-		current_commit=$(cat "$DOTEST"/current-commit) &&
-		git rev-parse HEAD > "$REWRITTEN"/$current_commit &&
-		rm "$DOTEST"/current-commit ||
-		die "Cannot write current commit's replacement sha1"
+		if [ "$fast_forward" == "t" ]
+		then
+			cat "$DOTEST"/current-commit | while read current_commit
+			do
+				git rev-parse HEAD > "$REWRITTEN"/$current_commit
+			done
+			rm "$DOTEST"/current-commit ||
+			die "Cannot write current commit's replacement sha1"
+		fi
 	fi
 
-	echo $sha1 > "$DOTEST"/current-commit
+	echo $sha1 >> "$DOTEST"/current-commit
 
 	# rewrite parents; if none were rewritten, we can fast-forward.
 	new_parents=
@@ -193,15 +198,19 @@ pick_one_preserving_merges () {
 			die "Cannot fast forward to $sha1"
 		;;
 	f)
-		test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
-
 		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
-		# detach HEAD to current parent
-		output git checkout $first_parent 2> /dev/null ||
-			die "Cannot move HEAD to $first_parent"
+
+		if [ "$1" != "-n" ]
+		then
+			# detach HEAD to current parent
+			output git checkout $first_parent 2> /dev/null ||
+				die "Cannot move HEAD to $first_parent"
+		fi
 
 		case "$new_parents" in
 		' '*' '*)
+			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
+
 			# redo merge
 			author_script=$(get_author_ident_from_commit $sha1)
 			eval "$author_script"
@@ -350,20 +359,7 @@ do_next () {
 	HEADNAME=$(cat "$DOTEST"/head-name) &&
 	OLDHEAD=$(cat "$DOTEST"/head) &&
 	SHORTONTO=$(git rev-parse --short $(cat "$DOTEST"/onto)) &&
-	if test -d "$REWRITTEN"
-	then
-		test -f "$DOTEST"/current-commit &&
-			current_commit=$(cat "$DOTEST"/current-commit) &&
-			git rev-parse HEAD > "$REWRITTEN"/$current_commit
-		if test -f "$REWRITTEN"/$OLDHEAD
-		then
-			NEWHEAD=$(cat "$REWRITTEN"/$OLDHEAD)
-		else
-			NEWHEAD=$OLDHEAD
-		fi
-	else
-		NEWHEAD=$(git rev-parse HEAD)
-	fi &&
+	NEWHEAD=$(git rev-parse HEAD) &&
 	case $HEADNAME in
 	refs/*)
 		message="$GIT_REFLOG_ACTION: $HEADNAME onto $SHORTONTO)" &&
@@ -561,11 +557,18 @@ first and then run 'git rebase --continue' again."
 			MERGES_OPTION=--no-merges
 		fi
 
+		if test t = "$PRESERVE_MERGES"
+		then
+			first_parent="--first-parent"
+		else
+			first_parent=""
+		fi
+
 		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
 		SHORTHEAD=$(git rev-parse --short $HEAD)
 		SHORTONTO=$(git rev-parse --short $ONTO)
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
-			--abbrev=7 --reverse --left-right --cherry-pick \
+			--abbrev=7 --reverse --left-right --cherry-pick $first_parent \
 			$UPSTREAM...$HEAD | \
 			sed -n "s/^>/pick /p" > "$TODO"
 		cat >> "$TODO" << EOF
diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
new file mode 100644
index 0000000..b130f5f
--- /dev/null
+++ b/t/t3411-rebase-preserve-around-merges.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Stephen Haberman
+#
+
+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.
+'
+. ./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
+
+test_set_editor "$(pwd)/fake-editor.sh"
+chmod a+x fake-editor.sh
+
+# set up two branches like this:
+#
+# A - B - D - E - F
+#      \     /
+#       - C -
+
+test_expect_success 'setup' '
+	touch a &&
+	touch b &&
+	git add a &&
+	git commit -m A &&
+	git add b &&
+	git commit -m B &&
+	git tag B &&
+	git checkout -b branch &&
+	touch c &&
+	git add c &&
+	git commit -m C &&
+	git checkout master &&
+	touch d &&
+	git add d &&
+	git commit -m D &&
+	git merge branch &&
+	touch f &&
+	git add f &&
+	git commit -m F &&
+	git tag F
+'
+
+test_expect_success 'squash F into D' '
+	FAKE_LINES="1 squash 3 2" git rebase -i -p B &&
+	test "$(git rev-parse HEAD^2)" = "$(git rev-parse branch)" &&
+	test "$(git rev-parse HEAD~2)" = "$(git rev-parse B)"
+'
+
+test_done
+
-- 
1.6.0.2

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Stephen Haberman]

> I agree with Avi on what the rebase -i -p behavior should be for his
> scenario. This patch makes it so. However, the bane of my existence,
> t3404 is failing ~12 tests in, which is a real PITA to debug, so
> please let me know if this is a worthwhile tangent to continue on.

Ah, good old t3404--it caught me on an aspect I had considered but
wanted to avoid--Avi's (and my) preferred "--first-parent" way of
listing merges works great if the right hand side of the merge commits
are outside of the branch getting rebased.

E.g. my use case is when I merge in a stable release with ~100 commits
or so and could potentially want to move it around, perhaps squash
around it as Avi pointed it, I don't want all 100 commits that are in
the stable branch to be listed in my todo file.

However, t3404 makes a good point that if the right hand of the merge
has parents that are going to get rebased, the right hand side does
need to be included/shown/rewritten.

I also went and looked at the git-sequencer rewrite of rebase-i and it
looks slick. I don't fully understand it yet, but I'm much more
inclined now to just let Stephan (& sponors/list) ably handle the
problem. Especially since its moving to builtin, it moves the required
technical ability to contribute above my current skillset--perhaps that
is the intent. :-)

So, unless I think of something else, I'm done hacking on this and
am withdrawing the patch.

Though I am curious--with the sequencer, is the Avi/my request of not
listing out-of-band commits in the todo file going to be handled?

Some sort of "--first-parent-unless-included-in-rebase" flag.

Thanks,
Stephen

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Stephen Haberman]

> So, unless I think of something else, I'm done hacking on this and am
> withdrawing the patch.
>
> Though I am curious--with the sequencer, is the Avi/my request of not
> listing out-of-band commits in the todo file going to be handled?
> 
> Some sort of "--first-parent-unless-included-in-rebase" flag.

Okay, I lied--I have a patch that implements this behavior, passes Avi's
script-turned-test, and passes t3404. It implements the above algorithm
of, if in preserving merges mode, start with only first parents in the
todo list, and then recursively prepend right hand side commits to the
todo list only if their parents are going to be rewritten. This drops a
lot of cruft from rebase-i-p with large merges and is very cool, IMHO.

However, lest I burn my "PATCH v2" opportunity, I'm holding off on
posting the updated patch. It works and passes tests but I'm sure I'll
tinker with it some more over the next few days. It will also likely
conflict with my pu sh/maint-rebase3 patch, so I don't know whether to
base it on top of that one or not (guessing not).

Also, I think the patch itself is less interesting than the discussion
of whether this "first parent only" behavior is desired or not.

Obviously I think so--do others agree/disagree?

I've read more into the sequencer, and from what I can tell it still
just drives off a todo of pick/etc. input, and does not generate the
todo itself. So I think my patch is still fair game in terms of how to
generate either the current or the next generation rebase-i-p todo list.

I could be wrong on that though.

Thanks,
Stephen

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Avi Kivity]

Stephen Haberman wrote:
> However, t3404 makes a good point that if the right hand of the merge
> has parents that are going to get rebased, the right hand side does
> need to be included/shown/rewritten.
>
>   

But, won't those commits get linearized?  Won't git rebase pick the 
commits into the left-hand side of the merge instead of into the right 
hand side?

If git rebase is to handle nonlinear history, it needs much more 
expressive commands; not only saying which commit to pick, but also what 
the commit's parents shall be.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Stephan Beyer]

Hi,

Avi Kivity wrote:
> If git rebase is to handle nonlinear history, it needs much more
> expressive commands; not only saying which commit to pick, but also what  
> the commit's parents shall be.

git-sequencer has a "merge" command for that. I'm really sorry that this has
not been sent to the list yet. Nevertheless I'm always glad to find testers
for sequencer, so if you like, fetch
	git://repo.or.cz/git/sbeyer.git seq-builtin-dev

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Avi Kivity]

Stephan Beyer wrote:
> Hi,
>
> Avi Kivity wrote:
>   
>> If git rebase is to handle nonlinear history, it needs much more
>> expressive commands; not only saying which commit to pick, but also what  
>> the commit's parents shall be.
>>     
>
> git-sequencer has a "merge" command for that. I'm really sorry that this has
> not been sent to the list yet. Nevertheless I'm always glad to find testers
> for sequencer, so if you like, fetch
> 	git://repo.or.cz/git/sbeyer.git seq-builtin-dev
>
>   

But this isn't a merge; it's more of a 'pick into this branch' instead.

Maybe 'merge' can do this, but we also need to populate the todo with 
the required information (otherwise, git rebase -i without changes to 
the todo file will not be a no-op).


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH RFC] rebase--interactive: if preserving merges, use first-parent to limit what is shown.

[Shawn O. Pearce]

Stephen Haberman <stephen@exigencecorp.com> wrote:
> However, lest I burn my "PATCH v2" opportunity, I'm holding off on
> posting the updated patch. It works and passes tests but I'm sure I'll
> tinker with it some more over the next few days. It will also likely
> conflict with my pu sh/maint-rebase3 patch, so I don't know whether to
> base it on top of that one or not (guessing not).

When a patch series is in `pu` it can be rebased/replaced/amended
at any time.  That's why I parked it there.  The pu branch rewinds
and is rebuilt on a daily basis.  Any commits not yet merged into
maint, master or next are automatically rebased onto the latest
maint or master branch and get merged into that day's pu.

So don't hold back on posting patches.  Folks expect to see
patches on this list; talking is less productive than posting code.
Showing code that purports to solve a problem, or that at least
displays a problem concretely is worthy of discussion.

And don't worry about replacing what is currently in pu.  Its easily
done by the maintainer.  However don't expect daily updates to a
topic in pu.  Junio (and I) just don't have the bandwidth to keep
replacing patches every day.
 
-- 
Shawn.