Re: [PATCH] rebase -i -p: doesn't pick certain merge commits that are children of "upstream"

Andrew Wong [off-list ref] writes:

This patch fixes the inconsistency and bug by ensuring that all children
of upstream are always picked.  This essentially reverts the commit:

  d80d6bc (rebase-i-p: do not include non-first-parent commits touching UPSTREAM, 2008-10-15)

... whose commit log message mumbles about somebody's script but came with
no tests, so we will not know if this is breaking the other guy's workflow
while adding support to yours (Cc'ed Stephen Haberman who wrote the
previous one).

 
+test_expect_success '' '

There is no title to this test?

+	(
+	cd clone4 &&
+	git fetch &&
+	git rebase -p HEAD^2 &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify B" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote-tracking branch " | wc -l)
+	)
+'
+
 test_done

In general I think it is wrong to change behaviour depending on which
parent of a merge we are looking at (unless of course the user tells us
to, like "git log --first-parent"), so in that sense philosophically I
think the patch is going in the right direction, but I do worry about
potential regressions.

On 06/13/2011 12:01 PM, Junio C Hamano wrote:

There is no title to this test?
  

Ah, that's embarrassing. I'll fix that. Thanks!

In general I think it is wrong to change behaviour depending on which
parent of a merge we are looking at (unless of course the user tells us
to, like "git log --first-parent"), so in that sense philosophically I
think the patch is going in the right direction, but I do worry about
potential regressions.
  

I totally agree.  Ever since Jeff brought up this issue, I've been
wondering what issue/workflow is that patch trying to fix.  If the
"todo" list doesn't change the parent of the merge commits, git should
be able to do a fast-forward on the merge, which means the merge won't
be rewritten anyway.  Just a wild guess: maybe back then, git will
actually rewrite the merge regardless?  Anyway, let's wait for a reply
from Stephen.

Hey,

Ever since Jeff brought up this issue, I've been wondering what
issue/workflow is that patch trying to fix.

I'm fairly sure the case the patch was fixing is t3411's "squash F1 into
D1".

Where, starting with a tree like:

# A1 - B1 - D1 - E1 - F1
#       \        /
#        -- C1 --

The user is on F1 and issues: "git rebase -i -p B1", the todo list
is "D1, E1, F1" (no C1), and they choose "D1 squash F1, E1",
the resulting tree should be:

# A1 - B1 - D2 - E2
#       \        /
#        -- C1 --

And, the fix was that C1 should not be in the todo list.

Perhaps that is unreasonable with whatever you guys are looking at now,
but, IIRC, the use case was that B1=some old commit, like a 2.0
release, and a bunch of work happened on the C1 branch, it was merged
in E1, but now when you want to rebase D1/E1/F1 on top of B1, you don't
want all of the noise of the C1 commit(s), since when rewriting E1 into
E2, you can just reuse the un-rewritten C1 as its 2nd parent.

Well, and not just the noise--since the todo is still flat, if C1
was listed in the todo, there's no way to recreate E2 as a merge and
maintain the C1 commit(s) as a separate branch. I think C1 would get
flattened between D2/E2, depending on where it was in the todo. You'd
lose a merge, contrary to the -p flag. That sounds like the core issue
that was being fixed.

The patch in question:

http://article.gmane.org/gmane.comp.version-control.git/98251

Did actually have a test (t3411) but it was still failing until
the following commit:

http://article.gmane.org/gmane.comp.version-control.git/98253

Where the test changed from expect failure to expect success. I
remember that looking odd at the time, but for some reason liked the
commits being separate.

- Stephen

Here's a list of those commits in "git log"-order for easy reference:
   80fe82e rebase-i-p: if todo was reordered use HEAD as the rewritten 
parent
   d80d6bc rebase-i-p: do not include non-first-parent commits touching 
UPSTREAM
   acc8559 rebase-i-p: only list commits that require rewriting in todo
   a4f25e3 rebase-i-p: fix 'no squashing merges' tripping up non-merges
   bb64507 rebase-i-p: delay saving current-commit to REWRITTEN if squashing
   72583e6 rebase-i-p: use HEAD for updating the ref instead of mapping 
OLDHEAD
   42f939e rebase-i-p: test to exclude commits from todo based on its 
parents

On 11-06-16 6:24 PM, Stephen Haberman wrote:

Perhaps that is unreasonable with whatever you guys are looking at now,
but, IIRC, the use case was that B1=some old commit, like a 2.0
release, and a bunch of work happened on the C1 branch, it was merged
in E1, but now when you want to rebase D1/E1/F1 on top of B1, you don't
want all of the noise of the C1 commit(s), since when rewriting E1 into
E2, you can just reuse the un-rewritten C1 as its 2nd parent.

In commit a4f25e3, we could already rebase B1 and squash F1 onto D1, 
while reusing C1 and recreating the merge. That means we could already 
pass t3411.2if we adjusted the todo-list to account for the extra "pick 
C1" line.

Well, and not just the noise--since the todo is still flat, if C1
was listed in the todo, there's no way to recreate E2 as a merge and
maintain the C1 commit(s) as a separate branch. I think C1 would get
flattened between D2/E2, depending on where it was in the todo. You'd
lose a merge, contrary to the -p flag. That sounds like the core issue
that was being fixed

The merge shouldn't get flattened when the "-p" is used. As long as the 
merge commit appears in the todo-list, git will trace the parents of the 
merge commit, find the original or rewritten parents, and perform the 
merge.  Slightly off-topic, but I believe the branches will remain 
intact as long as the branch commits remain in the same topo-order 
relative to each other in the todo-list. i.e. git will be confused if we 
try to move a commit from one branch into the other.

The "noise" is filtered out by by commit d80d6bc.  However, I think we 
should keep the commits from branch C1, since there could be a scenario 
where we actually want to squash F1 onto C1 instead. That commit also 
introduced a bug that Jeff King was running into: if we do "git rebase 
-i -p C1", the todo-list becomes a "noop", which means HEAD is reset to 
C1 and we lose the merge commit and F1.

So what I did in my patch is essentially revert the changes from 
d80d6bc, and adjust t3411.2 to account for the extra "pick C1" line.

In commit a4f25e3, we could already rebase B1 and squash F1 onto D1, 
while reusing C1 and recreating the merge. That means we could
already pass t3411.2if we adjusted the todo-list to account for the
extra "pick C1" line.

You're right. I was wrong about that.

Slightly off-topic, but I believe the branches will remain intact as
long as the branch commits remain in the same topo-order relative to
each other in the todo-list.

If in topo-order, yeah, I guess that is right.

i.e. git will be confused if we try to move a commit from one branch
into the other.

Right. If I do `rebase -i -p B1` and in the todo put C1 after F1, I get
a fatal message that E1 cannot be cherry picked.

Given rebase-i-p's limited ability to reorder graphs, e.g. the error
above, my understanding was that, when -p is used, only first-parent
changes should be in the todo. This straight line, non-graph list does
limit what the user can do, but, AFAIK, the benefit is that rebase-i-p
can then actually handle any given reordering of the todo.

Letting C1 into the todo would mean having to explain to the user why
some of their reorderings worked and others didn't. Or else making
rebase-i-p smart enough to handle all cases. Which, IIRC, was something
considered unlikely just given the fact that todo is flat and there
isn't a way for the user to express topo reorderings. At the time,
there was talk of another rewriting tool that would use marks and
other hints to handle graphs and it was considered what, if anything,
would eventually handle complex rewrites like this.

I think that Jeff's use case of rebase-i-p'ing C1, which is not on the
first-parent list of commits, should be an error as it delves into
territory (topo reordering) that rebase-i-p can't fully handle.

(If -p isn't used, just regular rebase, everything is being flattened,
so there is no concern of topo reordering, so things are a lot simpler
and C1 can/should be in the list.)

- Stephen

On 11-06-18 11:17 AM, Stephen Haberman wrote:

Letting C1 into the todo would mean having to explain to the user why
some of their reorderings worked and others didn't.

The bug section in rebase's documentation does mention that "attempts to 
reorder commits tend to produce counterintuitive results", which I think 
serves as a fairly good warning saying "reorder at your own risk".  
Also, if we do a "rebase-i-p A1", the C1 branch will appear in the todo 
list.  A while ago I actually ran into this scenario, and I want to 
squash a commit onto the C1 branch, which I can't if I simply choose B1 
as the base.  To workaround it, I just made A1 the base so that the C1 
branch will appear in the todo for me to squash upon.  Otherwise, doing 
the squash onto C1 manually would've involved several more steps.

I think that Jeff's use case of rebase-i-p'ing C1, which is not on the
first-parent list of commits, should be an error as it delves into
territory (topo reordering) that rebase-i-p can't fully handle.

There shouldn't be any topo-reordering unless the user explicitly 
changes the order of the commit.  The user is faced with the same 
limitations (and bugs) as rebase-i-p'ing D1, so we shouldn't have to 
handle the C1 case any different.  rebase is perfectly capable of 
handling the D1 case, just as how the C1 case is handled.  We're only 
running into this issue because we're trying to filter out C1 when 
rebase-i-p'ing B1.

The bug section in rebase's documentation does mention that "attempts
to reorder commits tend to produce counterintuitive results", which I
think serves as a fairly good warning saying "reorder at your own
risk".

True.

Also, if we do a "rebase-i-p A1", the C1 branch will appear in
the todo list.

Hm, good point.

There shouldn't be any topo-reordering unless the user explicitly 
changes the order of the commit.  The user is faced with the same 
limitations (and bugs) as rebase-i-p'ing D1, so we shouldn't have to 
handle the C1 case any different.  rebase is perfectly capable of 
handling the D1 case, just as how the C1 case is handled.  We're only 
running into this issue because we're trying to filter out C1 when 
rebase-i-p'ing B1.

Okay, that makes sense.

I agree with you then, with the behavior of "rebase-i-p A1" plus the
disclaimer in the docs warrants C1 showing up, C1 should be in the
todo list for "rebase-i-p B1" as well.

...I can think of cases where personally I'd want to only move
around commits on the first-parent line, e.g. even in the case of
"rebase-i-p A1", to have less noise (C1 and any others on its branch)
in the todo, but at that point it sounds like I'm projecting behavior
onto rebase-i-p that isn't actually there.

- Stephen

Consider this graph:

        D---E    (topic, HEAD)
       /   /
  A---B---C      (master)
   \
    F            (topic2)

and the following three commands:
  1. git rebase -i -p A
  2. git rebase -i -p --onto F A
  3. git rebase -i -p B

Currently, (1) and (2) will pick B, D, C, and E onto A and F,
respectively.  However, (3) will only pick D and E onto B, but not C,
which is inconsistent with (1) and (2).  As a result, we cannot modify C
during the interactive-rebase.

The current behavior also creates a bug if we do:
  4. git rebase -i -p C

In (4), E is never picked.  And since interactive-rebase resets "HEAD"
to "onto" before picking any commits, D and E are lost after the
interactive-rebase.

This patch fixes the inconsistency and bug by ensuring that all children
of upstream are always picked.  This essentially reverts the commit:
  d80d6bc146232d81f1bb4bc58e5d89263fd228d4

When compiling the todo list, commits reachable from "upstream" should
never be skipped under any conditions.  Otherwise, we lose the ability
to modify them like (3), and create a bug like (4).

Two of the tests contain a scenario like (3).  Since the new behavior
added more commits for picking, these tests need to be updated to
account for the additional pick lines.  A new test has also been added
for (4).

Signed-off-by: Andrew Wong <redacted>
---
 git-rebase--interactive.sh               |    3 +--
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t3409-rebase-preserve-merges.sh        |   28 +++++++++++++++++++++++++++-
 t/t3411-rebase-preserve-around-merges.sh |    2 +-
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 65690af..c6ba7c1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -713,7 +713,6 @@ then
 	# parents to rewrite and skipping dropped commits would
 	# prematurely end our probe
 	merges_option=
-	first_after_upstream="$(git rev-list --reverse --first-parent $upstream..$orig_head | head -n 1)"
 else
 	merges_option="--no-merges --cherry-pick"
 fi

@@ -746,7 +745,7 @@ do
 			preserve=t
 			for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
 			do
-				if test -f "$rewritten"/$p -a \( $p != $onto -o $sha1 = $first_after_upstream \)
+				if test -f "$rewritten"/$p
 				then
 					preserve=f
 				fi

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 47c8371..8538813 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -295,7 +295,7 @@ test_expect_success 'preserve merges with -p' '
 '
 
 test_expect_success 'edit ancestor with -p' '
-	FAKE_LINES="1 edit 2 3 4" git rebase -i -p HEAD~3 &&
+	FAKE_LINES="1 2 edit 3 4" git rebase -i -p HEAD~3 &&
 	echo 2 > unrelated-file &&
 	test_tick &&
 	git commit -m L2-modified --amend unrelated-file &&

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 08201e2..6de4e22 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh

@@ -37,7 +37,15 @@ export GIT_AUTHOR_EMAIL
 #      \
 #       B2     <-- origin/topic
 #
-# In all cases, 'topic' is rebased onto 'origin/topic'.
+# Clone 4 (merge using second parent as base):
+#
+# A1--A2--B3   <-- origin/master
+#  \
+#   B1--A3--M  <-- topic
+#    \     /
+#     \--A4    <-- topic2
+#      \
+#       B2     <-- origin/topic
 
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&

@@ -57,6 +65,13 @@ test_expect_success 'setup for merge-preserving rebase' \
 	git merge origin/master
 	) &&
 
+	git clone ./. clone4 &&
+	(
+		cd clone4 &&
+		git checkout -b topic origin/topic &&
+		git merge origin/master
+	) &&
+
 	echo Fifth > B &&
 	git add B &&
 	git commit -m "Add different B" &&

@@ -123,4 +138,15 @@ test_expect_success 'rebase -p preserves no-ff merges' '
 	)
 '
 
+test_expect_success 'rebase -p works when base inside second parent' '
+	(
+	cd clone4 &&
+	git fetch &&
+	git rebase -p HEAD^2 &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify B" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote-tracking branch " | wc -l)
+	)
+'
+
 test_done

diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
index 14a23cd..ace8e54 100755
--- a/t/t3411-rebase-preserve-around-merges.sh
+++ b/t/t3411-rebase-preserve-around-merges.sh

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

-- 
1.7.2.2

On 11-06-18 1:12 PM, Stephen Haberman wrote:

...I can think of cases where personally I'd want to only move
around commits on the first-parent line, e.g. even in the case of
"rebase-i-p A1", to have less noise (C1 and any others on its branch)
in the todo, but at that point it sounds like I'm projecting behavior
onto rebase-i-p that isn't actually there.

Yes, it would definitely be useful to be able to do that.  In fact, 
there's a somewhat relevant expect-failure-test t3404.18 that is testing 
for that.  Like you said, we need a way to express topology in the todo 
list, which I'm not sure what a good representation is.