git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
@ 2012-05-21 20:19 Johannes Sixt
  2012-05-22 18:23 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Johannes Sixt @ 2012-05-21 20:19 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Stephen Haberman, Andrew Wong

When rebase -p had to replay a merge commit, it used to redo the merge.
But this has drawbacks:

- When the merge was evil, i.e., contained changes that are in neither of
  the parents, that change was not preserved.

- The 'git merge' invocation passed the commit message of the old merge
  commit, but it still obeyed the merge.log option. If it was set, the log
  ended up twice in the commit message.

Replace the 'git merge' by a cherry-pick that picks the changes that the
merge introduces with respect to the first parent.

The change in t3409 reflects the new situation after a conflicting merge
is replayed: Earlier, 'git merge' found a conflict in an add/add
situation with file B only in stage 2 and 3, now it is a regular three-way
merge conflict.

A test in  t3410 fails now with this new implementation. The branch that
is merged in by the old history and that is to be preserved is already
merged into the new upstream. In the old implementation, 'git merge'
detected this situation and succeeded with "Already up to date", but the
new implementation does not notice that it is pointless to keep the merge.
It just tries to follow instructions, but fails due to a conflicting
cherry-pick. This is a corner case whose correct solution should be that
the "pick" instruction is not generated in the first place.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This is RFC because of the new failure addressed in the last paragraph
 above (see also the art work in the patch context below), how serious
 the failure is, and what to do about it.

 See also this recent thread for further motivation:
 http://thread.gmane.org/gmane.comp.version-control.git/194434/focus=194737

 git-rebase--interactive.sh                | 12 +++++++-----
 t/t3409-rebase-preserve-merges.sh         |  2 +-
 t/t3410-rebase-preserve-dropped-merges.sh |  4 ++--
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..642daab 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -312,12 +312,14 @@ pick_one_preserving_merges () {
 			msg_content="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
-			if ! do_with_author output \
-				git merge --no-ff ${strategy:+-s $strategy} -m \
-					"$msg_content" $new_parents
+			printf "%s\n" $new_parents >"$GIT_DIR"/MERGE_HEAD
+			printf "%s\n" "$msg_content" >"$GIT_DIR"/MERGE_MSG
+			if output git cherry-pick -m 1 -n "$sha1"
 			then
-				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
-				die_with_patch $sha1 "Error redoing merge $sha1"
+				do_with_author output git commit --no-verify -F "$GIT_DIR"/MERGE_MSG ||
+					die_with_patch $sha1 "Could not replay merge $sha1"
+			else
+				die_with_patch $sha1 "Could not pick merge $sha1"
 			fi
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 6de4e22..7161b99 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -116,7 +116,7 @@ test_expect_success '--continue works after a conflict' '
 	cd clone2 &&
 	git fetch &&
 	test_must_fail git rebase -p origin/topic &&
-	test 2 = $(git ls-files B | wc -l) &&
+	test 3 = $(git ls-files B | wc -l) &&
 	echo Resolved again > B &&
 	test_must_fail git rebase --continue &&
 	grep "^@@@ " .git/rebase-merge/patch &&
diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
index 6f73b95..36c69b2 100755
--- a/t/t3410-rebase-preserve-dropped-merges.sh
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -60,12 +60,12 @@ test_expect_success 'skip same-resolution merges with -p' '
 # A - B - C - D - E
 #   \             \ \
 #     F - G - H -- L2 \        -->   L2
 #       \             |                \
 #         I -- G3 --- J2 -- K2           I -- G3 -- K2
-# G2 = different changes as G
-test_expect_success 'keep different-resolution merges with -p' '
+# G3 = different changes as G
+test_expect_failure 'keep different-resolution merges with -p' '
 	git checkout H &&
 	test_must_fail git merge E &&
 	test_commit L2 file1 23 &&
 	git checkout I &&
 	test_commit G3 file1 4 &&
-- 
1.7.10.2.529.g0c18cfd

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-21 20:19 [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes Johannes Sixt
@ 2012-05-22 18:23 ` Junio C Hamano
  2012-05-22 19:30   ` Johannes Sixt
  2012-05-22 23:38 ` Jonathan Nieder
  2012-05-23 15:37 ` Martin von Zweigbergk
  2 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-05-22 18:23 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Git Mailing List, Stephen Haberman, Andrew Wong

Johannes Sixt <j6t@kdbg.org> writes:

> When rebase -p had to replay a merge commit, it used to redo the merge.
> But this has drawbacks:
>
> - When the merge was evil, i.e., contained changes that are in neither of
>   the parents, that change was not preserved.

This is a desiable property, and not necessarily limited to "evil" merges
but also applies to everyday conflict resolutions.  Replaying the change
between the merge and its first parent is a way to achieve it, but I think
it also has downsides.  If you are replaying a merge to an updated history
that already contains a part of what is merged, some part of the
difference between the original merge and its first parent already exists
in the commit that the will become the first parent of the replayed merge.

> - The 'git merge' invocation passed the commit message of the old merge
>   commit, but it still obeyed the merge.log option. If it was set, the log
>   ended up twice in the commit message.

This should be fixed independent of this patch, no?  Is it a matter of
just passing --no-log or something, or is there anything more elaborate
necessary?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-22 18:23 ` Junio C Hamano
@ 2012-05-22 19:30   ` Johannes Sixt
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Sixt @ 2012-05-22 19:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Stephen Haberman, Andrew Wong

Am 22.05.2012 20:23, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> When rebase -p had to replay a merge commit, it used to redo the merge.
>> But this has drawbacks:
>>
>> - When the merge was evil, i.e., contained changes that are in neither of
>>   the parents, that change was not preserved.
> 
> This is a desiable property, and not necessarily limited to "evil" merges
> but also applies to everyday conflict resolutions.  Replaying the change
> between the merge and its first parent is a way to achieve it, but I think
> it also has downsides.  If you are replaying a merge to an updated history
> that already contains a part of what is merged, some part of the
> difference between the original merge and its first parent already exists
> in the commit that the will become the first parent of the replayed merge.

Yes, in such a case we need some cleverness from the merge machinery in
cherry-pick: It has to recognize that some changes were applied on both
branches. This might generate new conflicts.

I'll have to see whether this is an issue in practice.

At any rate, if any difficulties with a particular merge commit can be
anticipated, it is always possible to replace the

  pick that-merge-commit

instruction by

  x git merge that-branch

(we could even generate these lines in the instruction sheet as a comment).

>> - The 'git merge' invocation passed the commit message of the old merge
>>   commit, but it still obeyed the merge.log option. If it was set, the log
>>   ended up twice in the commit message.
> 
> This should be fixed independent of this patch, no?  Is it a matter of
> just passing --no-log or something, or is there anything more elaborate
> necessary?

Frankly, IMHO[*], rebase -p that is based on git-cherry-pick is so much
better that I don't want to think about which workarounds would be
needed to make it work better with git-merge :-)

[*] It is still only an opinion. I haven't gotten around to use the
updated rebase -p in the field. Another reason that it's still RFC.

Thanks,
-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-21 20:19 [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes Johannes Sixt
  2012-05-22 18:23 ` Junio C Hamano
@ 2012-05-22 23:38 ` Jonathan Nieder
  2012-05-23 15:37 ` Martin von Zweigbergk
  2 siblings, 0 replies; 16+ messages in thread
From: Jonathan Nieder @ 2012-05-22 23:38 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Git Mailing List, Stephen Haberman, Andrew Wong

Johannes Sixt wrote:

> - The 'git merge' invocation passed the commit message of the old merge
>   commit, but it still obeyed the merge.log option. If it was set, the log
>   ended up twice in the commit message.

Likewise for the "Conflicts:" paragraph, which should also be pretty
simple to fix directly.  I don't think that points directly to a
fundamental flaw in the current implementation, but it does make it
seem like no one has been using "rebase -p" for anything nontrivial. :)

[...]
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -312,12 +312,14 @@ pick_one_preserving_merges () {
>  			msg_content="$(commit_message $sha1)"
>  			# No point in merging the first parent, that's HEAD
>  			new_parents=${new_parents# $first_parent}
> -			if ! do_with_author output \
> -				git merge --no-ff ${strategy:+-s $strategy} -m \
> -					"$msg_content" $new_parents
> +			printf "%s\n" $new_parents >"$GIT_DIR"/MERGE_HEAD
> +			printf "%s\n" "$msg_content" >"$GIT_DIR"/MERGE_MSG
> +			if output git cherry-pick -m 1 -n "$sha1"
>  			then
> +				do_with_author output git commit --no-verify -F "$GIT_DIR"/MERGE_MSG ||
> +					die_with_patch $sha1 "Could not replay merge $sha1"
> +			else
> -				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
> -				die_with_patch $sha1 "Error redoing merge $sha1"
> +				die_with_patch $sha1 "Could not pick merge $sha1"
>  			fi

Cute.  The approach looks very sensible to me.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-21 20:19 [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes Johannes Sixt
  2012-05-22 18:23 ` Junio C Hamano
  2012-05-22 23:38 ` Jonathan Nieder
@ 2012-05-23 15:37 ` Martin von Zweigbergk
  2012-05-23 18:53   ` Junio C Hamano
  2012-05-23 18:59   ` Johannes Sixt
  2 siblings, 2 replies; 16+ messages in thread
From: Martin von Zweigbergk @ 2012-05-23 15:37 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Git Mailing List, Stephen Haberman, Andrew Wong

On Mon, May 21, 2012 at 1:19 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Replace the 'git merge' by a cherry-pick that picks the changes that the
> merge introduces with respect to the first parent.

Just a reminder of what I said in [1] (the same thread that you referenced):

I think this will break "git rebase -p --onto g b f" on  the below history.

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

Even if this wasn't the intended use case, git currently supports it
and I would personally be a little surprised if no one has gotten used
to it working. So should we at least check for this case and handle it
with "git merge" as usual? Or stop supporting it and error out instead
(and mention in release notes?)?

Btw, with a history without "internal" merges, but where the merge
commit was generated "backwards" so the first parent is not in the
to-be-rebased history, am I correct in thinking that the "git
cherry-pick -m 1" will fail? Do you think we should consider this case
or do we consider that too broken to care about?


Martin

 [1] http://thread.gmane.org/gmane.comp.version-control.git/194434/focus=194786

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-23 15:37 ` Martin von Zweigbergk
@ 2012-05-23 18:53   ` Junio C Hamano
  2012-05-23 20:41     ` Martin von Zweigbergk
  2012-05-23 18:59   ` Johannes Sixt
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-05-23 18:53 UTC (permalink / raw)
  To: Martin von Zweigbergk
  Cc: Johannes Sixt, Git Mailing List, Stephen Haberman, Andrew Wong

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> On Mon, May 21, 2012 at 1:19 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Replace the 'git merge' by a cherry-pick that picks the changes that the
>> merge introduces with respect to the first parent.
>
> Just a reminder of what I said in [1] (the same thread that you referenced):
>
> I think this will break "git rebase -p --onto g b f" on  the below history.
>
>           .-e-.
>          /     \
>       .-c---d---f
>      /
> a---b---g


... where the replayed history would look like this:

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

and e' and e, even though being "moral equivalent", may have different
degrees of change since c' and c respectively depending on how changes
between b and c and changes between b and g are related to each other.

If the change in the dag b..f does not have much to do with the change
between b and g, then the change from c to e would be the same as the
change from c' to e', and if the original merge at f resolved conflicts
between e and d (or added an evil change), replaying the difference from e
to f on top of e' may give a better result.

I think your original point was that the above clean picture would not
hold if e-b and g-b has interactions.  g-b may contain some change that
e-b has independently done, in which case e'-g would be made smaller than
e-b when the conflict is resolved while replaying e to produce e' on top
of c' (the same applies if you replace e with any commit in the dag
"rev-list ^b e d").  The merge to produce f' may result in conflicts,
whether you merge e' and d' or replay f-e on top of e'.

A better way to keep potential "evil merges" at f while replaying to
produce f' may *not* be by applying the difference f-e on top of e'.
Instead, you can learn from what 'rerere' does.  That is, to attempt a
mechanical merge between e and d and call the result (with possible
conflict markers and all) pre-f.  If you compare pre-f and f, that is the
resolution and evil change made at f.  When reproducing f', mechanically
merge e' and d', call the result (again, with possible conflict markers
and all) pre-f', and try reproducing f' by a 3-way merge between pre-f,
pre-f' and f.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-23 15:37 ` Martin von Zweigbergk
  2012-05-23 18:53   ` Junio C Hamano
@ 2012-05-23 18:59   ` Johannes Sixt
  1 sibling, 0 replies; 16+ messages in thread
From: Johannes Sixt @ 2012-05-23 18:59 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: Git Mailing List, Stephen Haberman, Andrew Wong

Am 23.05.2012 17:37, schrieb Martin von Zweigbergk:
> On Mon, May 21, 2012 at 1:19 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Replace the 'git merge' by a cherry-pick that picks the changes that the
>> merge introduces with respect to the first parent.
> 
> Just a reminder of what I said in [1] (the same thread that you referenced):
> 
> I think this will break "git rebase -p --onto g b f" on  the below history.
> 
>           .-e-.
>          /     \
>       .-c---d---f
>      /
> a---b---g

This still works. It is tested by t3411.3.

> Even if this wasn't the intended use case, git currently supports it
> and I would personally be a little surprised if no one has gotten used
> to it working. So should we at least check for this case and handle it
> with "git merge" as usual? Or stop supporting it and error out instead
> (and mention in release notes?)?

YESSS, drop it. It would allow us to remove all sorts of crufty
work-arounds for work-arounds. THIS IS NOT 'git sequencer'!

> Btw, with a history without "internal" merges, but where the merge
> commit was generated "backwards" so the first parent is not in the
> to-be-rebased history, am I correct in thinking that the "git
> cherry-pick -m 1" will fail? Do you think we should consider this case
> or do we consider that too broken to care about?

I think something odd will happen. If in doubt, I would suggest to
consider this such an odd case which rebase is not intended for ;-)
(garbage in - garbage out).

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-23 18:53   ` Junio C Hamano
@ 2012-05-23 20:41     ` Martin von Zweigbergk
  2012-05-24 17:31       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Martin von Zweigbergk @ 2012-05-23 20:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Git Mailing List, Stephen Haberman, Andrew Wong

On Wed, May 23, 2012 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I think your original point was that the above clean picture would not
> hold if e-b and g-b has interactions.  g-b may contain some change that
> e-b has independently done, in which case e'-g would be made smaller than
> e-b when the conflict is resolved while replaying e to produce e' on top
> of c' (the same applies if you replace e with any commit in the dag
> "rev-list ^b e d").  The merge to produce f' may result in conflicts,
> whether you merge e' and d' or replay f-e on top of e'.

Right. Or if e' was changed as a result of a "edit" action (I meant to
include '-i' in addition to '-p').

> A better way to keep potential "evil merges" at f while replaying to
> produce f' may *not* be by applying the difference f-e on top of e'.
> Instead, you can learn from what 'rerere' does.  That is, to attempt a
> mechanical merge between e and d and call the result (with possible
> conflict markers and all) pre-f.  If you compare pre-f and f, that is the
> resolution and evil change made at f.  When reproducing f', mechanically
> merge e' and d', call the result (again, with possible conflict markers
> and all) pre-f', and try reproducing f' by a 3-way merge between pre-f,
> pre-f' and f.

Yes, I've had the same idea myself. Anyway, as Johannes said, that's
probably something to consider for the sequencer.

Martin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-23 20:41     ` Martin von Zweigbergk
@ 2012-05-24 17:31       ` Junio C Hamano
  2012-05-24 17:47         ` Martin von Zweigbergk
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-05-24 17:31 UTC (permalink / raw)
  To: Martin von Zweigbergk
  Cc: Johannes Sixt, Git Mailing List, Stephen Haberman, Andrew Wong

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> Yes, I've had the same idea myself. Anyway, as Johannes said, that's
> probably something to consider for the sequencer.

Are you saying that "rebase -any-variant" and the sequencer should behave
differently?  It is not immediately obvious to me why it is a good idea.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-24 17:31       ` Junio C Hamano
@ 2012-05-24 17:47         ` Martin von Zweigbergk
  2012-05-24 20:09           ` Junio C Hamano
  2012-05-24 20:32           ` Johannes Sixt
  0 siblings, 2 replies; 16+ messages in thread
From: Martin von Zweigbergk @ 2012-05-24 17:47 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Git Mailing List, Stephen Haberman, Andrew Wong

On Thu, May 24, 2012 at 10:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
>> Yes, I've had the same idea myself. Anyway, as Johannes said, that's
>> probably something to consider for the sequencer.
>
> Are you saying that "rebase -any-variant" and the sequencer should behave
> differently?  It is not immediately obvious to me why it is a good idea.

That's not what I meant to say. I thought the sequencer is supposed to
replace much of git-rebase and I thought that's what Johannes was
referring to as well when he said not to make git-rebase too
intelligent. I assumed the reasoning was that any work spent on
git-rebase at this point will be thrown away once git-rebase instead
calls into the sequencer. But I have not been very involved in the
discussions about the sequencer, so I may very well have misunderstood
things. Or are you saying that even if it's true that git-rebase will
eventually call into the sequencer for much of its operation, this
specific part (if at all implemented) does not belong in the
sequencer?

Martin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-24 17:47         ` Martin von Zweigbergk
@ 2012-05-24 20:09           ` Junio C Hamano
  2012-05-24 20:32           ` Johannes Sixt
  1 sibling, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2012-05-24 20:09 UTC (permalink / raw)
  To: Martin von Zweigbergk
  Cc: Johannes Sixt, Git Mailing List, Stephen Haberman, Andrew Wong

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> ... I assumed the reasoning was that any work spent on
> git-rebase at this point will be thrown away once git-rebase instead
> calls into the sequencer....

Ah, sorry I mis-read you.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-24 17:47         ` Martin von Zweigbergk
  2012-05-24 20:09           ` Junio C Hamano
@ 2012-05-24 20:32           ` Johannes Sixt
  2012-05-24 21:34             ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Johannes Sixt @ 2012-05-24 20:32 UTC (permalink / raw)
  To: Martin von Zweigbergk
  Cc: Junio C Hamano, Git Mailing List, Stephen Haberman, Andrew Wong

Am 24.05.2012 19:47, schrieb Martin von Zweigbergk:
> On Thu, May 24, 2012 at 10:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>>
>>> Yes, I've had the same idea myself. Anyway, as Johannes said, that's
>>> probably something to consider for the sequencer.
>>
>> Are you saying that "rebase -any-variant" and the sequencer should behave
>> differently?  It is not immediately obvious to me why it is a good idea.
> 
> That's not what I meant to say. I thought the sequencer is supposed to
> replace much of git-rebase and I thought that's what Johannes was
> referring to as well when he said not to make git-rebase too
> intelligent.

You are probably refering to what I said here:

http://thread.gmane.org/gmane.comp.version-control.git/194434/focus=195074

When I wrote the post, I was not aware that rebase -p *is* indeed able
to transplant a branchy topic to a new upstream. I was convinced that
rebase -p can only move a (first-parent) topic line which may have
merged in some unrelated other topics. So, you should take it with a
large grain of salt.

-------

Today I was able to use rebase -i -p in the field. I used it to rebuild
an integration branch (akin to git's pu branch). Guess what? It did not
work as expected:

Two of the topic branches' early parts were already merged in the
upstream. The instruction sheet had only 'pick' of merge commits for the
topics. Except for these two; there, all commits (that were not yet in
upstream) were offered to pick, including the merge commit.

I started with this:

    A--M--o--o   <- master
   /  /
--o--X--Y        <- side branch (partially merged in master)
   \     \
    R--S--N--T   <- integration (to be rebuilt on master)

I wanted this:

     A--M--o--o
    /  /       \
   /  /          R'--S'--N'--T'
--o--X--Y---------------´

But I got this:

    A--M--o--o-------Y'
   /  /       \       \
--o--X--Y      R'--S'--N'--T'

(Note that this has nothing to do with my patch; the badness happens
already before any rebasing begins.)

Gah! I'm frustrated. When --preserve-merges was invented, it supported
two very important use-cases:

1. Rebuild an integration branch.
2. Rebase a topic that merges an 'unrelated side branch'.

Then people came along thinking that "preserve merges" means that *any*
sort of merges should be preserved, including a branchy-and-mergy topic
like the example you gave. *Of course* it is much more difficult to
support this case. And sure as hell with all the work-arounds needed to
support it, a good deal of other good functionality became broken
subtly. This is why I say that we should drop support for the
complicated cases and resurrect correctness for the simpler, but
important cases.

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-24 20:32           ` Johannes Sixt
@ 2012-05-24 21:34             ` Junio C Hamano
  2012-05-25 15:58               ` Johannes Sixt
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-05-24 21:34 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Martin von Zweigbergk, Git Mailing List, Stephen Haberman,
	Andrew Wong

Johannes Sixt <j6t@kdbg.org> writes:

> Today I was able to use rebase -i -p in the field. I used it to rebuild
> an integration branch (akin to git's pu branch). Guess what? It did not
> work as expected:
>
> Two of the topic branches' early parts were already merged in the
> upstream. The instruction sheet had only 'pick' of merge commits for the
> topics. Except for these two; there, all commits (that were not yet in
> upstream) were offered to pick, including the merge commit.
>
> I started with this:
>
>     A--M--o--o   <- master
>    /  /
> --o--X--Y        <- side branch (partially merged in master)
>    \     \
>     R--S--N--T   <- integration (to be rebuilt on master)
>
> I wanted this:
>
>      A--M--o--o
>     /  /       \
>    /  /          R'--S'--N'--T'
> --o--X--Y---------------´

It is unclear what exact revisions you gave to rebase, but I am assuming
that you asked "rebase --onto master X^ T" (you can use R^ instead of X^;
they refer to the same commit).

It is straightforward to see what should happen to R and S; they should be
a straight replay of single parent commit on top of 'master'.  

But it is unclear what should happen to X and Y.

When you are rebasing the X^..T sub-DAG, can you say what makes S more
special than Y by looking at the original topology?  Your "I wanted this"
picture depicts S' to be rewritten but Y stays the same.  Why?  

They are both in the X^..T DAG, and neither of them is merged to 'master'.
I can sort of see why X and R would be treated differently (X is part of
master, R is not), but I cannot justify why your "I wanted this" picture
replays S' without replaying Y'.

What am I missing?

> But I got this:
>
>     A--M--o--o-------Y'
>    /  /       \       \
> --o--X--Y      R'--S'--N'--T'

Which is what I would expect, from the "Y and S play a similar role in the
sub-DAG X^..T in the original DAG, with respect to master" point of view.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-24 21:34             ` Junio C Hamano
@ 2012-05-25 15:58               ` Johannes Sixt
  2012-05-25 16:58                 ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Sixt @ 2012-05-25 15:58 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Martin von Zweigbergk, Git Mailing List, Stephen Haberman,
	Andrew Wong

Am 24.05.2012 23:34, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Today I was able to use rebase -i -p in the field. I used it to rebuild
>> an integration branch (akin to git's pu branch). Guess what? It did not
>> work as expected:
>>
>> Two of the topic branches' early parts were already merged in the
>> upstream. The instruction sheet had only 'pick' of merge commits for the
>> topics. Except for these two; there, all commits (that were not yet in
>> upstream) were offered to pick, including the merge commit.
>>
>> I started with this:
>>
>>     A--M--o--o   <- master
>>    /  /
>> --o--X--Y        <- side branch (partially merged in master)
>>    \     \
>>     R--S--N--T   <- integration (to be rebuilt on master)
>>
>> I wanted this:
>>
>>      A--M--o--o
>>     /  /       \
>>    /  /          R'--S'--N'--T'
>> --o--X--Y---------------´
> 
> It is unclear what exact revisions you gave to rebase, but I am assuming
> that you asked "rebase --onto master X^ T" (you can use R^ instead of X^;
> they refer to the same commit).

Sorry, I should have been more precise. The command was

  git rebase -i -p master T

An important detail is that the order of parents of N is S, Y. (I
assumed that this was somewhat clear from the context of my message.)

> It is straightforward to see what should happen to R and S; they should be
> a straight replay of single parent commit on top of 'master'.  
> 
> But it is unclear what should happen to X and Y.

Nothing. Relabel "integration" to "topic" in my artwork above. Then X
and Y are on an "unrelated side branch" that is merged into the topic
whose tip is at T. The early part of this side branch is already in
master.  In fact, if X were not merged into master, yet, then 'rebase -i
-p' already works as (I) intended: it leaves X and Y alone.

> When you are rebasing the X^..T sub-DAG, can you say what makes S more
> special than Y by looking at the original topology?  Your "I wanted this"
> picture depicts S' to be rewritten but Y stays the same.  Why?  

It is "not on topic" (pun intended). It is the second parent of N. Think
of X and Y as an independent bug fix sub-topic. It is merged into topic
only because T depends on the fix.

> They are both in the X^..T DAG, and neither of them is merged to 'master'.
> I can sort of see why X and R would be treated differently (X is part of
> master, R is not), but I cannot justify why your "I wanted this" picture
> replays S' without replaying Y'.
> 
> What am I missing?

First-parentship. When a topic or an integration branch is rebased (with
--preserve-merges), only the first-parent chain should be rewritten.

>> But I got this:
>>
>>     A--M--o--o-------Y'
>>    /  /       \       \
>> --o--X--Y      R'--S'--N'--T'
> 
> Which is what I would expect, from the "Y and S play a similar role in the
> sub-DAG X^..T in the original DAG, with respect to master" point of view.

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-25 15:58               ` Johannes Sixt
@ 2012-05-25 16:58                 ` Junio C Hamano
  2012-05-25 20:03                   ` Johannes Sixt
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-05-25 16:58 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Martin von Zweigbergk, Git Mailing List, Stephen Haberman,
	Andrew Wong

Johannes Sixt <j6t@kdbg.org> writes:

> First-parentship. When a topic or an integration branch is rebased (with
> --preserve-merges), only the first-parent chain should be rewritten.

While I think the "start from commit T and down to where it branches off
of 'master', rebuild first-parent chain on top of elsewhere" mode has
uses, there is no way it can co-exist under the same option name with the
current "transplant the DAG as a whole on top of elsewhere, preserving the
topology inside the DAG" mode.  The name "preserve merges" clicks with the
latter mode better than the former mode, at least to me.  Perhaps the
other mode can be called "rebase --first-parent" or something?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes
  2012-05-25 16:58                 ` Junio C Hamano
@ 2012-05-25 20:03                   ` Johannes Sixt
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Sixt @ 2012-05-25 20:03 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Martin von Zweigbergk, Git Mailing List, Stephen Haberman,
	Andrew Wong

Am 25.05.2012 18:58, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> First-parentship. When a topic or an integration branch is rebased (with
>> --preserve-merges), only the first-parent chain should be rewritten.
> 
> While I think the "start from commit T and down to where it branches off
> of 'master', rebuild first-parent chain on top of elsewhere" mode has
> uses, there is no way it can co-exist under the same option name with the
> current "transplant the DAG as a whole on top of elsewhere, preserving the
> topology inside the DAG" mode.

Fair enough.

>  The name "preserve merges" clicks with the
> latter mode better than the former mode, at least to me.  Perhaps the
> other mode can be called "rebase --first-parent" or something?

Sounds good. I'll take this route.

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-05-25 20:03 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-21 20:19 [PATCH/RFC] rebase -p: do not redo the merge, but cherry-pick first-parent changes Johannes Sixt
2012-05-22 18:23 ` Junio C Hamano
2012-05-22 19:30   ` Johannes Sixt
2012-05-22 23:38 ` Jonathan Nieder
2012-05-23 15:37 ` Martin von Zweigbergk
2012-05-23 18:53   ` Junio C Hamano
2012-05-23 20:41     ` Martin von Zweigbergk
2012-05-24 17:31       ` Junio C Hamano
2012-05-24 17:47         ` Martin von Zweigbergk
2012-05-24 20:09           ` Junio C Hamano
2012-05-24 20:32           ` Johannes Sixt
2012-05-24 21:34             ` Junio C Hamano
2012-05-25 15:58               ` Johannes Sixt
2012-05-25 16:58                 ` Junio C Hamano
2012-05-25 20:03                   ` Johannes Sixt
2012-05-23 18:59   ` Johannes Sixt

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).