Re: What's cooking in git.git (Apr 2012, #05; Thu, 12)

Linus Torvalds [off-list ref] writes:

So  Ack on Michal's patch, but I think we also should do the appended
equivalent thing for the fast-forward test on top of it (it's
white-space damaged, sorry).

Trivially tested with

    git merge HEAD^ HEAD^^

which did the wrong thing before, and now works.

And it seems to break 6028 ("merge -s ours" and "merge -s subtree"
up-to-date) X-<....

On Mon, Apr 16, 2012 at 10:36 AM, Junio C Hamano [off-list ref] wrote:

And it seems to break 6028 ("merge -s ours" and "merge -s subtree"
up-to-date) X-<....

Ugh. I don't even get that far - I get to t3410, which breaks.

(This is with both Michal's and my patch applied)

Oddly, running that test in verbose mode seems to imply that it's the
*rebase* that succeeds, not the merges in that test. Maybe I'm reading
the test results wrong, I didn't really try to understand the test
itself ;(

                        Linus

On Mon, Apr 16, 2012 at 11:02 AM, Linus Torvalds
[off-list ref] wrote:

Oddly, running that test in verbose mode seems to imply that it's the
*rebase* that succeeds, not the merges in that test. Maybe I'm reading
the test results wrong, I didn't really try to understand the test
itself ;(

Yes, it's the rebase that succeeds. "git log -g" in the trash
directory shows that we ended up successfully rebasing J2:

  commit 5fc34ec1a8ed96664198fefc74121cd052b10861
  Reflog: HEAD@{1} (C O Mitter [off-list ref])
  Reflog message: rebase -i (pick): Merge made by the 'recursive' strategy.
  Author: A U Thor [off-list ref]
  Date:   Thu Apr 7 15:28:13 2005 -0700

      J2

while a successful test will fail that.

However, I don't actually see what changed.

Oh - one thing to note is that the *patch* of that successful rebase
is empty. That may be the big clue: we successfully finish the merge
without noticing that it didn't change any state, and we should have
failed it as an empty commit. Hmm?

                   Linus

Linus Torvalds [off-list ref] wrote:

On Mon, Apr 16, 2012 at 11:02 AM, Linus Torvalds
[off-list ref] wrote:

quoted

Oddly, running that test in verbose mode seems to imply that it's the
*rebase* that succeeds, not the merges in that test. Maybe I'm reading
the test results wrong, I didn't really try to understand the test
itself ;(

Yes, it's the rebase that succeeds. "git log -g" in the trash
directory shows that we ended up successfully rebasing J2:

  commit 5fc34ec1a8ed96664198fefc74121cd052b10861
  Reflog: HEAD@{1} (C O Mitter [off-list ref])
  Reflog message: rebase -i (pick): Merge made by the 'recursive' strategy.
  Author: A U Thor [off-list ref]
  Date:   Thu Apr 7 15:28:13 2005 -0700

      J2

while a successful test will fail that.

However, I don't actually see what changed.

Oh - one thing to note is that the *patch* of that successful rebase
is empty. That may be the big clue: we successfully finish the merge
without noticing that it didn't change any state, and we should have
failed it as an empty commit. Hmm?

                   Linus

So, the difference is that `git merge --no-ff HEAD^` used to work, now
it doesn't because we reduce_heads() only if we allow fast-forward (and
even though there is just one remote we merge with, parents contains
two commits). So what about that trivial patch instead (discarding our
previous patches)?
---

diff --git a/builtin/merge.c b/builtin/merge.c
index 08e01e8..27e0026 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1346,6 +1346,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			allow_trivial = 0;
 	}
 
+	remoteheads = reduce_heads(remoteheads);
+
 	if (!remoteheads->next)
 		common = get_merge_bases(head_commit, remoteheads->item, 1);
 	else {

2012/4/16 Michał Kiedrowicz [off-list ref]:

So, the difference is that `git merge --no-ff HEAD^` used to work, now
it doesn't because we reduce_heads() only if we allow fast-forward (and
even though there is just one remote we merge with, parents contains
two commits). So what about that trivial patch instead (discarding our
previous patches)?

Yes, I suspect this might work.

And I like how reducing the remoteheads should also automatically fix
the case of the commit messages containing redundant information when
you give the same branch multiple times (assuming you have the whole
merge log thing enabled).

                     Linus

This makes us do proper fast-forward merges even for octopus merges,
which could otherwise result in "merge commit" that only had one actual
parent, and should have been a fast-forward.

Odd-case-triggered-by: James Morris [off-list ref]
Signed-off-by: Michał Kiedrowicz <redacted>
---

This is the proposed fix sent as a proper patch with commit message
stolen from Linus and testcases.

I'm not sure if I don't introduce a memleak with the call to
reduce_heads() but other callers seem to not care, just like whole
cmd_merge().

 builtin/merge.c               |    3 +++
 t/t7603-merge-reduce-heads.sh |   19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 08e01e8..2d5930f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1346,6 +1346,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			allow_trivial = 0;
 	}
 
+	if (remoteheads->next)
+		remoteheads = reduce_heads(remoteheads);
+
 	if (!remoteheads->next)
 		common = get_merge_bases(head_commit, remoteheads->item, 1);
 	else {

diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 7e17eb4..a3b08a6 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh

@@ -113,4 +113,23 @@ test_expect_success 'verify merge result' '
 	test $(git rev-parse HEAD^1) = $(git rev-parse E2) &&
 	test $(git rev-parse HEAD^2) = $(git rev-parse I2)
 '
+
+test_expect_success 'fast-forward to redundant refs' '
+	git reset --hard c0 &&
+	git merge c4 c5
+'
+
+test_expect_success 'verify merge result' '
+	test $(git rev-parse HEAD) = $(git rev-parse c5)
+'
+
+test_expect_success 'merge up-to-date redundant refs' '
+	git reset --hard c5 &&
+	git merge c0 c4
+'
+
+test_expect_success 'verify merge result' '
+	test $(git rev-parse HEAD) = $(git rev-parse c5)
+'
+
 test_done

-- 
1.7.8.4