[BUG] rebase -p loses commits

[BUG] rebase -p loses commits

[Jeff King]

I was trying to reproduce somebody's issue with a minimal test case, and
I ran across this setup wherein "rebase -p" silently drops some commits:

  commit() {
    echo $1 >file && git add file && git commit -m $1
  }

  # repo with two branches, each with conflicting content
  git init repo && cd repo &&
  commit base &&
  commit master &&
  git checkout -b feature HEAD^ &&
  commit feature &&

  # now merge them, with some fake resolution
  ! git merge master &&
  commit resolved &&

  # now try to "rebase -p" on top of master.
  git rebase -p master

The rebase completes successfully, but the "feature" commit and the
merge resolution are gone!

I'm totally unfamiliar with the preserve-merges code, and I won't have
time to dig further until later today or tomorrow, so I thought I'd
throw it out here and see if anybody has any clues.

-Peff

Re: [BUG] rebase -p loses commits

[Andrew Wong]

On 05/16/2011 06:33 AM, Jeff King wrote:
> I was trying to reproduce somebody's issue with a minimal test case, and
> I ran across this setup wherein "rebase -p" silently drops some commits:
>   

This particular patch seems to have something to do with the bug:
    d80d6bc146232d81f1bb4bc58e5d89263fd228d4
    http://thread.gmane.org/gmane.comp.version-control.git/98247/focus=98251

However, I can't figure out what the "odd boundary case" that this patch
was supposed to fix is. Anyone have any idea?

Andrew

Re: [BUG] rebase -p loses commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I was trying to reproduce somebody's issue with a minimal test case, and
> I ran across this setup wherein "rebase -p" silently drops some commits:
>
>   commit() {
>     echo $1 >file && git add file && git commit -m $1
>   }
>
>   # repo with two branches, each with conflicting content
>   git init repo && cd repo &&
>   commit base &&
>   commit master &&
>   git checkout -b feature HEAD^ &&
>   commit feature &&
>
>   # now merge them, with some fake resolution
>   ! git merge master &&
>   commit resolved &&
>
>   # now try to "rebase -p" on top of master.
>   git rebase -p master

Hmm, I am confused.  You have this:

   F---*  feature
  /   /
 B---M    master

and you are at "*".  If it were to rebase to linearize,

    B---M---F' 

with F' that has the same the contents as '*', possibly autoresolved by
"am -3" and/or "rerere", should be what you would get.

But what does it mean to rebase that on top of master, preserving merges
in the first place? You are already on top of 'master' and '*' itself
should be what you should get, no?  IOW, shouldn't you already be
up-to-date?

> I'm totally unfamiliar with the preserve-merges code, and I won't have
> time to dig further until later today or tomorrow, so I thought I'd
> throw it out here and see if anybody has any clues.

I don't use preserve-merge rebase either, but at least when you are
strictly ahead of the target, nothing should happen, I think.

Perhaps this should be a good start?

 git-rebase.sh |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 7a54bfc..2be10d6 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -466,10 +466,13 @@ require_clean_work_tree "rebase" "Please commit or stash them."
 # but this should be done only when upstream and onto are the same
 # and if this is not an interactive rebase.
 mb=$(git merge-base "$onto" "$orig_head")
-if test "$type" != interactive && test "$upstream" = "$onto" &&
-	test "$mb" = "$onto" &&
-	# linear history?
-	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
+if test "$upstream" = "$onto" && test "$mb" = "$onto" && {
+    {
+      test "$type" != interactive &&
+      # linear history?
+      ! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
+    } || test t,implied = "$preserve_merges,$interactive_rebase"
+  }
 then
 	if test -z "$force_rebase"
 	then

Re: [BUG] rebase -p loses commits

[Andrew Wong]

On 05/16/2011 04:36 PM, Junio C Hamano wrote:
>     F---*  feature
>    /   /
>   B---M    master
>
> But what does it mean to rebase that on top of master, preserving merges
> in the first place? You are already on top of 'master' and '*' itself
> should be what you should get, no?  IOW, shouldn't you already be
> up-to-date?
>    
Since preserve-merge uses the interactive-rebase, I think 
interactive-rebase should still pick the merge commit, which will then 
be consistent with what's happening if we rebase onto "F". So, without 
knowing whether "F" or "M" is the first-parent, I think 
interactive-rebase onto "F" and onto "M" should have the same effect. 
i.e. interactive-rebase picks the merge commit

Re: [BUG] rebase -p loses commits

[Junio C Hamano]

Andrew Wong <andrew.w@sohovfx.com> writes:

> On 05/16/2011 04:36 PM, Junio C Hamano wrote:
>>     F---*  feature
>>    /   /
>>   B---M    master
>>
>> But what does it mean to rebase that on top of master, preserving merges
>> in the first place? You are already on top of 'master' and '*' itself
>> should be what you should get, no?  IOW, shouldn't you already be
>> up-to-date?
>>    
> Since preserve-merge uses the interactive-rebase, I think
> interactive-rebase should still pick the merge commit, which will then
> be consistent with what's happening if we rebase onto "F". So, without
> knowing whether "F" or "M" is the first-parent, I think
> interactive-rebase onto "F" and onto "M" should have the same
> effect. i.e. interactive-rebase picks the merge commit

I agree that changing the behaviour based on the "first-ness" of the
parent is a wrong thing to do in general. But even then, I have a more
fundamental question.

What does rebasing that '*' on top of M really mean?

Does it mean this?

    F---*                 F'--*'
   /   /   --->          /   /
  B---M             B---M---/

In other words:

	Take all the commits reachable from '*', exclude the ones that are
	ancestors of M, and make sure that commit that corresponds to each
	of the remaining commits (in this case, F' and *' correspond to F
	and * respectively) are decendants of M, and reconstruct the graph
	preserving the parenthood relationship between corresponding
	commits.

I would understand why some project may want to require you to rebase to
the tip to keep a linear history, and the above sentence would be the
right specification for linearizing rebase except for the "and reconstruct
the graph" part.

But the above "preserving" rewrite does not even preserve the topology of
the graph (the original * is a true merge between two forks, but *' is
not) to begin with.  Also, if you want to _usefully_ place F' on top of M,
such a rewrite should resolve possible conflicts that was resolved at * in
the original graph at F' anyway, which would mean that the resulting *'
should become a totally empty commit.

Why would anybody want to do such a thing to begin with?

Re: [BUG] rebase -p loses commits

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> But the above "preserving" rewrite does not even preserve the topology of
> the graph (the original * is a true merge between two forks, but *' is
> not) to begin with.  Also, if you want to _usefully_ place F' on top of M,
> such a rewrite should resolve possible conflicts that was resolved at * in
> the original graph at F' anyway, which would mean that the resulting *'
> should become a totally empty commit.
>
> Why would anybody want to do such a thing to begin with?

Note that I am not saying "rebase -p" is not useful in general.  If you
had

         x---x---x---W---X
        /             \   \
    ---M               Y---Z

it is entirely sensible to want to have this history to exclude 'x'

         x---x---x---W---X
        /             \   \
    ---M---W'--X'      Y---Z
            \   \
             Y'--Z'

I think the patch I posted earlier should stop the problematic case Jeff
mentioned from happening, but I am trying to see if it makes sense to stop
without doing anything even when it is forced when onto and merge-base are
the same commit (which is not true for this "sensible" case).

Re: [BUG] rebase -p loses commits

[Jeff King]

On Mon, May 16, 2011 at 08:33:59PM -0400, Andrew Wong wrote:

> On 05/16/2011 04:36 PM, Junio C Hamano wrote:
> >    F---*  feature
> >   /   /
> >  B---M    master
> >
> >But what does it mean to rebase that on top of master, preserving merges
> >in the first place? You are already on top of 'master' and '*' itself
> >should be what you should get, no?  IOW, shouldn't you already be
> >up-to-date?
> Since preserve-merge uses the interactive-rebase, I think
> interactive-rebase should still pick the merge commit, which will
> then be consistent with what's happening if we rebase onto "F". So,
> without knowing whether "F" or "M" is the first-parent, I think
> interactive-rebase onto "F" and onto "M" should have the same effect.
> i.e. interactive-rebase picks the merge commit

Is it really the first-parentness here that is important to the
asymmetry? I thought it was more the fact that "feature" has the merge,
but "master" does not.

Therefore, "git rebase -p master feature" knows that there is nothing to
do; we already contain all of "master".

But "git rebase -p feature master" sees that we have an extra commit in
"feature" not in "master", and therefore we must attempt to rewrite on
top of that merge commit. Of course, when we try to do so, there are no
commits that are not already on "feature", so there is nothing to
rewrite.

So the outcomes are the same, but the reasoning is different. And isn't
that what happens with Junio's patch (I tried a simple test and it
seemed to be)?

-Peff

Re: [BUG] rebase -p loses commits

[Andrew Wong]

On 05/17/2011 01:44 AM, Jeff King wrote:
> Is it really the first-parentness here that is important to the
> asymmetry? I thought it was more the fact that "feature" has the merge,
> but "master" does not.
To know the fact that "feature" has the merge, don't we need to know
that "feature" is the first parent of the merge? For example, if "F" is
the head of "feature" and we're on "*" as a detached head, then we can
only say "feature" has the merge if we know "feature" is the first parent.
> So the outcomes are the same, but the reasoning is different. And isn't
> that what happens with Junio's patch (I tried a simple test and it
> seemed to be)?
I agree that the outcome of both should be the same. Junio's patch will
fix the case when we do "git rebase -p", but the bug will still appear
as soon as we do "git rebase -p -i", which I think is where the source
of the problem is. So we should be looking to fix the issue with "git
rebase -p -i", which will also fix "git rebase -p" too.

I think it's pretty reasonable for "rebase -p -i" to pick the merge
commit, which is already happening with "git rebase -p F". A possible
use case for picking the merge commit is to do a fixup/squash/reword on
"G" in the following graph:

      F---G---H
     /   /
    B---M

Re: [BUG] rebase -p loses commits

[Jeff King]

On Tue, May 17, 2011 at 12:07:38PM -0400, Andrew Wong wrote:

> > Is it really the first-parentness here that is important to the
> > asymmetry? I thought it was more the fact that "feature" has the merge,
> > but "master" does not.
> To know the fact that "feature" has the merge, don't we need to know
> that "feature" is the first parent of the merge? For example, if "F" is
> the head of "feature" and we're on "*" as a detached head, then we can
> only say "feature" has the merge if we know "feature" is the first parent.

No, if "F" is the head of "feature", then it does _not_ have the merge.
But it's not the merge that is important, it is really the fact that
if we are rebasing "feature" on "master", that "master ^feature" is
empty. IOW, we are already a superset. In this example, though, the
merge is the thing that gives us that superset.

> I agree that the outcome of both should be the same. Junio's patch will
> fix the case when we do "git rebase -p", but the bug will still appear
> as soon as we do "git rebase -p -i", which I think is where the source
> of the problem is. So we should be looking to fix the issue with "git
> rebase -p -i", which will also fix "git rebase -p" too.

Ah, I see. Yes, in that case, we should definitely be fixing "git rebase
-p -i".

-Peff

[RFC] Interactive-rebase doesn't pick all children of "upstream"

[Andrew Wong]

This is a work-in-progress patch for this issue.  Since this patch changes a
fairly significant behavior in interactive-rebase, I want to try to get some
feedbacks before I go ahead and start writing some tests for this new behavior.

Andrew Wong (1):
  Interactive-rebase doesn't pick all children of "upstream"

 git-rebase--interactive.sh               |    7 +++++--
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t3411-rebase-preserve-around-merges.sh |    2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

-- 
1.7.5.2.316.gd7d8c.dirty

[RFC] Interactive-rebase doesn't pick all children of "upstream"

[Andrew Wong]

Consider this graph:

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

and the following three commands:
  1. git rebase -i A
  2. git rebase -i --onto F B
  3. git rebase -i 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.  This
behavior of (3) is inconsistent with (1) and (2).

This also creates a bug if we do:
  4. git rebase -i C

In (4), E is never picked. And since interactive-rebase resets "HEAD" to
"onto", E is lost after the interactive-rebase.

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

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 edit
the "todo" list properly.
---
 git-rebase--interactive.sh               |    7 +++++--
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t3411-rebase-preserve-around-merges.sh |    2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 41ba96a..b6d1e5b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -711,7 +711,7 @@ 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)"
+	commits_after_upstream="$(git rev-list --reverse --parents $upstream..$orig_head | sane_grep " $upstream" | cut -d' ' -s -f1)"
 else
 	merges_option="--no-merges --cherry-pick"
 fi
@@ -744,7 +744,10 @@ 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 && (
+					test $p != $onto ||
+					expr "$commits_after_upstream" ":" ".*$sha1.*"
+					)
 				then
 					preserve=f
 				fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7d8147b..c3cddcd 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/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.5.2.316.gd7d8c.dirty

Re: [RFC] Interactive-rebase doesn't pick all children of "upstream"

[Andrew Wong]

On 11-05-21 1:51 AM, Andrew Wong wrote:
>   			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&&  (
> +					test $p != $onto ||
> +					expr "$commits_after_upstream" ":" ".*$sha1.*"
> +					)
>   				then
>   					preserve=f
>   				fi

Actually, I think this change might be effectively the same as removing 
both of the OR-conditions, which leaves only the "test -f" condition. 
That means commits are never skipped.

I guess this goes back to my first response to this issue. What kind of 
commits are skipped by the changes introduced in commit 
d80d6bc146232d81f1bb4bc58e5d89263fd228d4 ?

[PATCH] Interactive-rebase doesn't pick all children of "upstream"

[Andrew Wong]

Consider this graph:

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

and the following three commands:
  1. git rebase -i A
  2. git rebase -i --onto F A
  3. git rebase -i 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.  This
behavior of (3) is inconsistent with (1) and (2), and we cannot modify C
in the interactive-rebase.

The current behavior also creates a bug if we do:
  4. git rebase -i 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
Commits reachable from "upstream" should never be skipped under any
condition.  Otherwise we lose the chance to modify them like (3), and
create 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 edit
the "todo" list properly.  Also added test for scenario (4).
---
 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..16d316d 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 ():
+#
+# 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 '' '
+	(
+	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.6.rc0

Re: [PATCH] Interactive-rebase doesn't pick all children of "upstream"

[Johannes Sixt]

Am 05.06.2011 07:32, schrieb Andrew Wong:
> Consider this graph:
> 
>         D---E    (topic, HEAD)
>        /   /
>   A---B---C      (master)
>    \
>     F            (topic2)
> 
> and the following three commands:
>   1. git rebase -i A
>   2. git rebase -i --onto F A
>   3. git rebase -i 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.  This
> behavior of (3) is inconsistent with (1) and (2), and we cannot modify C
> in the interactive-rebase.

I cannot reproduce your claims:

- (1) and (2) picks B,C,D top A and F, but not E because E is a merge.

- (3) picks C and D, but not E because E is a merge.

> The current behavior also creates a bug if we do:
>   4. git rebase -i 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.

(4) picks only D, because E is a merge. I don't understand what you mean
that "D and E are lost"; E is not picked in the first place, but D is in
the todo-list; how can D be lost?

BTW, rebase never picks merges by design. I don't see anything wrong so
far with the current behavior. Please explain!

-- Hannes

Re: [PATCH] Interactive-rebase doesn't pick all children of "upstream"

[Andrew Wong]

On 11-06-05 5:16 AM, Johannes Sixt wrote:
> Am 05.06.2011 07:32, schrieb Andrew Wong:
>> 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.  This
>> behavior of (3) is inconsistent with (1) and (2), and we cannot modify C
>> in the interactive-rebase.
> I cannot reproduce your claims:
>
> - (1) and (2) picks B,C,D top A and F, but not E because E is a merge.
>
> - (3) picks C and D, but not E because E is a merge.
Ah, all those commands should have "-p" on them to preserve the merges. 
Thanks for the catch!

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

[Andrew Wong]

This patch contains an updated commit message.  The "-p" flags, which are vital
to this issue, were completely missing from the previous message.

Andrew Wong (1):
  rebase -i -p: doesn't pick certain merge commits that are children of
    "upstream"

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

-- 
1.7.6.rc0.1.gf20d7

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

[Andrew Wong]

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 edit
the "todo" list properly.  A new test has also been added for (4).

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 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..16d316d 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 ():
+#
+# 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 '' '
+	(
+	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.6.rc0.1.gf20d7

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

[Andrew Wong]

Any chance we can look into getting this patch into git?

http://permalink.gmane.org/gmane.comp.version-control.git/175185

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

[Junio C Hamano]

Andrew Wong <andrew.kw.w@gmail.com> 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.

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

[Andrew Wong]

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.

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

[Stephen Haberman]

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

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

[Andrew Wong]

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.

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

[Stephen Haberman]

> 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

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

[Andrew Wong]

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.

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

[Stephen Haberman]

> 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

[PATCH] rebase -i -p: include non-first-parent commits in todo list

[Andrew Wong]

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 <andrew.kw.w@gmail.com>
---
 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

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

[Andrew Wong]

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.

Re: [BUG] rebase -p loses commits

[Jeff King]

On Mon, May 16, 2011 at 01:36:51PM -0700, Junio C Hamano wrote:

> Hmm, I am confused.  You have this:
> 
>    F---*  feature
>   /   /
>  B---M    master
> 
> and you are at "*".  If it were to rebase to linearize,
> 
>     B---M---F' 
> 
> with F' that has the same the contents as '*', possibly autoresolved by
> "am -3" and/or "rerere", should be what you would get.
> 
> But what does it mean to rebase that on top of master, preserving merges
> in the first place? You are already on top of 'master' and '*' itself
> should be what you should get, no?  IOW, shouldn't you already be
> up-to-date?

To be honest, I am not sure what should happen. How this example came
about was that somebody had the same graph, except that a third branch,
"origin", also pointed at "B". They were confused why "git rebase
--pull" made them re-resolve the same conflict that they had
already handled during the merge.

The answer, of course, is that rebase is linearizing the commits instead
of trying to preserve the shape of history, and that they really wanted
"rebase -p".

I constructed the simplified example to show that the issue didn't have
to do with origin, but rather with linearizing.

So it's not a real-world example, in that sense. If it had done one of:

  1. Said "you are up to date" and one nothing.

  2. Put F' on top of M.

  3. Bailed and said "what you're doing is silly".

I would probably have shrugged and left it alone. But claiming success
and losing F entirely is pretty bad.

> I don't use preserve-merge rebase either, but at least when you are
> strictly ahead of the target, nothing should happen, I think.
> 
> Perhaps this should be a good start?

Aside from how unreadable that shell conditional is getting, I think
it's an improvement.

-Peff