threads / patch / 24752

patch, 2 partsset NO_TRIVIAL for custom merge strategies

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

## tl;dr

4 messages between Aug 16, 2010 and Aug 16, 2010. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jonathan Nieder· Aug 16, 2010, 01:06 UTC · lore

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

Jonathan Nieder· Aug 16, 2010, 01:08 UTC · re: Jonathan Nieder · lore

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

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

Show changes to t/t7606-merge-custom.sh +40 −26

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

Jonathan Nieder· Aug 16, 2010, 01:11 UTC · re: Jonathan Nieder · lore

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

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

Show changes to 2 files +34 −3

builtin/merge.c, t/t7606-merge-custom.sh

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

Junio C Hamano· Aug 16, 2010, 03:39 UTC · re: Jonathan Nieder · lore

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

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.