[PATCH 0/2] set NO_TRIVIAL for custom merge strategies

[PATCH 0/2] set NO_TRIVIAL for custom merge strategies

[Jonathan Nieder]

As noticed at <http://bugs.debian.org/581691>, the example custom
“theirs” strategy does not take effect for trivial merges.
A user-defined “ours”-like strategy would also not be possible without
this change.

Longer term, it would be nice to be able to override the
NO_FAST_FORWARD and NO_TRIVIAL flags for custom strategies in
~/.gitconfig, but let’s wait until we need it.

Thoughts?

Jonathan Nieder (2):
  t7606 (merge-theirs): modernize style
  merge: let custom strategies intervene in trivial merges

 builtin/merge.c         |    1 +
 t/t7606-merge-custom.sh |   96 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 71 insertions(+), 26 deletions(-)

[PATCH 1/2] t7606 (merge-theirs): modernize style

[Jonathan Nieder]

Guard setup commands with test_expect_success, so they are easier
to visually skip over and get to the good part.  While at it:

 - use test_commit for brevity and reproducible object names;

 - use test_cmp instead of using the test builtin to compare the
   result of command substitution, for better output with -v on
   failure.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7606-merge-custom.sh |   66 ++++++++++++++++++++++++++++------------------
 1 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
index 52a451d..82045cd 100755
--- a/t/t7606-merge-custom.sh
+++ b/t/t7606-merge-custom.sh
@@ -1,46 +1,60 @@
 #!/bin/sh
 
-test_description='git merge
+test_description="git merge
 
-Testing a custom strategy.'
+Testing a custom strategy.
+
+*   (HEAD, master) Merge commit 'c2'
+|\
+| * (tag: c2) c2
+* | (tag: c1) c1
+|/
+* (tag: c0) c0
+"
 
 . ./test-lib.sh
 
-cat >git-merge-theirs <<EOF
-#!$SHELL_PATH
-eval git read-tree --reset -u \\\$\$#
-EOF
-chmod +x git-merge-theirs
-PATH=.:$PATH
-export PATH
+test_expect_success 'set up custom strategy' '
+	cat >git-merge-theirs <<-EOF &&
+	#!$SHELL_PATH
+	eval git read-tree --reset -u \\\$\$#
+	EOF
+
+	chmod +x git-merge-theirs &&
+	PATH=.:$PATH &&
+	export PATH
+'
 
 test_expect_success 'setup' '
-	echo c0 >c0.c &&
-	git add c0.c &&
-	git commit -m c0 &&
-	git tag c0 &&
-	echo c1 >c1.c &&
-	git add c1.c &&
-	git commit -m c1 &&
-	git tag c1 &&
-	git reset --hard c0 &&
+	test_commit c0 c0.c &&
+	test_commit c1 c1.c &&
+	git reset --keep c0 &&
 	echo c1c1 >c1.c &&
-	echo c2 >c2.c &&
-	git add c1.c c2.c &&
-	git commit -m c2 &&
-	git tag c2
+	git add c1.c &&
+	test_commit c2 c2.c
 '
 
 test_expect_success 'merge c2 with a custom strategy' '
 	git reset --hard c1 &&
+
+	git rev-parse c1 >head.old &&
+	git rev-parse c2 >second-parent.expected &&
+	git rev-parse c2^{tree} >tree.expected &&
 	git merge -s theirs c2 &&
-	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
-	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
-	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-	test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" &&
+
+	git rev-parse HEAD >head &&
+	git rev-parse HEAD^1 >first-parent &&
+	git rev-parse HEAD^2 >second-parent &&
+	git rev-parse HEAD^{tree} >tree &&
+	git update-index --refresh &&
 	git diff --exit-code &&
 	git diff --exit-code c2 HEAD &&
 	git diff --exit-code c2 &&
+
+	! test_cmp head.old head &&
+	test_cmp head.old first-parent &&
+	test_cmp second-parent.expected second-parent &&
+	test_cmp tree.expected tree &&
 	test -f c0.c &&
 	grep c1c1 c1.c &&
 	test -f c2.c
-- 
1.7.2.1.544.ga752d.dirty

[PATCH 2/2] merge: let custom strategies intervene in trivial merges

[Jonathan Nieder]

As v1.6.1-rc1~294^2 (2008-08-23) explains, custom merge strategies
do not even kick in when the merge is truly trivial.  But they
should, since otherwise a custom “--strategy=theirs” is not useful.

Perhaps custom strategies should not allow fast-forward either.  This
patch does not make that change, since it is less important (because
it is always possible to explicitly use --no-ff).

Reported-by: Yaroslav Halchenko <debian@onerussian.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/merge.c         |    1 +
 t/t7606-merge-custom.sh |   36 +++++++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 37ce4f5..e48e90b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -131,6 +131,7 @@ static struct strategy *get_strategy(const char *name)
 
 	ret = xcalloc(1, sizeof(struct strategy));
 	ret->name = xstrdup(name);
+	ret->attr = NO_TRIVIAL;
 	return ret;
 }
 
diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
index 82045cd..13c2193 100755
--- a/t/t7606-merge-custom.sh
+++ b/t/t7606-merge-custom.sh
@@ -4,11 +4,13 @@ test_description="git merge
 
 Testing a custom strategy.
 
-*   (HEAD, master) Merge commit 'c2'
+*   (HEAD, master) Merge commit 'c3'
 |\
-| * (tag: c2) c2
+| * (tag: c3) c3
 * | (tag: c1) c1
 |/
+| * tag: c2) c2
+|/
 * (tag: c0) c0
 "
 
@@ -31,7 +33,9 @@ test_expect_success 'setup' '
 	git reset --keep c0 &&
 	echo c1c1 >c1.c &&
 	git add c1.c &&
-	test_commit c2 c2.c
+	test_commit c2 c2.c &&
+	git reset --keep c0 &&
+	test_commit c3 c3.c
 '
 
 test_expect_success 'merge c2 with a custom strategy' '
@@ -60,4 +64,30 @@ test_expect_success 'merge c2 with a custom strategy' '
 	test -f c2.c
 '
 
+test_expect_success 'trivial merge with custom strategy' '
+	git reset --hard c1 &&
+
+	git rev-parse c1 >head.old &&
+	git rev-parse c3 >second-parent.expected &&
+	git rev-parse c3^{tree} >tree.expected &&
+	git merge -s theirs c3 &&
+
+	git rev-parse HEAD >head &&
+	git rev-parse HEAD^1 >first-parent &&
+	git rev-parse HEAD^2 >second-parent &&
+	git rev-parse HEAD^{tree} >tree &&
+	git update-index --refresh &&
+	git diff --exit-code &&
+	git diff --exit-code c3 HEAD &&
+	git diff --exit-code c3 &&
+
+	! test_cmp head.old head &&
+	test_cmp head.old first-parent &&
+	test_cmp second-parent.expected second-parent &&
+	test_cmp tree.expected tree &&
+	test -f c0.c &&
+	! test -e c1.c &&
+	test -f c3.c
+'
+
 test_done
-- 
1.7.2.1.544.ga752d.dirty

Re: [PATCH 2/2] merge: let custom strategies intervene in trivial merges

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> As v1.6.1-rc1~294^2 (2008-08-23) explains, custom merge strategies
> do not even kick in when the merge is truly trivial.  But they
> should, since otherwise a custom “--strategy=theirs” is not useful.

I think it is a double-edged sword that trivial merge kicks in for custom
strategies.  If your mind is focused narrowly on "ours" or "theirs",
indeed it may be inconvenient, but on the other hand other custom
strategies may not want to run heavy-weight operations if a trivial merge
is sufficient.

So in the longer term, I suspect that each custom strategy needs to be
able to tell the merge machinery if it wants ff and trivial.