threads / patch / 9973

patch, 5 partsper branch options for git-merge incl. --no-ff

Subject: [PATCH 0/5] per branch options for git-merge incl. --no-ff

## tl;dr

14 messages between Sep 22, 2007 and Sep 23, 2007. Diffs are folded; open one to read it.

replies: 13people: 2as markdown or json

Lars Hjemli· Sep 22, 2007, 00:33 UTC · lore

This patch-series implements support for per branch configuration of git-merge, using entries (branch.<name>.mergeoptions) in .git/config.

There is also a new option, --no-ff, which can be used to force git-merge to create merge commits even when the merge was a fast-forward.

Finally, the new optiond --ff and --commit can be used to override setting specified in .git/config.

Note: patch 1/5 is a resend of the testscript for git-merge, this time
with correct mode bits.
Shortlog:
     Add test-script for git-merge porcelain
     git-merge: refactor option parsing
     git-merge: add support for branch.<name>.mergeoptions
     git-merge: add support for --commit
     git-merge: add --ff and --no-ff options
Diffstat:
 Documentation/config.txt        |    6 +
 Documentation/git-merge.txt     |    4 +
 Documentation/merge-options.txt |   13 ++
 git-merge.sh                    |   56 +++++-
 t/t7600-merge.sh                |  414 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 483 insertions(+), 10 deletions(-)
Lars Hjemli· Sep 22, 2007, 00:33 UTC · re: Lars Hjemli · lore

[PATCH 1/5] Add test-script for git-merge porcelain

This test-script tries to excercise the porcelainish aspects of git-merge.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 t/t7600-merge.sh |  317 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 317 insertions(+), 0 deletions(-)
 create mode 100755 t/t7600-merge.sh
Show changes to t/t7600-merge.sh +317 −0
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
new file mode 100755
index 0000000..9e58636
--- /dev/null
+++ b/t/t7600-merge.sh
@@ -0,0 +1,317 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Lars Hjemli
+#
+
+test_description='git-merge
+
+Testing basic merge operations/option parsing.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo "
+1
+2
+3
+4
+5
+6
+7
+8
+9
+" > file &&
+	git add file &&
+	git commit -m "commit 0" &&
+	git tag c0 &&
+	c0=$(git rev-parse HEAD) &&
+	echo "
+1 X
+2
+3
+4
+5
+6
+7
+8
+9
+" > file &&
+	git add file &&
+	git commit -m "commit 1" &&
+	git tag c1 &&
+	c1=$(git rev-parse HEAD) &&
+	git reset --hard "$c0" &&
+	echo "
+1
+2
+3
+4
+5 X
+6
+7
+8
+9
+" > file &&
+	git add file &&
+	git commit -m "commit 2" &&
+	git tag c2 &&
+	c2=$(git rev-parse HEAD) &&
+	git reset --hard "$c0" &&
+	echo "
+1
+2
+3
+4
+5
+6
+7
+8
+9 X
+" > file &&
+	git add file &&
+	git commit -m "commit 3" &&
+	git tag c3 &&
+	c3=$(git rev-parse HEAD)
+	git reset --hard "$c0"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'test option parsing' '
+	if git merge -$ c1
+	then
+		echo "[OOPS] -$ accepted"
+		false
+	fi &&
+	if git merge --no-such c1
+	then
+		echo "[OOPS] --no-such accepted"
+		false
+	fi &&
+	if git merge -s foobar c1
+	then
+		echo "[OOPS] -s foobar accepted"
+		false
+	fi &&
+	if git merge -s=foobar c1
+	then
+		echo "[OOPS] -s=foobar accepted"
+		false
+	fi &&
+	if git merge -m
+	then
+		echo "[OOPS] missing commit msg accepted"
+		false
+	fi &&
+	if git merge
+	then
+		echo "[OOPS] missing commit references accepted"
+		false
+	fi
+'
+
+test_expect_success 'merge c0 with c1' '
+	git reset --hard c0 &&
+	git merge c1 &&
+	test "$c1" = "$(git rev-parse HEAD)"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'verify merge result' '
+	echo "
+1 X
+2
+3
+4
+5
+6
+7
+8
+9
+" > result.1 &&
+	cmp -s file result.1
+'
+
+test_expect_success 'merge c1 with c2' '
+	git reset --hard c1 &&
+	git merge c2 &&
+	test "$c1" = "$(git rev-parse HEAD^1)" &&
+	test "$c2" = "$(git rev-parse HEAD^2)"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'verify merge result' '
+	echo "
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9
+" > result.1-5 &&
+	cmp -s file result.1-5
+'
+
+test_expect_success 'merge c1 with c2 and c3' '
+	git reset --hard c1 &&
+	git merge c2 c3 &&
+	test "$c1" = "$(git rev-parse HEAD^1)" &&
+	test "$c2" = "$(git rev-parse HEAD^2)" &&
+	test "$c3" = "$(git rev-parse HEAD^3)"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'verify merge result' '
+	echo "
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9 X
+" > result.1-5-9 &&
+	cmp -s file result.1-5-9
+'
+
+test_expect_success 'merge c0 with c1 (no-commit)' '
+	git reset --hard c0 &&
+	git merge --no-commit c1 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] fast-forward not performed"
+		false
+	fi &&
+	if ! cmp -s file result.1
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 (no-commit)' '
+	git reset --hard c1 &&
+	git merge --no-commit c2 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD changed"
+		false
+	fi &&
+	if ! cmp -s file result.1-5
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test "$c2" != "$(cat .git/MERGE_HEAD)"
+	then
+		echo "[OOPS] MERGE_HEAD is wrong"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 and c3 (no-commit)' '
+	git reset --hard c1 &&
+	git merge --no-commit c2 c3 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD changed"
+		false
+	fi &&
+	if ! cmp -s file result.1-5-9
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if ! grep -q "$c2" .git/MERGE_HEAD
+	then
+		echo "[OOPS] c2 not in MERGE_HEAD"
+		false
+	fi &&
+	if ! grep -q "$c3" .git/MERGE_HEAD
+	then
+		echo "[OOPS] c3 not in MERGE_HEAD"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (squash)' '
+	git reset --hard c0 &&
+	git merge --squash c1 &&
+	if test "$c0" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD changed"
+		false
+	fi &&
+	if ! cmp -s file result.1
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD exists"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 (squash)' '
+	git reset --hard c1 &&
+	git merge --squash c2 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] new commit created"
+		false
+	fi &&
+	if ! cmp -s file result.1-5
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD exists"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 and c3 (squash)' '
+	git reset --hard c1 &&
+	git merge --squash c2 c3 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD changed"
+		false
+	fi &&
+	if ! cmp -s file result.1-5-9
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD exists"
+		false
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_done
-- 
1.5.3.2.82.g75c8d
Lars Hjemli· Sep 22, 2007, 00:33 UTC · re: Lars Hjemli · lore

[PATCH 2/5] git-merge: refactor option parsing

Move the option parsing into a separate function as preparation for reuse by the next commit.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 git-merge.sh |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)
Show changes to git-merge.sh +14 −6
diff --git a/git-merge.sh b/git-merge.sh
index 3a01db0..a3ef676 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -119,11 +119,7 @@ merge_name () {
 	fi
 }
 
-case "$#" in 0) usage ;; esac
-
-have_message=
-while case "$#" in 0) break ;; esac
-do
+parse_option () {
 	case "$1" in
 	-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\
 		--no-summa|--no-summar|--no-summary)
@@ -166,9 +162,21 @@ do
 		have_message=t
 		;;
 	-*)	usage ;;
-	*)	break ;;
+	*)	return 1 ;;
 	esac
 	shift
+	args_left="$#"
+}
+
+case "$#" in 0) usage ;; esac
+
+have_message=
+while parse_option "$@"
+do
+	while test $args_left -lt $#
+	do
+		shift
+	done
 done
 
 if test -z "$show_diffstat"; then
-- 
1.5.3.2.82.g75c8d
Lars Hjemli· Sep 22, 2007, 00:33 UTC · re: Lars Hjemli · lore

[PATCH 3/5] git-merge: add support for branch.<name>.mergeoptions

This enables per branch configuration of merge options. Currently, the most useful options to specify per branch are --squash, --summary/--no-summary and possibly --strategy, but all options are supported.

Note: Options containing whitespace will _not_ be handled correctly. Luckily,
the only option which can include whitespace is --message and it doesn't
make much sense to give that option a default value.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/config.txt    |    6 ++++
 Documentation/git-merge.txt |    4 +++
 git-merge.sh                |   16 +++++++++++
 t/t7600-merge.sh            |   63 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 0 deletions(-)
Show changes to 4 files +89 −0

Documentation/config.txt, Documentation/git-merge.txt, git-merge.sh, t/t7600-merge.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015910f..d3c25f3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -337,6 +337,12 @@ branch.<name>.merge::
 	branch.<name>.merge to the desired branch, and use the special setting
 	`.` (a period) for branch.<name>.remote.
 
+branch.<name>.mergeoptions::
+	Sets default options for merging into branch <name>. The syntax and
+	supported options are equal to that of gitlink:git-merge[1], but
+	option values containing whitespace characters are currently not
+	supported.
+
 clean.requireForce::
 	A boolean to make git-clean do nothing unless given -f or -n.  Defaults
 	to false.
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 144bc16..b1771a1 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -58,6 +58,10 @@ merge.verbosity::
 	above outputs debugging information.  The default is level 2.
 	Can be overriden by 'GIT_MERGE_VERBOSITY' environment variable.
 
+branch.<name>.mergeoptions::
+	Sets default options for merging into branch <name>. The syntax and
+	supported options are equal to that of git-merge, but option values
+	containing whitespace characters are currently not supported.
 
 HOW MERGE WORKS
 ---------------
diff --git a/git-merge.sh b/git-merge.sh
index a3ef676..39a24ac 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -168,9 +168,25 @@ parse_option () {
 	args_left="$#"
 }
 
+parse_config () {
+	while test "$#" != "0"
+	do
+		parse_option "$@" || usage
+		while test $args_left -lt $#
+		do
+			shift
+		done
+	done
+}
+
 case "$#" in 0) usage ;; esac
 
 have_message=
+
+branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
+mergeopts=$(git config "branch.$branch.mergeoptions")
+parse_config $mergeopts
+
 while parse_option "$@"
 do
 	while test $args_left -lt $#
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9e58636..e99b5d9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -314,4 +314,67 @@ test_expect_success 'merge c1 with c2 and c3 (squash)' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2 (no-commit in config)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--no-commit" &&
+	git merge c2 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD changed"
+		false
+	fi &&
+	if ! cmp -s file result.1-5
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test "$c2" != "$(cat .git/MERGE_HEAD)"
+	then
+		echo "[OOPS] MERGE_HEAD is wrong"
+		false
+	fi
+'
+
+test_expect_success 'merge c1 with c2 (squash in config)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--squash" &&
+	git merge c2 &&
+	if test "$c1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] new commit created"
+		false
+	fi &&
+	if ! cmp -s file result.1-5
+	then
+		echo "[OOPS] merge result is wrong"
+		false
+	fi &&
+	if test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD exists"
+		false
+	fi
+'
+
+test_expect_success 'override config option -n' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "-n" &&
+	git merge --summary c2 >diffstat.txt &&
+	if ! grep -e "^ file | \+2 +-$" diffstat.txt
+	then
+		echo "[OOPS] diffstat was not generated"
+	fi
+'
+
+test_expect_success 'override config option --summary' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--summary" &&
+	git merge -n c2 >diffstat.txt &&
+	if grep -e "^ file | \+2 +-$" diffstat.txt
+	then
+		echo "[OOPS] diffstat was generated"
+		false
+	fi
+'
+
 test_done
-- 
1.5.3.2.82.g75c8d
Lars Hjemli· Sep 22, 2007, 00:33 UTC · re: Lars Hjemli · lore

[PATCH 4/5] git-merge: add support for --commit

This option can be used to override --no-commit and --squash. The change also introduces slightly different behavior for --no-commit: when specified, it explicitly overrides --squash.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/merge-options.txt |    4 ++++
 git-merge.sh                    |    6 ++++--
 t/t7600-merge.sh                |   16 ++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)
Show changes to 3 files +24 −2

Documentation/merge-options.txt, git-merge.sh, t/t7600-merge.sh

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index d64c259..9be90b5 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -10,6 +10,10 @@
 	not autocommit, to give the user a chance to inspect and
 	further tweak the merge result before committing.
 
+--commit::
+	Perform the merge and commit the result. This option can
+	be used to override --no-commit and --squash.
+
 --squash::
 	Produce the working tree and index state as if a real
 	merge happened, but do not actually make a commit or
diff --git a/git-merge.sh b/git-merge.sh
index 39a24ac..73ff130 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Junio C Hamano
 #
 
-USAGE='[-n] [--summary] [--no-commit] [--squash] [-s <strategy>] [-m=<merge-message>] <commit>+'
+USAGE='[-n] [--summary] [--[no-]commit] [--squash] [-s <strategy>] [-m=<merge-message>] <commit>+'
 
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
@@ -128,8 +128,10 @@ parse_option () {
 		show_diffstat=t ;;
 	--sq|--squ|--squa|--squas|--squash)
 		squash=t no_commit=t ;;
+	--c|--co|--com|--comm|--commi|--commit)
+		squash= no_commit= ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
-		no_commit=t ;;
+		squash= no_commit=t ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e99b5d9..75b0ee4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -377,4 +377,20 @@ test_expect_success 'override config option --summary' '
 	fi
 '
 
+test_expect_success 'merge c1 with c2 (override --no-commit)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--no-commit" &&
+	git merge --commit c2 &&
+	test "$c1" = "$(git rev-parse HEAD^1)" &&
+	test "$c2" = "$(git rev-parse HEAD^2)"
+'
+
+test_expect_success 'merge c1 with c2 (override --squash)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--squash" &&
+	git merge --commit c2 &&
+	test "$c1" = "$(git rev-parse HEAD^1)" &&
+	test "$c2" = "$(git rev-parse HEAD^2)"
+'
+
 test_done
-- 
1.5.3.2.82.g75c8d
Lars Hjemli· Sep 22, 2007, 00:33 UTC · re: Lars Hjemli · lore

[PATCH 5/5] git-merge: add --ff and --no-ff options

These new options can be used to control the policy for fast-forward merges: --ff allows it (this is the default) while --no-ff will create a merge commit.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/merge-options.txt |    9 +++++++++
 git-merge.sh                    |   20 +++++++++++++++-----
 t/t7600-merge.sh                |   18 ++++++++++++++++++
 3 files changed, 42 insertions(+), 5 deletions(-)
Show changes to 3 files +42 −5

Documentation/merge-options.txt, git-merge.sh, t/t7600-merge.sh

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 9be90b5..b6c4dc4 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -23,6 +23,15 @@
 	top of the current branch whose effect is the same as
 	merging another branch (or more in case of an octopus).
 
+--no-ff::
+	Generate a merge commit even if the merge resolved as a
+	fast-forward.
+
+--ff::
+	Do not generate a merge commit if the merge resolved as
+	a fast-forward, only update the branch pointer. This is
+	the default behavior of git-merge.
+
 -s <strategy>, \--strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
 	once to specify them in the order they should be tried.
diff --git a/git-merge.sh b/git-merge.sh
index 73ff130..daa95a1 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Junio C Hamano
 #
 
-USAGE='[-n] [--summary] [--[no-]commit] [--squash] [-s <strategy>] [-m=<merge-message>] <commit>+'
+USAGE='[-n] [--summary] [--[no-]commit] [--squash] [--[no-]ff] -s <strategy>] [-m=<merge-message>] <commit>+'
 
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
@@ -127,11 +127,15 @@ parse_option () {
 	--summary)
 		show_diffstat=t ;;
 	--sq|--squ|--squa|--squas|--squash)
-		squash=t no_commit=t ;;
+		allow_fast_forward=t squash=t no_commit=t ;;
 	--c|--co|--com|--comm|--commi|--commit)
-		squash= no_commit= ;;
+		allow_fast_forward=t squash= no_commit= ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
-		squash= no_commit=t ;;
+		allow_fast_forward=t squash= no_commit=t ;;
+	--ff)
+		allow_fast_forward=t squash= no_commit= ;;
+	--no-ff)
+		allow_fast_forward=false squash= no_commit= ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
@@ -470,7 +474,13 @@ done
 # auto resolved the merge cleanly.
 if test '' != "$result_tree"
 then
-    parents=$(git show-branch --independent "$head" "$@" | sed -e 's/^/-p /')
+    if test "$allow_fast_forward" = "t"
+    then
+        parents=$(git show-branch --independent "$head" "$@")
+    else
+        parents=$(git rev-parse "$head" "$@")
+    fi
+    parents=$(echo "$parents" | sed -e 's/^/-p /')
     result_commit=$(printf '%s\n' "$merge_msg" | git commit-tree $result_tree $parents) || exit
     finish "$result_commit" "Merge made by $wt_strategy."
     dropsave
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 75b0ee4..6e16a28 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -393,4 +393,22 @@ test_expect_success 'merge c1 with c2 (override --squash)' '
 	test "$c2" = "$(git rev-parse HEAD^2)"
 '
 
+test_expect_success 'merge c0 with c1 (no-ff)' '
+	git reset --hard c0 &&
+	git merge --no-ff c1 &&
+	test "$c0" = "$(git rev-parse HEAD^1)" &&
+	test "$c1" = "$(git rev-parse HEAD^2)"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions "--no-ff" &&
+	git merge --ff c1 &&
+	test "$c1" = "$(git rev-parse HEAD)"
+'
+
+test_debug 'gitk --all'
+
 test_done
-- 
1.5.3.2.82.g75c8d
Junio C Hamano· Sep 23, 2007, 00:51 UTC · re: Lars Hjemli · lore

Re: [PATCH 4/5] git-merge: add support for --commit

Lars Hjemli <hjemli@gmail.com> writes:
> This option can be used to override --no-commit and --squash. The change
> also introduces slightly different behavior for --no-commit: when specified,
> it explicitly overrides --squash.

Makes me wonder if --no-squash also make sense to override a --squash. Is this really needed? IOW, does it ever make sense to have --no-commit in the configuration?

Lars Hjemli· Sep 23, 2007, 10:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/5] git-merge: add support for --commit

On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > This option can be used to override --no-commit and --squash. The change
> > also introduces slightly different behavior for --no-commit: when specified,
> > it explicitly overrides --squash.
>
> Makes me wonder if --no-squash also make sense to override a --squash.
> Is this really needed?  IOW, does it ever make sense to have --no-commit
> in the configuration?
>

Actually, I did add --no-squash but the semantics (and implementation) was identical to --commit so I decided it wasn't worth it (but maybe it is - just to avoid user confusion?).

-- 
larsh
Junio C Hamano· Sep 23, 2007, 00:51 UTC · re: Lars Hjemli · lore

Re: [PATCH 3/5] git-merge: add support for branch.<name>.mergeoptions

Lars Hjemli <hjemli@gmail.com> writes:
Show 12 quoted lines
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 144bc16..b1771a1 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -58,6 +58,10 @@ merge.verbosity::
>  	above outputs debugging information.  The default is level 2.
>  	Can be overriden by 'GIT_MERGE_VERBOSITY' environment variable.
>  
> +branch.<name>.mergeoptions::
> +	Sets default options for merging into branch <name>. The syntax and
> +	supported options are equal to that of git-merge, but option values
> +	containing whitespace characters are currently not supported.
I think this is a sensible thing to do in general.
> +branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
> +mergeopts=$(git config "branch.$branch.mergeoptions")
> +parse_config $mergeopts
What should happen when your head is detached?
Lars Hjemli· Sep 23, 2007, 10:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] git-merge: add support for branch.<name>.mergeoptions

On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Lars Hjemli <hjemli@gmail.com> writes:
> > +branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
> > +mergeopts=$(git config "branch.$branch.mergeoptions")
> > +parse_config $mergeopts
>
> What should happen when your head is detached?
>

My plan was 'nothing', but I should have tested it (it does work, but also prints an ugly "fatal: ref HEAD is not a symbolic ref").

-- 
larsh
Junio C Hamano· Sep 23, 2007, 19:20 UTC · re: Lars Hjemli · lore

Re: [PATCH 3/5] git-merge: add support for branch.<name>.mergeoptions

"Lars Hjemli" <hjemli@gmail.com> writes:
Show 11 quoted lines
> On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Lars Hjemli <hjemli@gmail.com> writes:
>> > +branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
>> > +mergeopts=$(git config "branch.$branch.mergeoptions")
>> > +parse_config $mergeopts
>>
>> What should happen when your head is detached?
>>
>
> My plan was 'nothing', but I should have tested it (it does work, but
> also prints an ugly "fatal: ref HEAD is not a symbolic ref").

That, and also running "git config branch..mergeoptions" and expect it does not change behaviour to issue more strict warning, are both not so good. Perhaps the code needs to be more defensive like:

    if branch=$(git symbolic-ref -q HEAD)
    then
        mergeopts=$(git config "branch.${branch#refs/heads/}.mergeoptions")
	if test -n "$mergeopts"
        then
	    parse_config $mergeopts
	fi
    fi
 
Lars Hjemli· Sep 23, 2007, 19:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] git-merge: add support for branch.<name>.mergeoptions

On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> "Lars Hjemli" <hjemli@gmail.com> writes:
>
> > On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
> >> Lars Hjemli <hjemli@gmail.com> writes:
> >> > +branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
> >> > +mergeopts=$(git config "branch.$branch.mergeoptions")
> >> > +parse_config $mergeopts
> >>
> >> What should happen when your head is detached?
> >>
> >
> > My plan was 'nothing', but I should have tested it (it does work, but
> > also prints an ugly "fatal: ref HEAD is not a symbolic ref").
>
> That, and also running "git config branch..mergeoptions" and
> expect it does not change behaviour to issue more strict
> warning, are both not so good.  Perhaps the code needs to be
> more defensive like:
>
>     if branch=$(git symbolic-ref -q HEAD)
>     then
>         mergeopts=$(git config "branch.${branch#refs/heads/}.mergeoptions")
>         if test -n "$mergeopts"
>         then
>             parse_config $mergeopts
>         fi
>     fi
Yes, this is much better (and the ${parameter#word} syntax was nice too).
Thanks.

-- larsh

Junio C Hamano· Sep 23, 2007, 00:51 UTC · re: Lars Hjemli · lore

Re: [PATCH 1/5] Add test-script for git-merge porcelain

Lars Hjemli <hjemli@gmail.com> writes:
> This test-script tries to excercise the porcelainish aspects of git-merge.
It does exercise; no need for "tries to" ;-).
Show 14 quoted lines
> +test_expect_success 'verify merge result' '
> +	echo "
> +1 X
> +2
> +3
> +4
> +5
> +6
> +7
> +8
> +9
> +" > result.1 &&
> +	cmp -s file result.1
> +'

Unless there is a compelling reason otherwise, I'd prefer tests with a test vector like this to be spelled like this:

	diff -u result.1 file

That is, compare to show the difference actual output might have from the expected result. It's easier to spot the difference when you later break things this way.

Show 6 quoted lines
> +test_expect_success 'merge c1 with c2' '
> +	git reset --hard c1 &&
> +	git merge c2 &&
> +	test "$c1" = "$(git rev-parse HEAD^1)" &&
> +	test "$c2" = "$(git rev-parse HEAD^2)"
> +'
We might also want to test:
 - the index is merged;
 - the working tree matches the index;
 - the merge message (e.g. "git show -s --pretty=format:%s
   HEAD") is as expected;
Otherwise I think it is a good idea to add these tests.

By the way, I think squash_message() leaves a wrong message template for an Octopus, which might be worth fixing.

Lars Hjemli· Sep 23, 2007, 10:24 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/5] Add test-script for git-merge porcelain

On 9/23/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > This test-script tries to excercise the porcelainish aspects of git-merge.
>
> It does exercise; no need for "tries to" ;-).
Heh, ok
Show 11 quoted lines
> > +     cmp -s file result.1
> > +'
>
> Unless there is a compelling reason otherwise, I'd prefer tests
> with a test vector like this to be spelled like this:
>
>         diff -u result.1 file
>
> That is, compare to show the difference actual output might have
> from the expected result.  It's easier to spot the difference
> when you later break things this way.
Good point, I'll change it.
Show 16 quoted lines
>
> > +test_expect_success 'merge c1 with c2' '
> > +     git reset --hard c1 &&
> > +     git merge c2 &&
> > +     test "$c1" = "$(git rev-parse HEAD^1)" &&
> > +     test "$c2" = "$(git rev-parse HEAD^2)"
> > +'
>
> We might also want to test:
>
>  - the index is merged;
>
>  - the working tree matches the index;
>
>  - the merge message (e.g. "git show -s --pretty=format:%s
>    HEAD") is as expected;
Yes, sorry for being lazy ;-)  I'll add more thorough verification
Show 5 quoted lines
>
> Otherwise I think it is a good idea to add these tests.
>
> By the way, I think squash_message() leaves a wrong message
> template for an Octopus, which might be worth fixing.
Ok, I'll try to detect it in the test-script and send a proper fix.
Thanks for the review.
-- 
larsh

← back to recent threads