threads / patch / 27241

patchAdd default merge options for all branches

Subject: [PATCH 2] Add default merge options for all branches

## tl;dr

46 messages between May 2, 2011 and May 9, 2011. Diffs are folded; open one to read it.

replies: 45people: 8as markdown or json

Michael Grubb· May 2, 2011, 19:23 UTC · lore

Add support for branch.*.mergeoptions for setting default options for all branches. This new value shares semantics with the existing branch.<name>.mergeoptions variable. If a branch specific value is found, that value will be used.

The need for this arises from the fact that there is currently not an easy way to set merge options for all branches. Instead of having to specify merge options for each individual branch there should be a way to set defaults for all branches and then override a specific branch's options.

The approach taken is to make note of whether a branch specific mergeoptions key has been seen and only apply the global value if it hasn't.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 Documentation/git-merge.txt |    3 +++
 builtin/merge.c             |   27 ++++++++++++++++++++++-----
 t/t7600-merge.sh            |   27 +++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 5 deletions(-)
Show changes to 3 files +52 −5

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
 	Sets default options for merging into branch <name>. The syntax and
 	supported options are the same as those of 'git merge', but option
 	values containing whitespace characters are currently not supported.
+	The special value '*' for <name> may be used to configure default
+	options for all branches.  Values for specific branch names will
+	override the this default.
 
 SEE ALSO
 --------
diff --git a/builtin/merge.c b/builtin/merge.c
index 0bdd19a..9e5b6bd 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -505,9 +505,18 @@ cleanup:
 
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
-	if (branch && !prefixcmp(k, "branch.") &&
-		!prefixcmp(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
+	static int branch_merge_options_set = 0;
+	int merge_option_mode = 0;
+
+	if (!strcmp(k, "branch.*.mergeoptions"))
+		merge_option_mode = 1;
+	else if (branch && !prefixcmp(k, "branch.") &&
+			 !prefixcmp(k + 7, branch) &&
+			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
+		merge_option_mode = 2;
+
+	if ((merge_option_mode == 1 && !branch_merge_options_set) ||
+		  merge_option_mode == 2) {
 		const char **argv;
 		int argc;
 		char *buf;
@@ -515,14 +524,22 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		buf = xstrdup(v);
 		argc = split_cmdline(buf, &argv);
 		if (argc < 0)
-			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
-			    split_cmdline_strerror(argc));
+		{
+			if (merge_option_mode == 1)
+				die(_("Bad merge.mergeoptions string: %s"), 
+					split_cmdline_strerror(argc));
+			else
+				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
+					split_cmdline_strerror(argc));
+		}
 		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
 		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
 		argc++;
 		parse_options(argc, argv, NULL, builtin_merge_options,
 			      builtin_merge_usage, 0);
 		free(buf);
+		if (merge_option_mode == 2)
+			branch_merge_options_set = 1;
 	}
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87d5d78..bfb7348 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (global no-ff)' '
+	git reset --hard c0 &&
+	git config --unset branch.master.mergeoptions &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
+	git reset --hard c0 &&
+	git config --remove-section branch.master &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	git config branch.master.mergeoptions "--ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
-- 
1.7.5
Miklos Vajna· May 2, 2011, 22:47 UTC · re: Michael Grubb · lore

Re: [PATCH 2] Add default merge options for all branches

On Mon, May 02, 2011 at 02:23:49PM -0500, Michael Grubb <devel@dailyvoid.com> wrote:
Show 5 quoted lines
>  		if (argc < 0)
> -			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> -			    split_cmdline_strerror(argc));
> +		{
> +			if (merge_option_mode == 1)

checkpatch.pl from the kernel tree may help you - { belongs to the end of the previous line.

Thanks.
Junio C Hamano· May 2, 2011, 23:36 UTC · re: Michael Grubb · lore

Re: [PATCH 2] Add default merge options for all branches

Michael Grubb <devel@dailyvoid.com> writes:
Show 12 quoted lines
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0bdd19a..9e5b6bd 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -505,9 +505,18 @@ cleanup:
>  
>  static int git_merge_config(const char *k, const char *v, void *cb)
>  {
> -	if (branch && !prefixcmp(k, "branch.") &&
> -		!prefixcmp(k + 7, branch) &&
> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> +	static int branch_merge_options_set = 0;

I prefer to avoid "static int" that you cannot easily clear here. It would make it impossible to call the function twice.

I think it is easily doable by using the callback parameter (cb).

I am also wondering how this will scale, both in the direction of "later it is likely that we would want to support a glob not just '*' here", and also "later it is likely that we would want to support other per-branch variables, not just "mergeoptions" here".

Michael Grubb· May 3, 2011, 05:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 2] Add default merge options for all branches

On 5/2/11 6:36 PM, Junio C Hamano wrote:
Show 20 quoted lines
> Michael Grubb <devel@dailyvoid.com> writes:
> 
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index 0bdd19a..9e5b6bd 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -505,9 +505,18 @@ cleanup:
>>  
>>  static int git_merge_config(const char *k, const char *v, void *cb)
>>  {
>> -	if (branch && !prefixcmp(k, "branch.") &&
>> -		!prefixcmp(k + 7, branch) &&
>> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
>> +	static int branch_merge_options_set = 0;
> 
> I prefer to avoid "static int" that you cannot easily clear here.  It
> would make it impossible to call the function twice.
>
> I think it is easily doable by using the callback parameter (cb).
>
Too right.  I've reworked that bit to use the cb parameter.  With an eye
to the future I've created a new struct for this for future's sake, so
that this one feature doesn't monopolize the cb parameter in the future.
 
> I am also wondering how this will scale, both in the direction of "later
> it is likely that we would want to support a glob not just '*' here", and
> also "later it is likely that we would want to support other per-branch
> variables, not just "mergeoptions" here".

It seems that the easiest way would be to move this particular feature out into it's own function at some point to pave the way for doing more complex things with the branch configurations.

New patch forthcoming.
Show 5 quoted lines
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Michael Grubb· May 3, 2011, 05:38 UTC · re: Michael Grubb · lore

[PATCH v3] Add default merge options for all branches

Add support for branch.*.mergeoptions for setting default options for all branches. This new value shares semantics with the existing branch.<name>.mergeoptions variable. If a branch specific value is found, that value will be used.

The need for this arises from the fact that there is currently not an easy way to set merge options for all branches. Instead of having to specify merge options for each individual branch there should be a way to set defaults for all branches and then override a specific branch's options.

The approach taken is to make note of whether a branch specific mergeoptions key has been seen and only apply the global value if it hasn't.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 Documentation/git-merge.txt |    3 +++
 builtin/merge.c             |   40 +++++++++++++++++++++++++++++++++-------
 t/t7600-merge.sh            |   27 +++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 7 deletions(-)
Show changes to 3 files +63 −7

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
 	Sets default options for merging into branch <name>. The syntax and
 	supported options are the same as those of 'git merge', but option
 	values containing whitespace characters are currently not supported.
+	The special value '*' for <name> may be used to configure default
+	options for all branches.  Values for specific branch names will
+	override the this default.
 
 SEE ALSO
 --------
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..9fe129f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,6 +32,13 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
 
+#define MERGEOPTIONS_DEFAULT (1<<0)
+#define MERGEOPTIONS_BRANCH (1<<1)
+
+struct merge_options_cb {
+	int override_default;
+};
+
 struct strategy {
 	const char *name;
 	unsigned attr;
@@ -505,24 +512,42 @@ cleanup:
 
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
-	if (branch && !prefixcmp(k, "branch.") &&
-		!prefixcmp(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
+	int merge_option_mode = 0;
+	struct merge_options_cb *merge_options =
+		(struct merge_options_cb *)cb;
+
+	if (!strcmp(k, "branch.*.mergeoptions"))
+		merge_option_mode = MERGEOPTIONS_DEFAULT;
+	else if (branch && !prefixcmp(k, "branch.") &&
+			 !prefixcmp(k + 7, branch) &&
+			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
+		merge_option_mode = MERGEOPTIONS_BRANCH;
+
+	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
+		!merge_options->override_default) ||
+		merge_option_mode == MERGEOPTIONS_BRANCH) {
 		const char **argv;
 		int argc;
 		char *buf;
 
 		buf = xstrdup(v);
 		argc = split_cmdline(buf, &argv);
-		if (argc < 0)
-			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
-			    split_cmdline_strerror(argc));
+		if (argc < 0) {
+			if (merge_option_mode == 1)
+				die(_("Bad merge.mergeoptions string: %s"), 
+					split_cmdline_strerror(argc));
+			else
+				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
+					split_cmdline_strerror(argc));
+		}
 		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
 		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
 		argc++;
 		parse_options(argc, argv, NULL, builtin_merge_options,
 			      builtin_merge_usage, 0);
 		free(buf);
+		if (merge_option_mode == MERGEOPTIONS_BRANCH)
+			merge_options->override_default = 1;
 	}
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -987,6 +1012,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
+	struct merge_options_cb merge_options = {0};
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
@@ -1004,7 +1030,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (is_null_sha1(head))
 		head_invalid = 1;
 
-	git_config(git_merge_config, NULL);
+	git_config(git_merge_config, &merge_options);
 
 	/* for color.ui */
 	if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..cea2b31 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (global no-ff)' '
+	git reset --hard c0 &&
+	git config --unset branch.master.mergeoptions &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
+	git reset --hard c0 &&
+	git config --remove-section branch.master &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	git config branch.master.mergeoptions "--ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
-- 
1.7.5
Jonathan Nieder· May 3, 2011, 09:03 UTC · re: Michael Grubb · lore

Re: [PATCH v3] Add default merge options for all branches

Hi,
Michael Grubb wrote:
> Add support for branch.*.mergeoptions for setting default options for
> all branches.  This new value shares semantics with the existing
> branch.<name>.mergeoptions variable. If a branch specific value is
> found, that value will be used.
So in the future one might be able to do things like
	[branch "git-gui/*"]
		mergeoptions = -s subtree
Interesting.
> The need for this arises from the fact that there is currently not an
> easy way to set merge options for all branches.

I'm curious: what merge options/workflows does this tend to be useful for? The above explanation seems a bit abstract (though already convincing).

> The approach taken is to make note of whether a branch specific
> mergeoptions key has been seen and only apply the global value if it
> hasn't.
What happens if the global value is seen first?
On to the code.  Warning: nitpicks ahead.
[...]
Show 7 quoted lines
> +++ b/builtin/merge.c
> @@ -32,6 +32,13 @@
>  #define NO_FAST_FORWARD (1<<2)
>  #define NO_TRIVIAL      (1<<3)
>  
> +#define MERGEOPTIONS_DEFAULT (1<<0)
> +#define MERGEOPTIONS_BRANCH (1<<1)
Are these bitflags?
Show 7 quoted lines
> @@ -505,24 +512,42 @@ cleanup:
>  
>  static int git_merge_config(const char *k, const char *v, void *cb)
>  {
> +	int merge_option_mode = 0;
> +	struct merge_options_cb *merge_options =
> +		(struct merge_options_cb *)cb;
This cast should not needed, I'd think.
[...]
Show 14 quoted lines
> -	if (branch && !prefixcmp(k, "branch.") &&
> -		!prefixcmp(k + 7, branch) &&
> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> +	if (!strcmp(k, "branch.*.mergeoptions"))
> +		merge_option_mode = MERGEOPTIONS_DEFAULT;
> +	else if (branch && !prefixcmp(k, "branch.") &&
> +			 !prefixcmp(k + 7, branch) &&
> +			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
> +		merge_option_mode = MERGEOPTIONS_BRANCH;
> +
> +	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
> +		!merge_options->override_default) ||
> +		merge_option_mode == MERGEOPTIONS_BRANCH) {
>  		const char **argv;

It is hard to see at a glance where the "if" condition ends and the body begins. Why not

	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
	     !merge_options->override_default) ||
	    merge_option_mode == MERGEOPTIONS_BRANCH) {
		const char **argv;
		...
or
	if (merge_option_mode == MERGEOPTIONS_BRANCH ? 1 :
	    merge_option_mode == MERGEOPTIONS_DEFAULT ?
			!merge_options->override_default : 0) {
		const char **argv;
		...
or even
	if (merge_option_mode == MERGEOPTIONS_DEFAULT &&
	    merge_options->override_default)
		merge_option_mode = 0;
	if (merge_option_mode) {
		const char **argv;
		...
?
	    
Show 12 quoted lines
>  		int argc;
>  		char *buf;
>  
>  		buf = xstrdup(v);
>  		argc = split_cmdline(buf, &argv);
> -		if (argc < 0)
> -			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> -			    split_cmdline_strerror(argc));
> +		if (argc < 0) {
> +			if (merge_option_mode == 1)
> +				die(_("Bad merge.mergeoptions string: %s"), 
> +					split_cmdline_strerror(argc));
merge.*.mergeoptions, no?
Show 12 quoted lines
> +			else
> +				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> +					split_cmdline_strerror(argc));
> +		}
>  		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
>  		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
>  		argc++;
>  		parse_options(argc, argv, NULL, builtin_merge_options,
>  			      builtin_merge_usage, 0);
>  		free(buf);
> +		if (merge_option_mode == MERGEOPTIONS_BRANCH)
> +			merge_options->override_default = 1;

Could be clearer to put this next to the code that checks override_default.

[...]
Show 9 quoted lines
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
>  
>  test_debug 'git log --graph --decorate --oneline --all'
>  
> +test_expect_success 'merge c0 with c1 (global no-ff)' '
> +	git reset --hard c0 &&
> +	git config --unset branch.master.mergeoptions &&
Better to make that
	test_might_fail git config --unset ...

so it will still work if earlier tests stop setting that variable.

Show 9 quoted lines
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents $c0 $c1
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'

Yuck. Did anything come of the idea of a --between-tests option to use an arbitrary command here automatically? (Not your fault.)

> +
> +test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
> +	git reset --hard c0 &&
> +	git config --remove-section branch.master &&
Could make sense to use test_might_fail for this one, too.
Show 10 quoted lines
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	git config branch.master.mergeoptions "--ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'
Nice, a clean patch with a few reasonable tests.
With whichever of the changes below make sense,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
---
 builtin/merge.c  |   37 ++++++++++++++++++++-----------------
 t/t7600-merge.sh |    4 ++--
 2 files changed, 22 insertions(+), 19 deletions(-)
Show changes to 2 files +22 −19

builtin/merge.c, t/t7600-merge.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index 9fe129f..7156e92 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,8 +32,8 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
 
-#define MERGEOPTIONS_DEFAULT (1<<0)
-#define MERGEOPTIONS_BRANCH (1<<1)
+#define MERGEOPTIONS_DEFAULT 1
+#define MERGEOPTIONS_BRANCH 2
 
 struct merge_options_cb {
 	int override_default;
@@ -513,8 +513,7 @@ cleanup:
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
 	int merge_option_mode = 0;
-	struct merge_options_cb *merge_options =
-		(struct merge_options_cb *)cb;
+	struct merge_options_cb *merge_options = cb;
 
 	if (!strcmp(k, "branch.*.mergeoptions"))
 		merge_option_mode = MERGEOPTIONS_DEFAULT;
@@ -523,31 +522,35 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
 		merge_option_mode = MERGEOPTIONS_BRANCH;
 
-	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
-		!merge_options->override_default) ||
-		merge_option_mode == MERGEOPTIONS_BRANCH) {
+	/*
+	 * If an applicable [branch "foo"] mergeoptions setting was
+	 * seen already, let it mask the [branch "*"] defaults.
+	 */
+	if (merge_options->override_default &&
+	    merge_option_mode == MERGEOPTIONS_DEFAULT)
+		merge_option_mode = 0;
+
+	if (merge_option_mode == MERGEOPTIONS_BRANCH)
+		merge_options->override_default = 1;
+
+	if (merge_option_mode) {
 		const char **argv;
 		int argc;
 		char *buf;
 
 		buf = xstrdup(v);
 		argc = split_cmdline(buf, &argv);
-		if (argc < 0) {
-			if (merge_option_mode == 1)
-				die(_("Bad merge.mergeoptions string: %s"), 
-					split_cmdline_strerror(argc));
-			else
-				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
-					split_cmdline_strerror(argc));
-		}
+		if (argc < 0)
+			die(_("Bad merge.%s.mergeoptions string: %s"), 
+			    merge_option_mode == MERGEOPTIONS_DEFAULT ?
+							"*" : branch,
+			    split_cmdline_strerror(argc));
 		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
 		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
 		argc++;
 		parse_options(argc, argv, NULL, builtin_merge_options,
 			      builtin_merge_usage, 0);
 		free(buf);
-		if (merge_option_mode == MERGEOPTIONS_BRANCH)
-			merge_options->override_default = 1;
 	}
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index cea2b31..ff807f4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -417,7 +417,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c0 with c1 (global no-ff)' '
 	git reset --hard c0 &&
-	git config --unset branch.master.mergeoptions &&
+	test_might_fail git config --unset branch.master.mergeoptions &&
 	git config "branch.*.mergeoptions" "--no-ff" &&
 	test_tick &&
 	git merge c1 &&
@@ -430,7 +430,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
 	git reset --hard c0 &&
-	git config --remove-section branch.master &&
+	test_might_fail git config --remove-section branch.master &&
 	git config "branch.*.mergeoptions" "--no-ff" &&
 	git config branch.master.mergeoptions "--ff" &&
 	test_tick &&
-- 
1.7.5
Jonathan Nieder· May 3, 2011, 09:49 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] Add default merge options for all branches

Jonathan Nieder wrote:
> Michael Grubb wrote:
Show 5 quoted lines
>> The approach taken is to make note of whether a branch specific
>> mergeoptions key has been seen and only apply the global value if it
>> hasn't.
>
> What happens if the global value is seen first?
[...]
> With whichever of the changes below make sense,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Agh, what I meant is "except as noted above". I am still worried about (and have not carefully reviewed) the following case:

	[branch "*"]
		mergeoptions = --foo
	[branch "topic"]
		mergeoptions = --bar
Does it produce different behavior from
	[branch "topic"]
		mergeoptions = --bar
	[branch "*"]
		mergeoptions = --foo
?  If so, is that a good thing, and is it documented?
Sorry for the lack of clarity.
Michael Grubb· May 3, 2011, 16:46 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] Add default merge options for all branches

On 5/3/11 4:03 AM, Jonathan Nieder wrote:
Show 22 quoted lines
> Hi,
> 
> Michael Grubb wrote:
> 
>> Add support for branch.*.mergeoptions for setting default options for
>> all branches.  This new value shares semantics with the existing
>> branch.<name>.mergeoptions variable. If a branch specific value is
>> found, that value will be used.
> 
> So in the future one might be able to do things like
> 
> 	[branch "git-gui/*"]
> 		mergeoptions = -s subtree
> 
> Interesting.
> 
>> The need for this arises from the fact that there is currently not an
>> easy way to set merge options for all branches.
> 
> I'm curious: what merge options/workflows does this tend to be useful
> for?  The above explanation seems a bit abstract (though already
> convincing).

For myself I've adopted Vincent Driessen's branching model (http://nvie.com/posts/a-successful-git-branching-model/) Which specifically recommends using --no-ff when merging to preserve the history of the existence of a topic branch once those branches are removed. But it would equally be beneficial for folks that want the --log, etc option turned on for every merge.

Show 6 quoted lines
> 
>> The approach taken is to make note of whether a branch specific
>> mergeoptions key has been seen and only apply the global value if it
>> hasn't.
> 
> What happens if the global value is seen first?

I will code a test for this, but if the global option is seen first then those options are used, unless/until a branch specific option is seen which will override the globals. If the branch specific option is seen first then the global options are ignored. (I did test this btw, but I didn't add it in the unit tests, bad me).

Show 13 quoted lines
> 
> On to the code.  Warning: nitpicks ahead.
> 
> [...]
>> +++ b/builtin/merge.c
>> @@ -32,6 +32,13 @@
>>  #define NO_FAST_FORWARD (1<<2)
>>  #define NO_TRIVIAL      (1<<3)
>>  
>> +#define MERGEOPTIONS_DEFAULT (1<<0)
>> +#define MERGEOPTIONS_BRANCH (1<<1)
> 
> Are these bitflags?

I had been using literals in the git_merge_config function but thought better of it and added these defines. I'm not really using them as bitflags right now. Perhaps an enum, or just plain literals are better. I'm not clear what the best practice is in this case, so I'm certainly open to a better/more clear way of doing this.

Show 10 quoted lines
> 
>> @@ -505,24 +512,42 @@ cleanup:
>>  
>>  static int git_merge_config(const char *k, const char *v, void *cb)
>>  {
>> +	int merge_option_mode = 0;
>> +	struct merge_options_cb *merge_options =
>> +		(struct merge_options_cb *)cb;
> 
> This cast should not needed, I'd think.
Indeed.  It has been removed.
Show 19 quoted lines
> 
> [...]
>> -	if (branch && !prefixcmp(k, "branch.") &&
>> -		!prefixcmp(k + 7, branch) &&
>> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
>> +	if (!strcmp(k, "branch.*.mergeoptions"))
>> +		merge_option_mode = MERGEOPTIONS_DEFAULT;
>> +	else if (branch && !prefixcmp(k, "branch.") &&
>> +			 !prefixcmp(k + 7, branch) &&
>> +			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
>> +		merge_option_mode = MERGEOPTIONS_BRANCH;
>> +
>> +	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
>> +		!merge_options->override_default) ||
>> +		merge_option_mode == MERGEOPTIONS_BRANCH) {
>>  		const char **argv;
> 
> It is hard to see at a glance where the "if" condition ends and
> the body begins.  Why not

I completely agree, though believe it or not, this was what the original code looked like so I was following it's lead. Here is what the original looks like:

	if (branch && !prefixcmp(k, "branch.") &&
		!prefixcmp(k + 7, branch) &&
		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
		const char **argv;
		int argc;
		char *buf;
I would, of course, be happy to clear this up.
Show 57 quoted lines
> 
> 	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
> 	     !merge_options->override_default) ||
> 	    merge_option_mode == MERGEOPTIONS_BRANCH) {
> 		const char **argv;
> 		...
> 
> or
> 
> 	if (merge_option_mode == MERGEOPTIONS_BRANCH ? 1 :
> 	    merge_option_mode == MERGEOPTIONS_DEFAULT ?
> 			!merge_options->override_default : 0) {
> 		const char **argv;
> 		...
> 
> or even
> 
> 	if (merge_option_mode == MERGEOPTIONS_DEFAULT &&
> 	    merge_options->override_default)
> 		merge_option_mode = 0;
> 
> 	if (merge_option_mode) {
> 		const char **argv;
> 		...
> 
> ?
> 	    
>>  		int argc;
>>  		char *buf;
>>  
>>  		buf = xstrdup(v);
>>  		argc = split_cmdline(buf, &argv);
>> -		if (argc < 0)
>> -			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
>> -			    split_cmdline_strerror(argc));
>> +		if (argc < 0) {
>> +			if (merge_option_mode == 1)
>> +				die(_("Bad merge.mergeoptions string: %s"), 
>> +					split_cmdline_strerror(argc));
> 
> merge.*.mergeoptions, no?
> 
>> +			else
>> +				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
>> +					split_cmdline_strerror(argc));
>> +		}
>>  		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
>>  		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
>>  		argc++;
>>  		parse_options(argc, argv, NULL, builtin_merge_options,
>>  			      builtin_merge_usage, 0);
>>  		free(buf);
>> +		if (merge_option_mode == MERGEOPTIONS_BRANCH)
>> +			merge_options->override_default = 1;
> 
> Could be clearer to put this next to the code that checks
> override_default.

Yes. I had put this there originally so that it would only be set if there were no errors in parsing the options but after chasing the rabbit down the whole, if there are errors parse_options won't return, so I'll move this up closer to the override_default check.

Show 51 quoted lines
> 
> [...]
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
>>  
>>  test_debug 'git log --graph --decorate --oneline --all'
>>  
>> +test_expect_success 'merge c0 with c1 (global no-ff)' '
>> +	git reset --hard c0 &&
>> +	git config --unset branch.master.mergeoptions &&
> 
> Better to make that
> 
> 	test_might_fail git config --unset ...
> 
> so it will still work if earlier tests stop setting that
> variable.
> 
>> +	git config "branch.*.mergeoptions" "--no-ff" &&
>> +	test_tick &&
>> +	git merge c1 &&
>> +	git config --remove-section "branch.*" &&
>> +	verify_merge file result.1 &&
>> +	verify_parents $c0 $c1
>> +'
>> +
>> +test_debug 'git log --graph --decorate --oneline --all'
> 
> Yuck.  Did anything come of the idea of a --between-tests option to
> use an arbitrary command here automatically?  (Not your fault.)  
> 
>> +
>> +test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
>> +	git reset --hard c0 &&
>> +	git config --remove-section branch.master &&
> 
> Could make sense to use test_might_fail for this one, too.
> 
>> +	git config "branch.*.mergeoptions" "--no-ff" &&
>> +	git config branch.master.mergeoptions "--ff" &&
>> +	test_tick &&
>> +	git merge c1 &&
>> +	git config --remove-section "branch.*" &&
>> +	verify_merge file result.1 &&
>> +	verify_parents "$c0"
>> +'
>> +
>> +test_debug 'git log --graph --decorate --oneline --all'
> 
> Nice, a clean patch with a few reasonable tests.

Thanks for all the constructive criticism, Jonathan. I will make the recommended changes and post a new patch.

Show 106 quoted lines
> 
> With whichever of the changes below make sense,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks.
> ---
>  builtin/merge.c  |   37 ++++++++++++++++++++-----------------
>  t/t7600-merge.sh |    4 ++--
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9fe129f..7156e92 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -32,8 +32,8 @@
>  #define NO_FAST_FORWARD (1<<2)
>  #define NO_TRIVIAL      (1<<3)
>  
> -#define MERGEOPTIONS_DEFAULT (1<<0)
> -#define MERGEOPTIONS_BRANCH (1<<1)
> +#define MERGEOPTIONS_DEFAULT 1
> +#define MERGEOPTIONS_BRANCH 2
>  
>  struct merge_options_cb {
>  	int override_default;
> @@ -513,8 +513,7 @@ cleanup:
>  static int git_merge_config(const char *k, const char *v, void *cb)
>  {
>  	int merge_option_mode = 0;
> -	struct merge_options_cb *merge_options =
> -		(struct merge_options_cb *)cb;
> +	struct merge_options_cb *merge_options = cb;
>  
>  	if (!strcmp(k, "branch.*.mergeoptions"))
>  		merge_option_mode = MERGEOPTIONS_DEFAULT;
> @@ -523,31 +522,35 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
>  		merge_option_mode = MERGEOPTIONS_BRANCH;
>  
> -	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
> -		!merge_options->override_default) ||
> -		merge_option_mode == MERGEOPTIONS_BRANCH) {
> +	/*
> +	 * If an applicable [branch "foo"] mergeoptions setting was
> +	 * seen already, let it mask the [branch "*"] defaults.
> +	 */
> +	if (merge_options->override_default &&
> +	    merge_option_mode == MERGEOPTIONS_DEFAULT)
> +		merge_option_mode = 0;
> +
> +	if (merge_option_mode == MERGEOPTIONS_BRANCH)
> +		merge_options->override_default = 1;
> +
> +	if (merge_option_mode) {
>  		const char **argv;
>  		int argc;
>  		char *buf;
>  
>  		buf = xstrdup(v);
>  		argc = split_cmdline(buf, &argv);
> -		if (argc < 0) {
> -			if (merge_option_mode == 1)
> -				die(_("Bad merge.mergeoptions string: %s"), 
> -					split_cmdline_strerror(argc));
> -			else
> -				die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> -					split_cmdline_strerror(argc));
> -		}
> +		if (argc < 0)
> +			die(_("Bad merge.%s.mergeoptions string: %s"), 
> +			    merge_option_mode == MERGEOPTIONS_DEFAULT ?
> +							"*" : branch,
> +			    split_cmdline_strerror(argc));
>  		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
>  		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
>  		argc++;
>  		parse_options(argc, argv, NULL, builtin_merge_options,
>  			      builtin_merge_usage, 0);
>  		free(buf);
> -		if (merge_option_mode == MERGEOPTIONS_BRANCH)
> -			merge_options->override_default = 1;
>  	}
>  
>  	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index cea2b31..ff807f4 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -417,7 +417,7 @@ test_debug 'git log --graph --decorate --oneline --all'
>  
>  test_expect_success 'merge c0 with c1 (global no-ff)' '
>  	git reset --hard c0 &&
> -	git config --unset branch.master.mergeoptions &&
> +	test_might_fail git config --unset branch.master.mergeoptions &&
>  	git config "branch.*.mergeoptions" "--no-ff" &&
>  	test_tick &&
>  	git merge c1 &&
> @@ -430,7 +430,7 @@ test_debug 'git log --graph --decorate --oneline --all'
>  
>  test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
>  	git reset --hard c0 &&
> -	git config --remove-section branch.master &&
> +	test_might_fail git config --remove-section branch.master &&
>  	git config "branch.*.mergeoptions" "--no-ff" &&
>  	git config branch.master.mergeoptions "--ff" &&
>  	test_tick &&
Junio C Hamano· May 3, 2011, 18:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3] Add default merge options for all branches

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> So in the future one might be able to do things like
>
> 	[branch "git-gui/*"]
> 		mergeoptions = -s subtree
>
> Interesting.
>
>> The need for this arises from the fact that there is currently not an
>> easy way to set merge options for all branches.
>
> I'm curious: what merge options/workflows does this tend to be useful
> for?
I actually am curious myself, too.  I want to see a real-life example.

The "git-gui/*" example you gave is unfortunately not it. It specifies the branch at the wrong end. Whether I am merging into "master" or "next", I would want "-s subtree" when I am merging "git-gui/*" project, but all my merges to "master" or "next" do not necessarily want to use "-s subtree".

It might turn out to be that "branch.<name>.mergeoptions" is a ill-conceived idea to begin with.

Show 5 quoted lines
>> The approach taken is to make note of whether a branch specific
>> mergeoptions key has been seen and only apply the global value if it
>> hasn't.
>
> What happens if the global value is seen first?

If implemented correctly, it should use the specific one and fall back to the wildcard one. Another issue is if the values should be cumulative or overriding, but in the remainder I'd assume we want overriding.

Show 9 quoted lines
>> @@ -505,24 +512,42 @@ cleanup:
>>  
>>  static int git_merge_config(const char *k, const char *v, void *cb)
>>  {
>> +	int merge_option_mode = 0;
>> +	struct merge_options_cb *merge_options =
>> +		(struct merge_options_cb *)cb;
>
> This cast should not needed, I'd think.

Correct. That is the whole point of using (void *) as a parameter, so that it can be assigned to the real expected type easily without cast.

Show 18 quoted lines
>> +	if (!strcmp(k, "branch.*.mergeoptions"))
>> +		merge_option_mode = MERGEOPTIONS_DEFAULT;
>> +	else if (branch && !prefixcmp(k, "branch.") &&
>> +			 !prefixcmp(k + 7, branch) &&
>> +			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
>> +		merge_option_mode = MERGEOPTIONS_BRANCH;
>> +
>> +	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
>> +		!merge_options->override_default) ||
>> +		merge_option_mode == MERGEOPTIONS_BRANCH) {
>>  		const char **argv;
>
> It is hard to see at a glance where the "if" condition ends and
> the body begins.  Why not
> ...
> or even
> ...
> ?

Why not have two string pointers in merge_options_cb structure that are initialized to NULL and holds the matched config key and the value?

When we are looking at a (k, v) pair in this function, if there is no previous key in cb, we store (k, v) in cb and return. If there already is a previous key, we see if k is more specific than that key, and replace (k, v) in cb with what we currently have. Otherwise we do not do anything.

When git_config() returns, the caller will find the final value in cb.
Show 12 quoted lines
>> +	git config "branch.*.mergeoptions" "--no-ff" &&
>> +	test_tick &&
>> +	git merge c1 &&
>> +	git config --remove-section "branch.*" &&
>> +	verify_merge file result.1 &&
>> +	verify_parents $c0 $c1
>> +'
>> +
>> +test_debug 'git log --graph --decorate --oneline --all'
>
> Yuck.  Did anything come of the idea of a --between-tests option to
> use an arbitrary command here automatically?  (Not your fault.)  

I actually think test_debug should go inside the previous test. Why not have it immediately after "git merge c1" above?

Michael Grubb· May 3, 2011, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] Add default merge options for all branches

On 5/3/11 1:16 PM, Junio C Hamano wrote:
Show 17 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
>> So in the future one might be able to do things like
>>
>> 	[branch "git-gui/*"]
>> 		mergeoptions = -s subtree
>>
>> Interesting.
>>
>>> The need for this arises from the fact that there is currently not an
>>> easy way to set merge options for all branches.
>>
>> I'm curious: what merge options/workflows does this tend to be useful
>> for?
> 
> I actually am curious myself, too.  I want to see a real-life example.
> 

In my reply to Jonathan I gave the example of turning off fast forward merges globally for all branches (and then perhaps turning them back on for specific branches). The same could be done with --log or any other option that might make since to turn on for all branches without having to specifically name each branch in the config file.

Also as previously cited in my earlier reply to Jonathan, a good real life example can be found in Vincent Driessen's branching model where he creates ephemeral feature/topic branches for development then deletes them when they get merged back into the mainline development tree. However, he wants to keep the history that there was a merge from a branch instead of it just looking like a straight line. The link to his work is here: http://nvie.com/posts/a-successful-git-branching-model/

Show 18 quoted lines
> The "git-gui/*" example you gave is unfortunately not it. It specifies the
> branch at the wrong end. Whether I am merging into "master" or "next", I
> would want "-s subtree" when I am merging "git-gui/*" project, but all my
> merges to "master" or "next" do not necessarily want to use "-s subtree".
> 
> It might turn out to be that "branch.<name>.mergeoptions" is a
> ill-conceived idea to begin with.
> 
>>> The approach taken is to make note of whether a branch specific
>>> mergeoptions key has been seen and only apply the global value if it
>>> hasn't.
>>
>> What happens if the global value is seen first?
> 
> If implemented correctly, it should use the specific one and fall back to
> the wildcard one.  Another issue is if the values should be cumulative or
> overriding, but in the remainder I'd assume we want overriding.
> 
There is now a unit test for this scenario.
Show 13 quoted lines
>>> @@ -505,24 +512,42 @@ cleanup:
>>>  
>>>  static int git_merge_config(const char *k, const char *v, void *cb)
>>>  {
>>> +	int merge_option_mode = 0;
>>> +	struct merge_options_cb *merge_options =
>>> +		(struct merge_options_cb *)cb;
>>
>> This cast should not needed, I'd think.
> 
> Correct.  That is the whole point of using (void *) as a parameter, so
> that it can be assigned to the real expected type easily without cast.
> 
This has been corrected in the latest version of the patch.
Show 22 quoted lines
>>> +	if (!strcmp(k, "branch.*.mergeoptions"))
>>> +		merge_option_mode = MERGEOPTIONS_DEFAULT;
>>> +	else if (branch && !prefixcmp(k, "branch.") &&
>>> +			 !prefixcmp(k + 7, branch) &&
>>> +			 !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
>>> +		merge_option_mode = MERGEOPTIONS_BRANCH;
>>> +
>>> +	if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
>>> +		!merge_options->override_default) ||
>>> +		merge_option_mode == MERGEOPTIONS_BRANCH) {
>>>  		const char **argv;
>>
>> It is hard to see at a glance where the "if" condition ends and
>> the body begins.  Why not
>> ...
>> or even
>> ...
>> ?
> 
> Why not have two string pointers in merge_options_cb structure that are
> initialized to NULL and holds the matched config key and the value?
> 

That was a great idea. I've reworked things around this and also moved to a voting type method for determining priority of the keys. So no more defines needed. I think keeping all that state in the struct is much better and scales better as well.

Show 23 quoted lines
> When we are looking at a (k, v) pair in this function, if there is no
> previous key in cb, we store (k, v) in cb and return.  If there already is
> a previous key, we see if k is more specific than that key, and replace
> (k, v) in cb with what we currently have.  Otherwise we do not do
> anything.
> 
> When git_config() returns, the caller will find the final value in cb.
> 
>>> +	git config "branch.*.mergeoptions" "--no-ff" &&
>>> +	test_tick &&
>>> +	git merge c1 &&
>>> +	git config --remove-section "branch.*" &&
>>> +	verify_merge file result.1 &&
>>> +	verify_parents $c0 $c1
>>> +'
>>> +
>>> +test_debug 'git log --graph --decorate --oneline --all'
>>
>> Yuck.  Did anything come of the idea of a --between-tests option to
>> use an arbitrary command here automatically?  (Not your fault.)  
> 
> I actually think test_debug should go inside the previous test.  Why not
> have it immediately after "git merge c1" above?

Again I followed the existing pattern here. I didn't want to be the odd man out. Do you want me to make that change?

Show 5 quoted lines
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Jonathan Nieder· May 3, 2011, 20:50 UTC · re: Michael Grubb · lore

Re: [PATCH v3] Add default merge options for all branches

Michael Grubb wrote:
> On 5/3/11 1:16 PM, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
>>> Yuck.  Did anything come of the idea of a --between-tests option to
>>> use an arbitrary command here automatically?  (Not your fault.)  
>>
>> I actually think test_debug should go inside the previous test.  Why not
>> have it immediately after "git merge c1" above?
>
> Again I followed the existing pattern here. I didn't want to be the odd man out.
> Do you want me to make that change?
No, I was just reminding the list at large about how annoying it is.
You did right to mimic the style of surrounding tests.  Thank you.
Jens Lehmann· May 3, 2011, 20:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] Add default merge options for all branches

Am 03.05.2011 20:16, schrieb Junio C Hamano:
Show 8 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> The need for this arises from the fact that there is currently not an
>>> easy way to set merge options for all branches.
>>
>> I'm curious: what merge options/workflows does this tend to be useful
>> for?
> 
> I actually am curious myself, too.  I want to see a real-life example.

At my dayjob the reviewer of a branch does the merge of a topic into master. And to always record who did the review even if the merge is a fast forward we use the "branch.master.mergeoptions=--no-ff" config in all our repos, set by our installer.

But that doesn't work out of the box for our release branches, we have to manually set stuff like "branch.v1.23-4.mergeoptions=--no-ff" for every release branch in every clone where merges are done. This is easy to forget and as we use git gui for our daily work, we can't simply use the command line option --no-ff.

So we would greatly benefit from this patch (and actually I looked into doing it myself some time ago but postponed it to do some submodule stuff first ;-)

Michael Grubb· May 3, 2011, 20:07 UTC · re: Jonathan Nieder · lore

[PATCH v4] Add default merge options for all branches

Add support for branch.*.mergeoptions for setting default options for all branches. This new value shares semantics with the existing branch.<name>.mergeoptions variable. If a branch specific value is found, that value will be used.

The need for this arises from the fact that there is currently not an easy way to set merge options for all branches. Instead of having to specify merge options for each individual branch there should be a way to set defaults for all branches and then override a specific branch's options.

In order to facilitate future features centered around this new "globlike" syntax a new struct has been created to keep track of the branch.*.* options. Currently it only supports branch.*.mergeoptions, but can be easily modified in the future to support other branch specific options as well. The mechanism will "vote" on specific-"ness" of the configuration key and ultimately only use the most specific options. This is not cumulative but overriding.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 Documentation/git-merge.txt |    3 +
 builtin/merge.c             |   90 +++++++++++++++++++++++++++++++++---------
 t/t7600-merge.sh            |   39 ++++++++++++++++++
 3 files changed, 112 insertions(+), 20 deletions(-)
Show changes to 3 files +112 −20

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
 	Sets default options for merging into branch <name>. The syntax and
 	supported options are the same as those of 'git merge', but option
 	values containing whitespace characters are currently not supported.
+	The special value '*' for <name> may be used to configure default
+	options for all branches.  Values for specific branch names will
+	override the this default.
 
 SEE ALSO
 --------
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..d6ce85e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,6 +32,21 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
 
+/* This is for branch.<foo>. blocks
+ * the vote member holds a value between
+ * 0.0 and 1.0 which measures how closely
+ * a branch name matches the key member.
+ * where branch.*.mergeoptions would be 0.1 and
+ * branch.<name>.mergeoptions would be 1.0
+ * Also it is called vote because I couldn't come
+ * up with a better name.
+ */
+struct merge_options_cb {
+	char *key;
+	char *value;
+	float vote;
+};
+
 struct strategy {
 	const char *name;
 	unsigned attr;
@@ -503,28 +518,60 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-static int git_merge_config(const char *k, const char *v, void *cb)
+static void parse_git_merge_options(const char *k, const char *v,
+			void *cb)
 {
-	if (branch && !prefixcmp(k, "branch.") &&
-		!prefixcmp(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
-		const char **argv;
-		int argc;
-		char *buf;
-
-		buf = xstrdup(v);
-		argc = split_cmdline(buf, &argv);
-		if (argc < 0)
-			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
-			    split_cmdline_strerror(argc));
-		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
-		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
-		argc++;
-		parse_options(argc, argv, NULL, builtin_merge_options,
-			      builtin_merge_usage, 0);
-		free(buf);
+	struct merge_options_cb *merge_options = cb;
+	int changed = 0;
+
+	/* We only handle mergeoptions for now */
+	if (suffixcmp(k, ".mergeoptions"))
+		return;
+
+	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
+		merge_options->vote = 0.1;
+		changed = 1;
+	} else if (branch && !prefixcmp(k, "branch.") &&
+				!prefixcmp(k + 7, branch) &&
+				merge_options->vote < 1.0) {
+		merge_options->vote = 1.0;
+		changed = 1;
 	}
 
+	if (changed) {
+		merge_options->key = (char *)k;
+		merge_options->value = (char *)v;
+	}
+}
+
+static void apply_merge_options(struct merge_options_cb *opts)
+{
+	const char **argv;
+	int argc;
+	char *buf;
+
+	if ( opts == NULL )
+		return;
+
+	buf = xstrdup(opts->value);
+	argc = split_cmdline(buf, &argv);
+	if (argc < 0)
+		die(_("Bad %s string: %s"), 
+			opts->key, split_cmdline_strerror(argc));
+
+	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
+	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
+	argc++;
+	parse_options(argc, argv, NULL, builtin_merge_options,
+			  builtin_merge_usage, 0);
+	free(buf);
+}
+
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+	if (cb != NULL && branch && !prefixcmp(k, "branch."))
+		parse_git_merge_options(k, v, cb);
+
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
+	struct merge_options_cb merge_options = {NULL, NULL, 0.0};
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (is_null_sha1(head))
 		head_invalid = 1;
 
-	git_config(git_merge_config, NULL);
+	git_config(git_merge_config, &merge_options);
+	if (merge_options.key != NULL && merge_options.value != NULL)
+		apply_merge_options(&merge_options);
 
 	/* for color.ui */
 	if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..5b1f8e1 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (default no-ff)' '
+	git reset --hard c0 &&
+	test_might_fail git config --unset branch.master.mergeoptions &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
+	git reset --hard c0 &&
+	test_might_fail git config --remove-section branch.master &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	git config branch.master.mergeoptions "--ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
+	git reset --hard c0 &&
+	test_might_fail git config --remove-section branch.master &&
+	git config branch.master.mergeoptions "--ff" &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
-- 
1.7.5
Michael Grubb· May 3, 2011, 20:36 UTC · re: Michael Grubb · lore

Re: [PATCH v4] Add default merge options for all branches

This patch has a trailing whitespace on one line. I'm resubmitting to fix it.

On 5/3/11 3:07 PM, Michael Grubb wrote:
Show 215 quoted lines
> Add support for branch.*.mergeoptions for setting default options for
> all branches.  This new value shares semantics with the existing
> branch.<name>.mergeoptions variable. If a branch specific value is
> found, that value will be used.
> 
> The need for this arises from the fact that there is currently not an
> easy way to set merge options for all branches. Instead of having to
> specify merge options for each individual branch there should be a way
> to set defaults for all branches and then override a specific branch's
> options.
> 
> In order to facilitate future features centered around this new
> "globlike" syntax a new struct has been created to keep track of the
> branch.*.* options.  Currently it only supports branch.*.mergeoptions,
> but can be easily modified in the future to support other branch
> specific options as well. The mechanism will "vote" on specific-"ness"
> of the configuration key and ultimately only use the most specific
> options.  This is not cumulative but overriding.
> 
> Signed-off-by: Michael Grubb <devel@dailyvoid.com>
> ---
>  Documentation/git-merge.txt |    3 +
>  builtin/merge.c             |   90 +++++++++++++++++++++++++++++++++---------
>  t/t7600-merge.sh            |   39 ++++++++++++++++++
>  3 files changed, 112 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index e2e6aba..eaab3e4 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
>  	Sets default options for merging into branch <name>. The syntax and
>  	supported options are the same as those of 'git merge', but option
>  	values containing whitespace characters are currently not supported.
> +	The special value '*' for <name> may be used to configure default
> +	options for all branches.  Values for specific branch names will
> +	override the this default.
>  
>  SEE ALSO
>  --------
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d171c63..d6ce85e 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -32,6 +32,21 @@
>  #define NO_FAST_FORWARD (1<<2)
>  #define NO_TRIVIAL      (1<<3)
>  
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */
> +struct merge_options_cb {
> +	char *key;
> +	char *value;
> +	float vote;
> +};
> +
>  struct strategy {
>  	const char *name;
>  	unsigned attr;
> @@ -503,28 +518,60 @@ cleanup:
>  	strbuf_release(&bname);
>  }
>  
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static void parse_git_merge_options(const char *k, const char *v,
> +			void *cb)
>  {
> -	if (branch && !prefixcmp(k, "branch.") &&
> -		!prefixcmp(k + 7, branch) &&
> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> -		const char **argv;
> -		int argc;
> -		char *buf;
> -
> -		buf = xstrdup(v);
> -		argc = split_cmdline(buf, &argv);
> -		if (argc < 0)
> -			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> -			    split_cmdline_strerror(argc));
> -		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> -		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
> -		argc++;
> -		parse_options(argc, argv, NULL, builtin_merge_options,
> -			      builtin_merge_usage, 0);
> -		free(buf);
> +	struct merge_options_cb *merge_options = cb;
> +	int changed = 0;
> +
> +	/* We only handle mergeoptions for now */
> +	if (suffixcmp(k, ".mergeoptions"))
> +		return;
> +
> +	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
> +		merge_options->vote = 0.1;
> +		changed = 1;
> +	} else if (branch && !prefixcmp(k, "branch.") &&
> +				!prefixcmp(k + 7, branch) &&
> +				merge_options->vote < 1.0) {
> +		merge_options->vote = 1.0;
> +		changed = 1;
>  	}
>  
> +	if (changed) {
> +		merge_options->key = (char *)k;
> +		merge_options->value = (char *)v;
> +	}
> +}
> +
> +static void apply_merge_options(struct merge_options_cb *opts)
> +{
> +	const char **argv;
> +	int argc;
> +	char *buf;
> +
> +	if ( opts == NULL )
> +		return;
> +
> +	buf = xstrdup(opts->value);
> +	argc = split_cmdline(buf, &argv);
> +	if (argc < 0)
> +		die(_("Bad %s string: %s"), 
> +			opts->key, split_cmdline_strerror(argc));
> +
> +	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> +	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
> +	argc++;
> +	parse_options(argc, argv, NULL, builtin_merge_options,
> +			  builtin_merge_usage, 0);
> +	free(buf);
> +}
> +
> +static int git_merge_config(const char *k, const char *v, void *cb)
> +{
> +	if (cb != NULL && branch && !prefixcmp(k, "branch."))
> +		parse_git_merge_options(k, v, cb);
> +
>  	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
>  		show_diffstat = git_config_bool(k, v);
>  	else if (!strcmp(k, "pull.twohead"))
> @@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	const char *head_arg;
>  	int flag, head_invalid = 0, i;
>  	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
> +	struct merge_options_cb merge_options = {NULL, NULL, 0.0};
>  	struct commit_list *common = NULL;
>  	const char *best_strategy = NULL, *wt_strategy = NULL;
>  	struct commit_list **remotes = &remoteheads;
> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	if (is_null_sha1(head))
>  		head_invalid = 1;
>  
> -	git_config(git_merge_config, NULL);
> +	git_config(git_merge_config, &merge_options);
> +	if (merge_options.key != NULL && merge_options.value != NULL)
> +		apply_merge_options(&merge_options);
>  
>  	/* for color.ui */
>  	if (diff_use_color_default == -1)
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index e84e822..5b1f8e1 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
>  
>  test_debug 'git log --graph --decorate --oneline --all'
>  
> +test_expect_success 'merge c0 with c1 (default no-ff)' '
> +	git reset --hard c0 &&
> +	test_might_fail git config --unset branch.master.mergeoptions &&
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents $c0 $c1
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'
> +
> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
> +	git reset --hard c0 &&
> +	test_might_fail git config --remove-section branch.master &&
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	git config branch.master.mergeoptions "--ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'
> +
> +test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
> +	git reset --hard c0 &&
> +	test_might_fail git config --remove-section branch.master &&
> +	git config branch.master.mergeoptions "--ff" &&
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'
> +
>  test_expect_success 'combining --squash and --no-ff is refused' '
>  	test_must_fail git merge --squash --no-ff c1 &&
>  	test_must_fail git merge --no-ff --squash c1
Jonathan Nieder· May 3, 2011, 20:44 UTC · re: Michael Grubb · lore

Re: [PATCH v4] Add default merge options for all branches

Michael Grubb wrote:
Show 7 quoted lines
> In order to facilitate future features centered around this new
> "globlike" syntax a new struct has been created to keep track of the
> branch.*.* options.  Currently it only supports branch.*.mergeoptions,
> but can be easily modified in the future to support other branch
> specific options as well. The mechanism will "vote" on specific-"ness"
> of the configuration key and ultimately only use the most specific
> options.
My immediate reactions:
 - Is this really needed?  If we need some priority field when other
   branch globs are being supported, can't we add it then?
 - Floating point?  *turns to flee*
Show 9 quoted lines
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
>  	Sets default options for merging into branch <name>. The syntax and
>  	supported options are the same as those of 'git merge', but option
>  	values containing whitespace characters are currently not supported.
> +	The special value '*' for <name> may be used to configure default
> +	options for all branches.  Values for specific branch names will
> +	override the this default.
In what sense are they overridden?  For example, if I write
	[branch "*"]
		mergeoptions = --no-ff
	[branch "master"]
		mergeoptions = --log=5

and merge another branch into master, will the effect be as though I wrote --no-ff --log=5 or just --log=5?

I'm starting to suspect it might be simpler to add a new "[merge] no-ff" configuration item, like the existing "[merge] log".

Show 15 quoted lines
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -32,6 +32,21 @@
>  #define NO_FAST_FORWARD (1<<2)
>  #define NO_TRIVIAL      (1<<3)
>  
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */
Style: comments should look like this:
	/*
	 * Explanation comes here, with each line being
	 * approximately the same length as the next one.
	 */

"It is called vote because I couldn't come up with a better name" does not belong in a comment unless you are asking the reader to fix it. In that case, I would write something like

	float vote;	/* NEEDSWORK: give this a better name */
or
	float priority;
Show 5 quoted lines
> +struct merge_options_cb {
> +	char *key;
> +	char *value;
> +	float vote;
> +};

Since I didn't read the comment, I'm not sure what these represent. Who is responsible for freeing them? Is the key a glob?

Show 32 quoted lines
> @@ -503,28 +518,60 @@ cleanup:
>  	strbuf_release(&bname);
>  }
>  
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static void parse_git_merge_options(const char *k, const char *v,
> +			void *cb)
>  {
> -	if (branch && !prefixcmp(k, "branch.") &&
> -		!prefixcmp(k + 7, branch) &&
> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> -		const char **argv;
> -		int argc;
> -		char *buf;
> -
> -		buf = xstrdup(v);
> -		argc = split_cmdline(buf, &argv);
> -		if (argc < 0)
> -			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> -			    split_cmdline_strerror(argc));
> -		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> -		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
> -		argc++;
> -		parse_options(argc, argv, NULL, builtin_merge_options,
> -			      builtin_merge_usage, 0);
> -		free(buf);
> +	struct merge_options_cb *merge_options = cb;
> +	int changed = 0;
> +
> +	/* We only handle mergeoptions for now */
> +	if (suffixcmp(k, ".mergeoptions"))
> +		return;

The function is called parse_git_merge_options; I wonder if this "for now" is an old comment from when it had a different name.

> +
> +	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
> +		merge_options->vote = 0.1;
> +		changed = 1;
Style: parens.  What happens if I use
	[branch "*aster"]
		mergeoptions = ...
or
	[branch "*.c"]
		mergeoptions = ...
?  Where does 0.1 come from?
Show 5 quoted lines
> +	} else if (branch && !prefixcmp(k, "branch.") &&
> +				!prefixcmp(k + 7, branch) &&
> +				merge_options->vote < 1.0) {
> +		merge_options->vote = 1.0;
> +		changed = 1;
What does changed mean?
Show 5 quoted lines
>  	}
>  
> +	if (changed) {
> +		merge_options->key = (char *)k;
> +		merge_options->value = (char *)v;
Why not make the struct fields const?
Show 10 quoted lines
> +	}
> +}
> +
> +static void apply_merge_options(struct merge_options_cb *opts)
> +{
> +	const char **argv;
> +	int argc;
> +	char *buf;
> +
> +	if ( opts == NULL )
Style: parens.  It would be more idiomatic to say
	if (!opts)
Show 15 quoted lines
> +		return;
> +
> +	buf = xstrdup(opts->value);
> +	argc = split_cmdline(buf, &argv);
> +	if (argc < 0)
> +		die(_("Bad %s string: %s"), 
> +			opts->key, split_cmdline_strerror(argc));
> +
> +	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> +	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
> +	argc++;
> +	parse_options(argc, argv, NULL, builtin_merge_options,
> +			  builtin_merge_usage, 0);
> +	free(buf);
> +}
Idea: the series could be made easier to read if splitting this off
into its own function were a separate patch.  Anyway, splitting it out
was a good idea; thanks for doing that.

It seems you changed the whitespace while unindenting? The whitespace on the continuation line for parse_options(...) arguments is especially confusing (I suspect you are using a tabwidth of 6, but that doesn't make sense, either, since opts->key is not an argument to _. The official rendering uses a tabwidth of 8).

Show 5 quoted lines
> +
> +static int git_merge_config(const char *k, const char *v, void *cb)
> +{
> +	if (cb != NULL && branch && !prefixcmp(k, "branch."))
> +		parse_git_merge_options(k, v, cb);
More idiomatic to say
	if (!cb && branch && !prefixcmp(k, "branch."))
		...

Changing behavior when cb is NULL seems odd to me. Would it ever actually be NULL?

Show 8 quoted lines
> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	if (is_null_sha1(head))
>  		head_invalid = 1;
>  
> -	git_config(git_merge_config, NULL);
> +	git_config(git_merge_config, &merge_options);
> +	if (merge_options.key != NULL && merge_options.value != NULL)
> +		apply_merge_options(&merge_options);
More idiomatic to say
	if (merge_options.key && merge_options.value)

But this seems odd, too --- when would "key" be non-null without "value" being so?

> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
[...]
Show 19 quoted lines
> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
> +	git reset --hard c0 &&
> +	test_might_fail git config --remove-section branch.master &&
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	git config branch.master.mergeoptions "--ff" &&
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'
> +
> +test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
> +	git reset --hard c0 &&
> +	test_might_fail git config --remove-section branch.master &&
> +	git config branch.master.mergeoptions "--ff" &&
> +	git config "branch.*.mergeoptions" "--no-ff" &&
> +	test_tick &&
> +	git merge c1 &&

Thanks. This might pass by mistake with a "git config" implementation that sorts its entries when writing them; since I think git is intended to allow users writing to the config file directly, too, maybe something along the lines of

	cat >>.git/config <<-\EOF &&
	[branch "master"]
		mergeoptions = --ff
	[branch "*"]
		mergeoptions = --no-ff
	EOF
could be interesting.

Also --ff and --no-ff do not have orthogonal effect. What happens when the merge options do (e.g., --ff and --log)?

> +	git config --remove-section "branch.*" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'
Thanks again.

Hope that helps, Jonathan

Junio C Hamano· May 3, 2011, 22:51 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4] Add default merge options for all branches

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> In what sense are they overridden?  For example, if I write
>
> 	[branch "*"]
> 		mergeoptions = --no-ff
>
> 	[branch "master"]
> 		mergeoptions = --log=5
>
> and merge another branch into master, will the effect be as though I
> wrote --no-ff --log=5 or just --log=5?
I think the latter is overriding, and the former is cumulative.
> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
> configuration item, like the existing "[merge] log".
Surely
	[merge]
        	log = false
                ff = false
would be a lot simpler and probably far easier to explain.
Does
	[merge]
		log = false
	[branch "master"]
        	mergeoptions = --log=5
do the right thing with the current codebase?
Junio C Hamano· May 4, 2011, 04:25 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] Add default merge options for all branches

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>> configuration item, like the existing "[merge] log".
>
> Surely
>
> 	[merge]
>         	log = false
>               ff = false
>
> would be a lot simpler and probably far easier to explain.

Yes, it is far simpler and easier to explain. I'll leave the tests and the commit log message to people who are more interested in this topic than I am ;-)

 Documentation/merge-config.txt |    6 ++++++
 builtin/merge.c                |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)
Show changes to 2 files +9 −0

Documentation/merge-config.txt, builtin/merge.c

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..2aa4408 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,12 @@ merge.defaultToUpstream::
 	to their corresponding remote tracking branches, and the tips of
 	these tracking branches are merged.
 
+merge.ff::
+	Do not generate a merge commit if the merge resolved as a
+	fast-forward; only update the branch pointer instead.  Setting
+	this to `false` would be equivalent to giving `--no-ff` from
+	the command line.
+
 merge.log::
 	In addition to branch names, populate the log message with at
 	most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..5194f04 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		if (is_bool && shortlog_len)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
+	} else if (!strcmp(k, "merge.ff")) {
+		allow_fast_forward = git_config_bool(k, v);
+		return 0;
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
 		default_to_upstream = git_config_bool(k, v);
 		return 0;
Michael Grubb· May 4, 2011, 04:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] Add default merge options for all branches

I take this to mean that my patch is no longer needed/wanted?
On 5/3/11 11:25 PM, Junio C Hamano wrote:
Show 59 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>>> configuration item, like the existing "[merge] log".
>>
>> Surely
>>
>> 	[merge]
>>         	log = false
>>               ff = false
>>
>> would be a lot simpler and probably far easier to explain.
> 
> Yes, it is far simpler and easier to explain.  I'll leave the tests and
> the commit log message to people who are more interested in this topic
> than I am ;-)
> 
>  Documentation/merge-config.txt |    6 ++++++
>  builtin/merge.c                |    3 +++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 8920258..2aa4408 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -16,6 +16,12 @@ merge.defaultToUpstream::
>  	to their corresponding remote tracking branches, and the tips of
>  	these tracking branches are merged.
>  
> +merge.ff::
> +	Do not generate a merge commit if the merge resolved as a
> +	fast-forward; only update the branch pointer instead.  Setting
> +	this to `false` would be equivalent to giving `--no-ff` from
> +	the command line.
> +
>  merge.log::
>  	In addition to branch names, populate the log message with at
>  	most the specified number of one-line descriptions from the
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d171c63..5194f04 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		if (is_bool && shortlog_len)
>  			shortlog_len = DEFAULT_MERGE_LOG_LEN;
>  		return 0;
> +	} else if (!strcmp(k, "merge.ff")) {
> +		allow_fast_forward = git_config_bool(k, v);
> +		return 0;
>  	} else if (!strcmp(k, "merge.defaulttoupstream")) {
>  		default_to_upstream = git_config_bool(k, v);
>  		return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Jonathan Nieder· May 4, 2011, 04:58 UTC · re: Michael Grubb · lore

Re: [PATCH v4] Add default merge options for all branches

Michael Grubb wrote:
> I take this to mean that my patch is no longer needed/wanted?

No, as usual it means that Junio was thinking and sent us his thoughts.

Do you like it? If so, the way to move it forward would be to add tests and write a commit log message. If not, the way to move things forward would be to explain what use case it misses and work on a better patch that takes care of them.

Hope that helps, Jonathan

Michael Grubb· May 4, 2011, 18:58 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4] Add default merge options for all branches

Well, it certainly serves *my* immediate needs and addresses the specific use case that I was originally working on.  Though I think that what we've come up with would benefit the codebase in general if for no other reason than it lays some ground work for future features and is a bit more generic.  I also wouldn't mind going a step further and working on the globbing feature in a separate series.
Here are some corrections to the previous patch:

This patch makes the branch specific options more flexible by laying the ground work for supporting globs for the branch names. At this time only the value '*' is supported which will set default values for all branches. In the future this may be expanded to support true globbing.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 builtin/merge.c  |  120 ++++++++++++++++++++++++++++++++++-------------------
 t/t7600-merge.sh |   27 ++++++++----
 2 files changed, 95 insertions(+), 52 deletions(-)
Show changes to 2 files +95 −52

builtin/merge.c, t/t7600-merge.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index fa56205..9e6ec5f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -31,20 +31,12 @@
 #define DEFAULT_OCTOPUS (1<<1)
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
+#define MERGE_OPTIONS_INIT {NULL, NULL, NULL}
 
-/* This is for branch.<foo>. blocks
- * the vote member holds a value between
- * 0.0 and 1.0 which measures how closely
- * a branch name matches the key member.
- * where branch.*.mergeoptions would be 0.1 and
- * branch.<name>.mergeoptions would be 1.0
- * Also it is called vote because I couldn't come
- * up with a better name.
- */
 struct merge_options_cb {
-	char *key;
-	char *value;
-	float vote;
+	char *option;
+	char *spec;
+	char *name;
 };
 
 struct strategy {
@@ -518,30 +510,11 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-static void parse_git_merge_options(const char *k, const char *v,
-			void *cb)
+static void free_merge_options_cb(struct merge_options_cb *data)
 {
-	struct merge_options_cb *merge_options = cb;
-	int changed = 0;
-
-	/* We only handle mergeoptions for now */
-	if (suffixcmp(k, ".mergeoptions"))
-		return;
-
-	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
-		merge_options->vote = 0.1;
-		changed = 1;
-	} else if (branch && !prefixcmp(k, "branch.") &&
-				!prefixcmp(k + 7, branch) &&
-				merge_options->vote < 1.0) {
-		merge_options->vote = 1.0;
-		changed = 1;
-	}
-
-	if (changed) {
-		merge_options->key = (char *)k;
-		merge_options->value = (char *)v;
-	}
+	free(data->option);
+	free(data->spec);
+	free(data->name);
 }
 
 static void apply_merge_options(struct merge_options_cb *opts)
@@ -550,26 +523,85 @@ static void apply_merge_options(struct merge_options_cb *opts)
 	int argc;
 	char *buf;
 
-	if ( opts == NULL )
+	if (!opts)
 		return;
 
-	buf = xstrdup(opts->value);
+	buf = xstrdup(opts->option);
 	argc = split_cmdline(buf, &argv);
 	if (argc < 0)
-		die(_("Bad %s string: %s"),
-			opts->key, split_cmdline_strerror(argc));
+		die(_("Bad branch.%s.%s string: %s"),
+			opts->spec, opts->name, split_cmdline_strerror(argc));
 
 	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
 	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
 	argc++;
 	parse_options(argc, argv, NULL, builtin_merge_options,
-			  builtin_merge_usage, 0);
+		builtin_merge_usage, 0);
 	free(buf);
 }
 
+/*
+ * This function returns -1 if the first argument is less specific
+ * than the second, and 1 vice versa. Returns -1 if both arguments
+ * are the same.
+ */
+static int cmp_specificity(const char *a, const char *b)
+{
+	switch((!strcmp(a, "*") ? 2 : 0) |
+		(!strcmp(b, "*") ? 1 : 0)) {
+	case 3:
+	case 0:
+		return -1; /* later one wins if they are the same */
+	case 1:
+		return 1;
+	case 2:
+		return -1;
+	}
+	return 0; /* should never happen */
+}
+
+static void parse_git_merge_options(const char *k, const char *v,
+			void *cb)
+{
+	struct merge_options_cb *merge_options = cb;
+	char *spec, *eon; /* end-of-name */
+
+	k += 7; /* not interested in the leading "branch." */
+	eon = strrchr(k, '.');
+	if (!eon || strcmp(eon, ".mergeoptions"))
+		return;
+
+	/* k through eon is name or glob */
+	spec = xmemdupz(k, eon - k);
+	/*
+	 * NEEDSWORK: for now we say "*" matches; we would need
+	 * to turn the following into something like:
+	 *	if (has_wildcard(spec)
+	 *		? !glob_matches(spec, branch)
+	 *		: strcmp(spec, branch)) {
+	 *		free(spec);
+	 *		return;
+	 *	}
+	 */
+	if (strcmp(spec, "*") && strcmp(spec, branch)) {
+		free(spec);
+		return;
+	}
+
+	if (!merge_options->option ||
+			cmp_specificity(merge_options->spec, spec) < 0) {
+		free_merge_options_cb(merge_options);
+		merge_options->option = xstrdup(v);
+		merge_options->name = xstrdup(eon + 1);
+		merge_options->spec = spec;
+		return;
+	}
+	free(spec);
+}
+
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
-	if (cb != NULL && branch && !prefixcmp(k, "branch."))
+	if (branch && !prefixcmp(k, "branch."))
 		parse_git_merge_options(k, v, cb);
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -1034,7 +1066,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
-	struct merge_options_cb merge_options = {NULL, NULL, 0.0};
+	struct merge_options_cb merge_options = MERGE_OPTIONS_INIT;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
@@ -1053,8 +1085,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		head_invalid = 1;
 
 	git_config(git_merge_config, &merge_options);
-	if (merge_options.key != NULL && merge_options.value != NULL)
+	if (merge_options.option) {
 		apply_merge_options(&merge_options);
+		free_merge_options_cb(&merge_options);
+	}
 
 	/* for color.ui */
 	if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5b1f8e1..d7a60b2 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -417,37 +417,46 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c0 with c1 (default no-ff)' '
 	git reset --hard c0 &&
-	test_might_fail git config --unset branch.master.mergeoptions &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "*"]
+		mergeoptions = --no-ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
 	verify_merge file result.1 &&
 	verify_parents $c0 $c1
 '
-
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
 	git reset --hard c0 &&
-	test_might_fail git config --remove-section branch.master &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
-	git config branch.master.mergeoptions "--ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "*"]
+		mergeoptions = --no-ff
+	[branch "master"]
+		mergeoptions = --ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
+	git config --remove-section "branch.master" &&
 	verify_merge file result.1 &&
 	verify_parents "$c0"
 '
 
 test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
 	git reset --hard c0 &&
-	test_might_fail git config --remove-section branch.master &&
-	git config branch.master.mergeoptions "--ff" &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "master"]
+		mergeoptions = --ff
+	[branch "*"]
+		mergeoptions = --no-ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
+	git config --remove-section "branch.master" &&
 	verify_merge file result.1 &&
 	verify_parents "$c0"
 '
-- 
1.7.5



On 5/3/11 11:58 PM, Jonathan Nieder wrote:
> Michael Grubb wrote:
> 
>> I take this to mean that my patch is no longer needed/wanted?
> 
> No, as usual it means that Junio was thinking and sent us his
> thoughts.
> 
> Do you like it?  If so, the way to move it forward would be to add
> tests and write a commit log message.  If not, the way to move things
> forward would be to explain what use case it misses and work on a
> better patch that takes care of them.
> 
> Hope that helps,
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Junio C Hamano· May 4, 2011, 21:35 UTC · re: Michael Grubb · lore

Re: [PATCH v4] Add default merge options for all branches

Michael Grubb <devel@dailyvoid.com> writes:
> Well, it certainly serves *my* immediate needs and addresses the
> specific use case that I was originally working on.  Though I think that
> what we've come up with would benefit the codebase in general if for no
> other reason than it lays some ground work for future features...

The discussion was fruitful, I think, and it may have laid the groundwork at the _conceptual level_. I however do not think that the approach the patch takes is generic enough, even if we add globbing of the branch name, to build on other things that can come out of the future directions the discussion suggested.

For example, why does "merge.c" hold the parser for "branch.*.*", when there are a lot more configuration variables that apply per branch that are totally unrelated to merge? Don't these branch.*.* variables benefit from having a wildcard support as well? If we wanted to add such support, which configuration parser should be called by many pgorams that deal with branches (e.g. "checkout -b", "branch", "fetch", ...), and where that parser should be defined in? I suspect the answer might be "branch.c", but I do not htink we explored the uses widely enough to make that decision yet.

Even if I limit the discussion to "mergeoptions" [*1*], why can I specify the merge options based on what branch I am currently on, but not based on what branch I am merging to my current branch? When on master branch, should branch.*.mo augument what I have in branch.master.mo, or should it overwrite it? I may want to say "all my merges should use --log" and "when on master I want --no-ff". Should branch.master.mo repeat "--log", or should the values on the two variables automatically combine? If so in what order? Am I allowed to say "Use 5 lines of --log" for generic one, and then "Use 10 more lines of --log than generic" for branch.master.mo, so that later I can easily change my mind and update branch.*.mo to use 10 lines, and I get automatically 20 lines for the master branch? If not why not?

etc.etc.etc....

I am not saying some of these issues are unsolvable, nor we should solve all these problems right now, but I do not think these issues are something you are trying to solve with this patch, nor the approach this patch happens to use was designed with these problems in mind (I certainly didn't, when I made many suggestions I see in this round of your patch) to become a foundation to solve them in the future.

The suggestion by Jonathan does not have such a design issue that requires us to open a huge can of worms right now and potentially result in a solution that is overengineered to address a wrong problem [*2*].

If it solves the original issue without such downsides, that would be more preferrable, I think; no?

[Footnote]

*1* I personally think "branch.<name>.mergeoptions" was a mistake. If it were separate "branch.<name>.merge-ff", "branch.<name>.merge-log", ... it might have been more clear what the combining semantics should be. But that is not going to change, so I'd rather not to extend the support for it, until we come up with something more sensible.

*2* For example, globbing to match the current branch name could become an overengineered solution to a wrong problem, if it turns out that it is not useful to decide things based solely on the current branch name.

John Szakmeister· May 4, 2011, 10:58 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] Add default merge options for all branches

On Wed, May 4, 2011 at 12:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>>> configuration item, like the existing "[merge] log".
>>
>> Surely
>>
>>       [merge]
>>               log = false
>>               ff = false
>>
>> would be a lot simpler and probably far easier to explain.
>
> Yes, it is far simpler and easier to explain.  I'll leave the tests and
> the commit log message to people who are more interested in this topic
> than I am ;-)
I like the idea.  I assume it can be overridden on the command line too?
-John
Junio C Hamano· May 3, 2011, 22:03 UTC · re: Michael Grubb · lore

Re: [PATCH v4] Add default merge options for all branches

Michael Grubb <devel@dailyvoid.com> writes:
/*
 * Our Multi-line comments begin with a line with
 * slash asterisk then newline.
 */
Show 9 quoted lines
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */

How about simply dropping that "vote" thing? I do not want to see unnecessary float creeping into our codebase.

The k and v parameters are volatile from the point of view of this function. You need to xstrdup() them to keep a copy.

There is no need to store "branch." part in cb->key, as it is common across the variables.

The logic would probably look like this:
	if (prefixcmp(k, "branch."))
		return;
	k += 7; /* past "branch." part */
	eon = strrchr(k, '.'); /* end-of-name 8/
        if (!eon || strcmp(eon, ".mergeoptions"))
        	return;
	/* k thru eon is the name or wildcard */
	spec = xmemdupz(k, eon - k);
        /*
         * NEEDSWORK: for now we say "*" matches; we would need
         * to turn the following into something like:
         *	if (has_wildcard(spec) 
	 *		? !glob_matches(spec, branch)
	 *		: strcmp(spec, branch)) {
         *		free(spec);
         *		return;
         *	}
         */
	if (strcmp(spec, "*") && strcmp(spec, branch)) {
        	free(spec);
                return;
	}
        if (!merge_options->option ||
             cmp_specificity(merge_options->spec, spec) < 0) {
		/* use this one */
                free(merge_options->spec);
                free(merge_options->option);
                merge_options->option = xstrdup(v);
                merge_options->spec = spec;
		return;
	}
        free(spec);
And then cmp_specificity() would say something like:
	static int cmp_specificity(const char *a, const char *b)
        {
        	switch ((!strcmp(a, "*") ? 2 : 0) |
                	(!strcmp(b, "*") ? 1 : 0)) {
		case 3:
                        /*
                         * NEEDSWORK: when we start truly globbing,
                         * we need to decide "foo/*" is more specific than
                         * "*" and the like. But for now we do not have to
                         * worry about that case.
                         */
		case 0:
                        return -1; /* later one wins if they are the same */
		case 1:
			return 1;
		case 2:
			return -1;
		}
	}
meaning, the ones with wildcard are weaker than the ones without.
Michael Grubb· May 3, 2011, 20:37 UTC · re: Jonathan Nieder · lore

[PATCH v4.1] Add default merge options for all branches

Add support for branch.*.mergeoptions for setting default options for all branches. This new value shares semantics with the existing branch.<name>.mergeoptions variable. If a branch specific value is found, that value will be used.

The need for this arises from the fact that there is currently not an easy way to set merge options for all branches. Instead of having to specify merge options for each individual branch there should be a way to set defaults for all branches and then override a specific branch's options.

In order to facilitate future features centered around this new "globlike" syntax a new struct has been created to keep track of the branch.*.* options. Currently it only supports branch.*.mergeoptions, but can be easily modified in the future to support other branch specific options as well. The mechanism will "vote" on specific-"ness" of the configuration key and ultimately only use the most specific options. This is not cumulative but overriding.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 Documentation/git-merge.txt |    3 +
 builtin/merge.c             |   90 +++++++++++++++++++++++++++++++++---------
 t/t7600-merge.sh            |   39 ++++++++++++++++++
 3 files changed, 112 insertions(+), 20 deletions(-)
Show changes to 3 files +112 −20

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
 	Sets default options for merging into branch <name>. The syntax and
 	supported options are the same as those of 'git merge', but option
 	values containing whitespace characters are currently not supported.
+	The special value '*' for <name> may be used to configure default
+	options for all branches.  Values for specific branch names will
+	override the this default.
 
 SEE ALSO
 --------
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..fa56205 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,6 +32,21 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
 
+/* This is for branch.<foo>. blocks
+ * the vote member holds a value between
+ * 0.0 and 1.0 which measures how closely
+ * a branch name matches the key member.
+ * where branch.*.mergeoptions would be 0.1 and
+ * branch.<name>.mergeoptions would be 1.0
+ * Also it is called vote because I couldn't come
+ * up with a better name.
+ */
+struct merge_options_cb {
+	char *key;
+	char *value;
+	float vote;
+};
+
 struct strategy {
 	const char *name;
 	unsigned attr;
@@ -503,28 +518,60 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-static int git_merge_config(const char *k, const char *v, void *cb)
+static void parse_git_merge_options(const char *k, const char *v,
+			void *cb)
 {
-	if (branch && !prefixcmp(k, "branch.") &&
-		!prefixcmp(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
-		const char **argv;
-		int argc;
-		char *buf;
-
-		buf = xstrdup(v);
-		argc = split_cmdline(buf, &argv);
-		if (argc < 0)
-			die(_("Bad branch.%s.mergeoptions string: %s"), branch,
-			    split_cmdline_strerror(argc));
-		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
-		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
-		argc++;
-		parse_options(argc, argv, NULL, builtin_merge_options,
-			      builtin_merge_usage, 0);
-		free(buf);
+	struct merge_options_cb *merge_options = cb;
+	int changed = 0;
+
+	/* We only handle mergeoptions for now */
+	if (suffixcmp(k, ".mergeoptions"))
+		return;
+
+	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
+		merge_options->vote = 0.1;
+		changed = 1;
+	} else if (branch && !prefixcmp(k, "branch.") &&
+				!prefixcmp(k + 7, branch) &&
+				merge_options->vote < 1.0) {
+		merge_options->vote = 1.0;
+		changed = 1;
 	}
 
+	if (changed) {
+		merge_options->key = (char *)k;
+		merge_options->value = (char *)v;
+	}
+}
+
+static void apply_merge_options(struct merge_options_cb *opts)
+{
+	const char **argv;
+	int argc;
+	char *buf;
+
+	if ( opts == NULL )
+		return;
+
+	buf = xstrdup(opts->value);
+	argc = split_cmdline(buf, &argv);
+	if (argc < 0)
+		die(_("Bad %s string: %s"),
+			opts->key, split_cmdline_strerror(argc));
+
+	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
+	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
+	argc++;
+	parse_options(argc, argv, NULL, builtin_merge_options,
+			  builtin_merge_usage, 0);
+	free(buf);
+}
+
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+	if (cb != NULL && branch && !prefixcmp(k, "branch."))
+		parse_git_merge_options(k, v, cb);
+
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
+	struct merge_options_cb merge_options = {NULL, NULL, 0.0};
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (is_null_sha1(head))
 		head_invalid = 1;
 
-	git_config(git_merge_config, NULL);
+	git_config(git_merge_config, &merge_options);
+	if (merge_options.key != NULL && merge_options.value != NULL)
+		apply_merge_options(&merge_options);
 
 	/* for color.ui */
 	if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..5b1f8e1 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (default no-ff)' '
+	git reset --hard c0 &&
+	test_might_fail git config --unset branch.master.mergeoptions &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
+	git reset --hard c0 &&
+	test_might_fail git config --remove-section branch.master &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	git config branch.master.mergeoptions "--ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
+	git reset --hard c0 &&
+	test_might_fail git config --remove-section branch.master &&
+	git config branch.master.mergeoptions "--ff" &&
+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
+test_debug 'git log --graph --decorate --oneline --all'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
-- 
1.7.5
Michael Grubb· May 4, 2011, 22:07 UTC · re: Jonathan Nieder · lore

[PATCH v5] Add default merge options for all branches

Add merge.ff flag to support turning off fast-forward merges by default. This flag is overridden if any branch.x.mergeoptions turn it back on.

 Documentation/merge-config.txt |    6 ++++++
 builtin/merge.c                |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reported-by: Michael Grubb <devel@dailyvoid.com>
---
 Documentation/merge-config.txt |    6 ++++++
 builtin/merge.c                |    3 +++
 t/t7600-merge.sh               |   23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+), 0 deletions(-)
Show changes to 3 files +32 −0

Documentation/merge-config.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..2aa4408 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,12 @@ merge.defaultToUpstream::
 	to their corresponding remote tracking branches, and the tips of
 	these tracking branches are merged.
 
+merge.ff::
+	Do not generate a merge commit if the merge resolved as a
+	fast-forward; only update the branch pointer instead.  Setting
+	this to `false` would be equivalent to giving `--no-ff` from
+	the command line.
+
 merge.log::
 	In addition to branch names, populate the log message with at
 	most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..5194f04 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		if (is_bool && shortlog_len)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
+	} else if (!strcmp(k, "merge.ff")) {
+		allow_fast_forward = git_config_bool(k, v);
+		return 0;
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
 		default_to_upstream = git_config_bool(k, v);
 		return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..21c25d4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (merge.ff=false)' '
+	git reset --hard c0 &&
+	git config merge.ff false &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section merge &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions --ff
+	git config merge.ff false
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.master" &&
+	git config --remove-section "merge" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
Junio C Hamano· May 5, 2011, 00:42 UTC · re: Michael Grubb · lore

Re: [PATCH v5] Add default merge options for all branches

Thanks.

I think we still need to work on this a bit more, but I found an unrelated and nastier issue before this patch can sanely be applied.

Show 11 quoted lines
> +test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
> +	git reset --hard c0 &&
> +	git config branch.master.mergeoptions --ff
> +	git config merge.ff false
> +	test_tick &&
> +	git merge c1 &&
> +	git config --remove-section "branch.master" &&
> +	git config --remove-section "merge" &&
> +	verify_merge file result.1 &&
> +	verify_parents "$c0"
> +'

If you insert an "exit" after this test and inspect the resulting commit, you will see that it created a merge.

	Side note: I think verify_parents is buggy. It only makes sure
	that the earlier parents of HEAD match the commits given, and does
	not care if there actually are more parents.

In any case, your test exposed an ancient breakage ever since the per-branch mergeoptions was introduced back when git-merge was a shell script (aec7b36 (git-merge: add support for branch.<name>.mergeoptions, 2007-09-24).

I am not going to fix verify_parents tonight, as I have other git things to do.

-- >8 --
Subject: [PATCH] merge: fix branch.<name>.mergeoptions

The parsing of the additional command line parameters supplied to the branch.<name>.mergeoptions configuration variable was implemented at the wrong stage. If any merge-related variable came after we read branch.<name>.mergeoptions, the earlier value was overwritten.

We should first read all the merge.* configuration, override them by reading from branch.<name>.mergeoptions and then finally read from the command line.

This patch should fix it, even though I now strongly suspect that branch.<name>.mergeoptions that gives a single command line that needs to be parsed was likely to be an ill-conceived idea to begin with. Sigh...

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-merge.c  |   39 +++++++++++++++++++++++++--------------
 t/t7600-merge.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 14 deletions(-)
Show changes to 2 files +57 −14

builtin-merge.c, t/t7600-merge.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index 3aaec7b..01389a3 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
 static size_t xopts_nr, xopts_alloc;
 static const char *branch;
+static char *branch_mergeoptions;
 static int verbosity;
 static int allow_rerere_auto;
 
@@ -474,25 +475,33 @@ cleanup:
 	strbuf_release(&bname);
 }
 
+static void parse_branch_merge_options(char *bmo)
+{
+	const char **argv;
+	int argc;
+	char *buf;
+
+	if (!bmo)
+		return;
+	argc = split_cmdline(bmo, &argv);
+	if (argc < 0)
+		die("Bad branch.%s.mergeoptions string", branch);
+	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
+	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
+	argc++;
+	parse_options(argc, argv, NULL, builtin_merge_options,
+		      builtin_merge_usage, 0);
+	free(buf);
+}
+
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
 	if (branch && !prefixcmp(k, "branch.") &&
 		!prefixcmp(k + 7, branch) &&
 		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
-		const char **argv;
-		int argc;
-		char *buf;
-
-		buf = xstrdup(v);
-		argc = split_cmdline(buf, &argv);
-		if (argc < 0)
-			die("Bad branch.%s.mergeoptions string", branch);
-		argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
-		memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
-		argc++;
-		parse_options(argc, argv, NULL, builtin_merge_options,
-			      builtin_merge_usage, 0);
-		free(buf);
+		free(branch_mergeoptions);
+		branch_mergeoptions = xstrdup(v);
+		return 0;
 	}
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -918,6 +927,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
+	if (branch_mergeoptions)
+		parse_branch_merge_options(branch_mergeoptions);
 	argc = parse_options(argc, argv, prefix, builtin_merge_options,
 			builtin_merge_usage, 0);
 	if (verbosity < 0)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 57f6d2b..56c653d 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -372,6 +372,38 @@ test_expect_success 'merge c1 with c2 (no-commit in config)' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2 (log in config)' '
+	git config branch.master.mergeoptions "" &&
+	git reset --hard c1 &&
+	git merge --log c2 &&
+	git show -s --pretty=tformat:%s%n%b >expect &&
+
+	git config branch.master.mergeoptions --log &&
+	git reset --hard c1 &&
+	git merge c2 &&
+	git show -s --pretty=tformat:%s%n%b >actual &&
+
+	test_cmp expect actual
+'
+
+test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
+	(
+		git config --remove-section branch.master
+		git config --remove-section merge
+	)
+	git reset --hard c1 &&
+	git merge c2 &&
+	git show -s --pretty=tformat:%s%n%b >expect &&
+
+	git config branch.master.mergeoptions "--no-log" &&
+	git config merge.log true &&
+	git reset --hard c1 &&
+	git merge c2 &&
+	git show -s --pretty=tformat:%s%n%b >actual &&
+
+	test_cmp expect actual
+'
+
 test_expect_success 'merge c1 with c2 (squash in config)' '
 	git reset --hard c1 &&
 	git config branch.master.mergeoptions "--squash" &&
-- 
1.7.5.284.g84c3a8

        
Junio C Hamano· May 6, 2011, 20:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v5] Add default merge options for all branches

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> In any case, your test exposed an ancient breakage ever since the
> per-branch mergeoptions was introduced back when git-merge was a shell
> script (aec7b36 (git-merge: add support for branch.<name>.mergeoptions,
> 2007-09-24).
>
> -- >8 --
> Subject: [PATCH] merge: fix branch.<name>.mergeoptions
> ...
And then on top of that fix, we can do this.

I have a seemingly unrelated change to the existing test but that was because it only made sure that the --ff-only option made the command fail when it should fail, without making sure that it does not interfere when it should succeed. A typical symptom of "showing off shiny new toy because I am too excited" developer disease, I would guess. I didn't want to forget to fix it.

-- >8 --
Subject: [PATCH] merge: introduce merge.ff configuration variable

This variable gives the default setting for --ff, --no-ff or --ff-only options of "git merge" command.

Helped-by: Michael Grubb <devel@dailyvoid.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * If we were doing the command line option of "merge" from scratch today,
   we probably would have done --ff, --ff=no, and --ff=only, instead of a
   separate --ff-only.  We could still add the latter two as a consistency
   synonyms without deprecating anything, though.
 Documentation/merge-config.txt |   10 +++++++++
 builtin/merge.c                |    9 ++++++++
 t/t7600-merge.sh               |   43 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 60 insertions(+), 2 deletions(-)
Show changes to 3 files +60 −2

Documentation/merge-config.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..861bd6f 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,16 @@ merge.defaultToUpstream::
 	to their corresponding remote tracking branches, and the tips of
 	these tracking branches are merged.
 
+merge.ff::
+	By default, git does not create an extra merge commit when merging
+	a commit that is a descendant of the current commit. Instead, the
+	tip of the current branch is fast-forwarded. When set to `false`,
+	this variable tells git to create an extra merge commit in such
+	a case (equivalent to giving the `--no-ff` option from the command
+	line). When set to `only`, only such fast-forward merges are
+	allowed (equivalent to giving the `--ff-only` option from the
+	command line).
+
 merge.log::
 	In addition to branch names, populate the log message with at
 	most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index 4fa789a..1c3ff13 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		if (is_bool && shortlog_len)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
+	} else if (!strcmp(k, "merge.ff")) {
+		int boolval = git_config_maybe_bool(k, v);
+		if (0 <= boolval) {
+			allow_fast_forward = boolval;
+		} else if (v && !strcmp(v, "only")) {
+			allow_fast_forward = 1;
+			fast_forward_only = 1;
+		} /* do not barf on values from future versions of git */
+		return 0;
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
 		default_to_upstream = git_config_bool(k, v);
 		return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5463f87..4f1d4eb 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -225,12 +225,28 @@ test_expect_success 'merge c1 with c2 and c3' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
-test_expect_success 'failing merges with --ff-only' '
+test_expect_success 'merges with --ff-only' '
 	git reset --hard c1 &&
 	test_tick &&
 	test_must_fail git merge --ff-only c2 &&
 	test_must_fail git merge --ff-only c3 &&
-	test_must_fail git merge --ff-only c2 c3
+	test_must_fail git merge --ff-only c2 c3 &&
+	git reset --hard c0 &&
+	git merge c3 &&
+	verify_head $c3
+'
+
+test_expect_success 'merges with merge.ff=only' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_when_finished "git config --unset merge.ff" &&
+	git config merge.ff only &&
+	test_must_fail git merge c2 &&
+	test_must_fail git merge c3 &&
+	test_must_fail git merge c2 c3 &&
+	git reset --hard c0 &&
+	git merge c3 &&
+	verify_head $c3
 '
 
 test_expect_success 'merge c0 with c1 (no-commit)' '
@@ -447,6 +463,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+test_expect_success 'merge c0 with c1 (merge.ff=false)' '
+	git reset --hard c0 &&
+	git config merge.ff false &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section merge &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions --ff
+	git config merge.ff false
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.master" &&
+	git config --remove-section "merge" &&
+	verify_merge file result.1 &&
+	verify_parents "$c0"
+'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
-- 
1.7.5.1.268.gce5bd
Jonathan Nieder· May 6, 2011, 21:59 UTC · re: Junio C Hamano · lore

Re: [PATCH v5] Add default merge options for all branches

Junio C Hamano wrote:
Show 7 quoted lines
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -16,6 +16,16 @@ merge.defaultToUpstream::
>  	to their corresponding remote tracking branches, and the tips of
>  	these tracking branches are merged.
>  
> +merge.ff::
[...]
> +	line). When set to `only`, only such fast-forward merges are
> +	allowed (equivalent to giving the `--ff-only` option from the
> +	command line).

A habitual "git pull" user that uses git in a read-only fashion to get the latest development snapshot of a project's code would probably find this handy.

Show 14 quoted lines
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		if (is_bool && shortlog_len)
>  			shortlog_len = DEFAULT_MERGE_LOG_LEN;
>  		return 0;
> +	} else if (!strcmp(k, "merge.ff")) {
> +		int boolval = git_config_maybe_bool(k, v);
> +		if (0 <= boolval) {
> +			allow_fast_forward = boolval;
> +		} else if (v && !strcmp(v, "only")) {
> +			allow_fast_forward = 1;
> +			fast_forward_only = 1;
> +		} /* do not barf on values from future versions of git */
Perhaps deserves a test, like so (meant for squashing)?
-- >8 --
Subject: tests: check git does not barf on merge.ff values for future versions of git

Maybe some day in the future we will want to support a syntax like

	[merge]
		ff = branch1
		ff = branch2
		ff = branch3

in addition to the currently permitted "true", "false", and "only" values. So make sure we continue to treat such configurations as though an unknown variable had been defined rather than erroring out, until it is time to implement such a thing, so configuration files using such a facility can be shared between present and future git.

While at it, add a few missing && and start the "combining --squash and --no-ff" test with a known state so we can be sure it does not succeed or fail for the wrong reason.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7600-merge.sh |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)
Show changes to t/t7600-merge.sh +14 −2
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 89e0b77..1e20f2e 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -35,6 +35,7 @@ printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
 printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
+>empty
 
 create_merge_msgs () {
 	echo "Merge commit 'c2'" >msg.1-5 &&
@@ -475,8 +476,8 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
 	git reset --hard c0 &&
-	git config branch.master.mergeoptions --ff
-	git config merge.ff false
+	git config branch.master.mergeoptions --ff &&
+	git config merge.ff false &&
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.master" &&
@@ -485,7 +486,18 @@ test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
 	verify_parents "$c0"
 '
 
+test_expect_success 'tolerate unknown values for merge.ff' '
+	git reset --hard c0 &&
+	git config merge.ff something-new &&
+	test_tick &&
+	git merge c1 2>message &&
+	git config --remove-section "merge" &&
+	verify_head "$c1" &&
+	test_cmp empty message
+'
+
 test_expect_success 'combining --squash and --no-ff is refused' '
+	git reset --hard c0 &&
 	test_must_fail git merge --squash --no-ff c1 &&
 	test_must_fail git merge --no-ff --squash c1
 '
-- 
1.7.5.1
Jonathan Nieder· May 6, 2011, 20:54 UTC · re: Junio C Hamano · lore

[PATCH 0/2] tests: make verify_merge check that the number of parents is right

Junio C Hamano wrote:
> 	Side note: I think verify_parents is buggy. It only makes sure
> 	that the earlier parents of HEAD match the commits given, and does
> 	not care if there actually are more parents.
How about this?
Jonathan Nieder (2):
  tests: eliminate unnecessary setup test assertions
  tests: teach verify_parents to check for extra parents
 t/t6010-merge-base.sh |   62 +++++++++++-----------
 t/t7600-merge.sh      |  135 ++++++++++++++++++++++++-------------------------
 2 files changed, 98 insertions(+), 99 deletions(-)
Jonathan Nieder· May 6, 2011, 20:58 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] tests: eliminate unnecessary setup test assertions

Most of git's tests write files and define shell functions and variables that will last throughout a test script at the top of the script, before all test assertions:

	. ./test-lib.sh
	VAR='some value'
	export VAR
	>empty
	fn () {
		do something
	}
	test_expect_success 'setup' '
		... nontrivial commands go here ...
	'

Two scripts use a different style with this kind of trivial code enclosed by a test assertion; fix them. The usual style is easier to read since there is less indentation to keep track of and no need to worry about nested quotes; and on the other hand, because the commands in question are trivial, it should not make the test suite any worse at catching future bugs in git.

While at it, make some other small tweaks:
 - spell function definitions with a space before () for consistency
   with other scripts;
 - use the self-contained command "git mktree </dev/null" in
   preference to "git write-tree" which looks at the index when
   writing an empty tree.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I should have done this long ago.  Sorry for the eyesore.
 t/t6010-merge-base.sh |   62 +++++++++++-----------
 t/t7600-merge.sh      |  134 ++++++++++++++++++++++++-------------------------
 2 files changed, 97 insertions(+), 99 deletions(-)
Show changes to 2 files +97 −99

t/t6010-merge-base.sh, t/t7600-merge.sh

diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 082032e..f80bba8 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -8,38 +8,38 @@ test_description='Merge base and parent list computation.
 
 . ./test-lib.sh
 
+M=1130000000
+Z=+0000
+
+GIT_COMMITTER_EMAIL=git@comm.iter.xz
+GIT_COMMITTER_NAME='C O Mmiter'
+GIT_AUTHOR_NAME='A U Thor'
+GIT_AUTHOR_EMAIL=git@au.thor.xz
+export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
+
+doit () {
+	OFFSET=$1 &&
+	NAME=$2 &&
+	shift 2 &&
+
+	PARENTS= &&
+	for P
+	do
+		PARENTS="${PARENTS}-p $P "
+	done &&
+
+	GIT_COMMITTER_DATE="$(($M + $OFFSET)) $Z" &&
+	GIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&
+	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&
+
+	commit=$(echo $NAME | git commit-tree $T $PARENTS) &&
+
+	echo $commit >.git/refs/tags/$NAME &&
+	echo $commit
+}
+
 test_expect_success 'setup' '
-	T=$(git write-tree) &&
-
-	M=1130000000 &&
-	Z=+0000 &&
-
-	GIT_COMMITTER_EMAIL=git@comm.iter.xz &&
-	GIT_COMMITTER_NAME="C O Mmiter" &&
-	GIT_AUTHOR_NAME="A U Thor" &&
-	GIT_AUTHOR_EMAIL=git@au.thor.xz &&
-	export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
-
-	doit() {
-		OFFSET=$1 &&
-		NAME=$2 &&
-		shift 2 &&
-
-		PARENTS= &&
-		for P
-		do
-			PARENTS="${PARENTS}-p $P "
-		done &&
-
-		GIT_COMMITTER_DATE="$(($M + $OFFSET)) $Z" &&
-		GIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&
-		export GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&
-
-		commit=$(echo $NAME | git commit-tree $T $PARENTS) &&
-
-		echo $commit >.git/refs/tags/$NAME &&
-		echo $commit
-	}
+	T=$(git mktree </dev/null)
 '
 
 test_expect_success 'set up G and H' '
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87d5d78..c665acd 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -28,80 +28,78 @@ Testing basic merge operations/option parsing.
 
 . ./test-lib.sh
 
-test_expect_success 'set up test data and helpers' '
-	printf "%s\n" 1 2 3 4 5 6 7 8 9 >file &&
-	printf "%s\n" "1 X" 2 3 4 5 6 7 8 9 >file.1 &&
-	printf "%s\n" 1 2 3 4 "5 X" 6 7 8 9 >file.5 &&
-	printf "%s\n" 1 2 3 4 5 6 7 8 "9 X" >file.9 &&
-	printf "%s\n" "1 X" 2 3 4 5 6 7 8 9 >result.1 &&
-	printf "%s\n" "1 X" 2 3 4 "5 X" 6 7 8 9 >result.1-5 &&
-	printf "%s\n" "1 X" 2 3 4 "5 X" 6 7 8 "9 X" >result.1-5-9 &&
+printf '%s\n' 1 2 3 4 5 6 7 8 9 >file
+printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1
+printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
+printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
+printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
+printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
 
-	create_merge_msgs() {
-		echo "Merge commit '\''c2'\''" >msg.1-5 &&
-		echo "Merge commit '\''c2'\''; commit '\''c3'\''" >msg.1-5-9 &&
-		{
-			echo "Squashed commit of the following:" &&
-			echo &&
-			git log --no-merges ^HEAD c1
-		} >squash.1 &&
-		{
-			echo "Squashed commit of the following:" &&
-			echo &&
-			git log --no-merges ^HEAD c2
-		} >squash.1-5 &&
-		{
-			echo "Squashed commit of the following:" &&
-			echo &&
-			git log --no-merges ^HEAD c2 c3
-		} >squash.1-5-9 &&
-		echo >msg.nolog &&
-		{
-			echo "* commit '\''c3'\'':" &&
-			echo "  commit 3" &&
-			echo
-		} >msg.log
-	} &&
+create_merge_msgs () {
+	echo "Merge commit 'c2'" >msg.1-5 &&
+	echo "Merge commit 'c2'; commit 'c3'" >msg.1-5-9 &&
+	{
+		echo "Squashed commit of the following:" &&
+		echo &&
+		git log --no-merges ^HEAD c1
+	} >squash.1 &&
+	{
+		echo "Squashed commit of the following:" &&
+		echo &&
+		git log --no-merges ^HEAD c2
+	} >squash.1-5 &&
+	{
+		echo "Squashed commit of the following:" &&
+		echo &&
+		git log --no-merges ^HEAD c2 c3
+	} >squash.1-5-9 &&
+	echo >msg.nolog &&
+	{
+		echo "* commit 'c3':" &&
+		echo "  commit 3" &&
+		echo
+	} >msg.log
+}
 
-	verify_merge() {
-		test_cmp "$2" "$1" &&
-		git update-index --refresh &&
-		git diff --exit-code &&
-		if test -n "$3"
-		then
-			git show -s --pretty=format:%s HEAD >msg.act &&
-			test_cmp "$3" msg.act
-		fi
-	} &&
+verify_merge () {
+	test_cmp "$2" "$1" &&
+	git update-index --refresh &&
+	git diff --exit-code &&
+	if test -n "$3"
+	then
+		git show -s --pretty=format:%s HEAD >msg.act &&
+		test_cmp "$3" msg.act
+	fi
+}
 
-	verify_head() {
-		echo "$1" >head.expected &&
-		git rev-parse HEAD >head.actual &&
-		test_cmp head.expected head.actual
-	} &&
+verify_head () {
+	echo "$1" >head.expected &&
+	git rev-parse HEAD >head.actual &&
+	test_cmp head.expected head.actual
+}
 
-	verify_parents() {
-		printf "%s\n" "$@" >parents.expected &&
-		>parents.actual &&
-		i=1 &&
-		while test $i -le $#
-		do
-			git rev-parse HEAD^$i >>parents.actual &&
-			i=$(expr $i + 1) ||
-			return 1
-		done &&
-		test_cmp parents.expected parents.actual
-	} &&
+verify_parents () {
+	printf '%s\n' "$@" >parents.expected &&
+	>parents.actual &&
+	i=1 &&
+	while test $i -le $#
+	do
+		git rev-parse HEAD^$i >>parents.actual &&
+		i=$(expr $i + 1) ||
+		return 1
+	done &&
+	test_cmp parents.expected parents.actual
+}
 
-	verify_mergeheads() {
-		printf "%s\n" "$@" >mergehead.expected &&
-		test_cmp mergehead.expected .git/MERGE_HEAD
-	} &&
+verify_mergeheads () {
+	printf '%s\n' "$@" >mergehead.expected &&
+	test_cmp mergehead.expected .git/MERGE_HEAD
+}
 
-	verify_no_mergehead() {
-		! test -e .git/MERGE_HEAD
-	}
-'
+verify_no_mergehead () {
+	! test -e .git/MERGE_HEAD
+}
 
 test_expect_success 'setup' '
 	git add file &&
-- 
1.7.5.1
Jeff King· May 6, 2011, 21:48 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

On Fri, May 06, 2011 at 03:58:52PM -0500, Jonathan Nieder wrote:
Show 6 quoted lines
> Two scripts use a different style with this kind of trivial code
> enclosed by a test assertion; fix them.  The usual style is easier to
> read since there is less indentation to keep track of and no need to
> worry about nested quotes; and on the other hand, because the commands
> in question are trivial, it should not make the test suite any worse
> at catching future bugs in git.

Thanks. Glancing at the first few hunks, this change didn't seem like a big cleanup, but then I got to the hunk with all the ugly '\'' bits. :)

The patch looks correct to me (reviewing with "git diff -b" was a big help).

> While at it, make some other small tweaks:
> 
>  - spell function definitions with a space before () for consistency
>    with other scripts;
Hmm.
  $ cd t
  $ git grep ' ()' *.sh  | wc -l
  271
  $ git grep '[^ ]()' *.sh  | wc -l
  247

I'm not sure you are making things any more consistent. But I don't really care either way.

Just for giggles, I was curious who introduced the styles:
  pattern_authors() {
    git grep -n "$@" |
    while IFS=: read file line match; do
      git blame -L "$line,$line" "$file"
    done |
    perl -lpe '/\((.*?) \d+-\d+-\d+/; $_=$1'
  }
  $ pattern_authors ' ()' t/*.sh | sort | uniq -c | sort -rn
  84 Junio C Hamano
  32 Jonathan Nieder
  16 Thomas Rast
  16 Johan Herland
  12 Johannes Sixt
  12 Johannes Schindelin
  ...
  $ pattern_authors '[^ ]()' t/*.sh | sort | uniq -c | sort -rn
  52 Jeff King
  26 Jonathan Nieder
  18 Nguyễn Thái Ngọc Duy
  16 Wincent Colaiuta
  14 Jon Seymour
  10 Tay Ray Chuan
  ...

So there are definitely particular people who prefer different styles (and I recalled that Junio and I differed on this style point, which is confirmed here). Interestingly, you are the only person to fall right in the middle. I guess that means you are good at emulating surrounding code. :)

-Peff
Jeff King· May 6, 2011, 22:13 UTC · re: Jeff King · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

On Fri, May 06, 2011 at 05:48:01PM -0400, Jeff King wrote:
Show 7 quoted lines
>   pattern_authors() {
>     git grep -n "$@" |
>     while IFS=: read file line match; do
>       git blame -L "$line,$line" "$file"
>     done |
>     perl -lpe '/\((.*?) \d+-\d+-\d+/; $_=$1'
>   }

Two minor complaints on git-blame; maybe somebody can point out something clever I've missed.

  1. blame's "-L" understands patterns already. So in theory I could
     tell it "blame all lines that match pattern X". But I don't think
     there is a way to do that (it tries looking to for _one_ range to
     blame for each -L, not a set of ranges).
  2. Parsing the human-readable output blame output sucks. But parsing
     --porcelain is annoyingly complex for quick-and-dirty things like
     this. It doesn't repeat the commit information per-line.
     I guess we could have an inefficient --line-porcelain format that
     breaks down ranges into single lines and repeats the commit info
     for each one.
     The clever among you may notice that in this particular case,
     though, I could have gotten away with regular --porcelain as I
     blame a single line at a time.
-Peff
Junio C Hamano· May 6, 2011, 22:27 UTC · re: Jeff King · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

Jeff King <peff@peff.net> writes:
> Two minor complaints on git-blame; maybe somebody can point out
> something clever I've missed.
>   1. blame's "-L" understands patterns already.

Teaching blame to take multiple -L options has been one of many longstanding todo item for me. Someday.

>   2. Parsing the human-readable output blame output sucks. But parsing
>      --porcelain is annoyingly complex for quick-and-dirty things like
>      this. It doesn't repeat the commit information per-line.

Non-repetition was quite deliberate, as the reader was expected to have memory proportional to the number of lines in the range, but I agree it is not friendly for quick and dirty hack.

You should be able to add a command line option that disables the early return at the beginning of emit_one_suspect_detail() with a 5-6 lines of patch.

Jeff King· May 6, 2011, 22:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

On Fri, May 06, 2011 at 03:27:08PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Two minor complaints on git-blame; maybe somebody can point out
> > something clever I've missed.
> 
> >   1. blame's "-L" understands patterns already.
> 
> Teaching blame to take multiple -L options has been one of many
> longstanding todo item for me.  Someday.

I think multiple -L is not quite enough. I want a single "-L" that matches every instance of a pattern, like:

  -L "/ ()/,+0"
Show 11 quoted lines
> >   2. Parsing the human-readable output blame output sucks. But parsing
> >      --porcelain is annoyingly complex for quick-and-dirty things like
> >      this. It doesn't repeat the commit information per-line.
> 
> Non-repetition was quite deliberate, as the reader was expected to have
> memory proportional to the number of lines in the range, but I agree it is
> not friendly for quick and dirty hack.
> 
> You should be able to add a command line option that disables the early
> return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
> patch.

I tried that, and it is slightly more involved. You also need to break a multi-line run of lines that blame to a single suspect into its constituent lines. I am 75% of the way to such a patch if you are interested. It's not a lot of code, but it takes some refactoring of emit_porcelain.

-Peff
Junio C Hamano· May 7, 2011, 22:05 UTC · re: Jeff King · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

Jeff King <peff@peff.net> writes:
> I think multiple -L is not quite enough. I want a single "-L" that
> matches every instance of a pattern, like:
>
>   -L "/ ()/,+0"

Of course I am aware that needs to be a part of the multiple -L topic; after all I was the one who invented -L "$regexp" support ;-)

Jeff King· May 9, 2011, 13:31 UTC · re: Jeff King · lore

[PATCH 0/3] blame --line-porcelain

On Fri, May 06, 2011 at 06:29:51PM -0400, Jeff King wrote:
Show 13 quoted lines
> > Non-repetition was quite deliberate, as the reader was expected to have
> > memory proportional to the number of lines in the range, but I agree it is
> > not friendly for quick and dirty hack.
> > 
> > You should be able to add a command line option that disables the early
> > return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
> > patch.
> 
> I tried that, and it is slightly more involved. You also need to break a
> multi-line run of lines that blame to a single suspect into its
> constituent lines. I am 75% of the way to such a patch if you are
> interested. It's not a lot of code, but it takes some refactoring of
> emit_porcelain.
It turned out to not be too bad. Here's the series.
  [1/3]: add tests for various blame formats
  [2/3]: blame: refactor porcelain output
  [3/3]: blame: add --line-porcelain output format
-Peff
Jeff King· May 9, 2011, 13:33 UTC · re: Jeff King · lore

[PATCH 1/3] add tests for various blame formats

We don't seem to have any tests for "blame --porcelain". Let's at least do a trivial test on a simple example.

Signed-off-by: Jeff King <peff@peff.net>
---
I always feel funny putting human-readable output in a test. Maybe it is
not worth including the "normal" output test below, but I assume it's
going to remain pretty stable.
 t/t8008-blame-formats.sh |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100755 t/t8008-blame-formats.sh
Show changes to t/t8008-blame-formats.sh +71 −0
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
new file mode 100755
index 0000000..387d1a6
--- /dev/null
+++ b/t/t8008-blame-formats.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+
+test_description='blame output in various formats on a simple case'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a >file &&
+	git add file
+	test_tick &&
+	git commit -m one &&
+	echo b >>file &&
+	echo c >>file &&
+	echo d >>file &&
+	test_tick &&
+	git commit -a -m two
+'
+
+cat >expect <<'EOF'
+^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d
+EOF
+test_expect_success 'normal blame output' '
+	git blame file >actual &&
+	test_cmp expect actual
+'
+
+ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc
+COMMIT1='author A U Thor
+author-mail <author@example.com>
+author-time 1112911993
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112911993
+committer-tz -0700
+summary one
+boundary
+filename file'
+ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec
+COMMIT2='author A U Thor
+author-mail <author@example.com>
+author-time 1112912053
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112912053
+committer-tz -0700
+summary two
+previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file
+filename file'
+
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+	a
+$ID2 2 2 3
+$COMMIT2
+	b
+$ID2 3 3
+	c
+$ID2 4 4
+	d
+EOF
+test_expect_success 'blame --porcelain output' '
+	git blame --porcelain file >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.5.rc2.8.gc085
Jeff King· May 9, 2011, 13:34 UTC · re: Jeff King · lore

[PATCH 2/3] blame: refactor porcelain output

This is in preparation for adding more porcelain output options. The three changes are:

  1. emit_porcelain now receives the format option flags
  2. emit_one_suspect_detail takes an optional "repeat"
     parameter to suppress the "show only once" behavior
  3. The code for emitting porcelain suspect is factored
     into its own function for repeatability.
There should be no functional changes.
Signed-off-by: Jeff King <peff@peff.net>
---
I broke this out for readability. I can break each of the 3 out into a
separate patch if that helps, but it seemed excessive.
 builtin/blame.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)
Show changes to builtin/blame.c +16 −9
diff --git a/builtin/blame.c b/builtin/blame.c
index 4242e4b..d74e18f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
 /*
  * Porcelain/Incremental format wants to show a lot of details per
  * commit.  Instead of repeating this every line, emit it only once,
- * the first time each commit appears in the output.
+ * the first time each commit appears in the output (unless the
+ * user has specifically asked for us to repeat).
  */
-static int emit_one_suspect_detail(struct origin *suspect)
+static int emit_one_suspect_detail(struct origin *suspect, int repeat)
 {
 	struct commit_info ci;
 
-	if (suspect->commit->object.flags & METAINFO_SHOWN)
+	if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
 		return 0;
 
 	suspect->commit->object.flags |= METAINFO_SHOWN;
@@ -1529,7 +1530,7 @@ static void found_guilty_entry(struct blame_entry *ent)
 		printf("%s %d %d %d\n",
 		       sha1_to_hex(suspect->commit->object.sha1),
 		       ent->s_lno + 1, ent->lno + 1, ent->num_lines);
-		emit_one_suspect_detail(suspect);
+		emit_one_suspect_detail(suspect, 0);
 		write_filename_info(suspect->path);
 		maybe_flush_or_die(stdout, "stdout");
 	}
@@ -1619,7 +1620,15 @@ static const char *format_time(unsigned long time, const char *tz_str,
 #define OUTPUT_NO_AUTHOR       0200
 #define OUTPUT_SHOW_EMAIL	0400
 
-static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
+static void emit_porcelain_details(struct origin *suspect, int repeat)
+{
+	if (emit_one_suspect_detail(suspect, repeat) ||
+	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
+		write_filename_info(suspect->path);
+}
+
+static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
+			   int opt)
 {
 	int cnt;
 	const char *cp;
@@ -1633,9 +1642,7 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
 	       ent->s_lno + 1,
 	       ent->lno + 1,
 	       ent->num_lines);
-	if (emit_one_suspect_detail(suspect) ||
-	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
-		write_filename_info(suspect->path);
+	emit_porcelain_details(suspect, 0);
 
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
@@ -1756,7 +1763,7 @@ static void output(struct scoreboard *sb, int option)
 
 	for (ent = sb->ent; ent; ent = ent->next) {
 		if (option & OUTPUT_PORCELAIN)
-			emit_porcelain(sb, ent);
+			emit_porcelain(sb, ent, option);
 		else {
 			emit_other(sb, ent, option);
 		}
-- 
1.7.5.rc2.8.gc085
Thiago Farina· May 9, 2011, 15:39 UTC · re: Jeff King · lore

Re: [PATCH 2/3] blame: refactor porcelain output

On Mon, May 9, 2011 at 10:34 AM, Jeff King <peff@peff.net> wrote:
Show 40 quoted lines
> This is in preparation for adding more porcelain output
> options. The three changes are:
>
>  1. emit_porcelain now receives the format option flags
>
>  2. emit_one_suspect_detail takes an optional "repeat"
>     parameter to suppress the "show only once" behavior
>
>  3. The code for emitting porcelain suspect is factored
>     into its own function for repeatability.
>
> There should be no functional changes.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I broke this out for readability. I can break each of the 3 out into a
> separate patch if that helps, but it seemed excessive.
>
>  builtin/blame.c |   25 ++++++++++++++++---------
>  1 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4242e4b..d74e18f 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
>  /*
>  * Porcelain/Incremental format wants to show a lot of details per
>  * commit.  Instead of repeating this every line, emit it only once,
> - * the first time each commit appears in the output.
> + * the first time each commit appears in the output (unless the
> + * user has specifically asked for us to repeat).
>  */
> -static int emit_one_suspect_detail(struct origin *suspect)
> +static int emit_one_suspect_detail(struct origin *suspect, int repeat)
>  {
>        struct commit_info ci;
>
> -       if (suspect->commit->object.flags & METAINFO_SHOWN)
> +       if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
Maybe would be worth adding parentheses here:
if (!repeat && (...))
  return 0;
?
Probably is fine as is though.
Jeff King· May 9, 2011, 13:34 UTC · re: Jeff King · lore

[PATCH 3/3] blame: add --line-porcelain output format

This is just like --porcelain, except that we always output the commit information for each line, not just the first time it is referenced. This can make quick and dirty scripts much easier to write; see the example added to the blame documentation.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm not 100% happy with the name, but I couldn't think of anything
better. Something like --verbose-porcelain works, but is a little too
vague. Suggestions welcome.
 Documentation/blame-options.txt |    5 +++++
 Documentation/git-blame.txt     |   13 +++++++++++++
 builtin/blame.c                 |   10 ++++++++--
 t/t8008-blame-formats.sh        |   19 +++++++++++++++++++
 4 files changed, 45 insertions(+), 2 deletions(-)
Show changes to 4 files +45 −2

Documentation/blame-options.txt, Documentation/git-blame.txt, builtin/blame.c, t/t8008-blame-formats.sh

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 16e3c68..e76195a 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -52,6 +52,11 @@ of lines before or after the line given by <start>.
 --porcelain::
 	Show in a format designed for machine consumption.
 
+--line-porcelain::
+	Show the porcelain format, but output commit information for
+	each line, not just the first time a commit is referenced.
+	Implies --porcelain.
+
 --incremental::
 	Show the result incrementally in a format designed for
 	machine consumption.
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index bb8edb4..9516914 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -105,6 +105,19 @@ The contents of the actual line is output after the above
 header, prefixed by a TAB. This is to allow adding more
 header elements later.
 
+The porcelain format generally suppresses commit information that has
+already been seen. For example, two lines that are blamed to the same
+commit will both be shown, but the details for that commit will be shown
+only once. This is more efficient, but may require more state be kept by
+the reader. The `--line-porcelain` option can be used to output full
+commit information for each line, allowing simpler (but less efficient)
+usage like:
+
+	# count the number of lines attributed to each author
+	git blame --line-porcelain file |
+	sed -n 's/^author //p' |
+	sort | uniq -c | sort -rn
+
 
 SPECIFYING RANGES
 -----------------
diff --git a/builtin/blame.c b/builtin/blame.c
index d74e18f..6c26672 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1619,6 +1619,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
 #define OUTPUT_SHOW_SCORE      0100
 #define OUTPUT_NO_AUTHOR       0200
 #define OUTPUT_SHOW_EMAIL	0400
+#define OUTPUT_LINE_PORCELAIN 01000
 
 static void emit_porcelain_details(struct origin *suspect, int repeat)
 {
@@ -1630,6 +1631,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
 static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
 			   int opt)
 {
+	int repeat = opt & OUTPUT_LINE_PORCELAIN;
 	int cnt;
 	const char *cp;
 	struct origin *suspect = ent->suspect;
@@ -1642,15 +1644,18 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
 	       ent->s_lno + 1,
 	       ent->lno + 1,
 	       ent->num_lines);
-	emit_porcelain_details(suspect, 0);
+	emit_porcelain_details(suspect, repeat);
 
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
-		if (cnt)
+		if (cnt) {
 			printf("%s %d %d\n", hex,
 			       ent->s_lno + 1 + cnt,
 			       ent->lno + 1 + cnt);
+			if (repeat)
+				emit_porcelain_details(suspect, 1);
+		}
 		putchar('\t');
 		do {
 			ch = *cp++;
@@ -2307,6 +2312,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
 		OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
 		OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
+		OPT_BIT(0, "line-porcelain", &output_option, "Show porcelain format with per-line commit information", OUTPUT_PORCELAIN|OUTPUT_LINE_PORCELAIN),
 		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
 		OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
 		OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 387d1a6..d15f8b3 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -68,4 +68,23 @@ test_expect_success 'blame --porcelain output' '
 	test_cmp expect actual
 '
 
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+	a
+$ID2 2 2 3
+$COMMIT2
+	b
+$ID2 3 3
+$COMMIT2
+	c
+$ID2 4 4
+$COMMIT2
+	d
+EOF
+test_expect_success 'blame --line-porcelain output' '
+	git blame --line-porcelain file >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.5.rc2.8.gc085
Jonathan Nieder· May 6, 2011, 22:26 UTC · re: Jeff King · lore

Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions

Jeff King wrote:
Show 5 quoted lines
> So there are definitely particular people who prefer different styles
> (and I recalled that Junio and I differed on this style point, which is
> confirmed here). Interestingly, you are the only person to fall right in
> the middle.  I guess that means you are good at emulating surrounding
> code. :)

Probably my older code leaves out the space more often, and newer code includes it. There is an odd kind of logic that can be used to justify including or not including the space, namely:

In C, a function definition starts with an expression that looks something like a function call, as in "double sin(double x);". So when you want to know everything there is to know about the sine function, you can do a "git grep -F -e 'sin('", and it will return to you its definition and a list of callers.

The shell function definition syntax looks oddly like an old-style C prototype "f()". But do not be misled: to duplicate the above property familiar from C, one needs to include a space before the parentheses, so "git grep -F -e 'f '" will return its definition and a list of callers.

Of course the same argument works backwards: if you want the definition without the callers, then only the spaceless syntax will allow you to grep for 'f()'.

Unlike brace placement, this seems to be a question of style with no right answer. :)

Jonathan Nieder· May 6, 2011, 21:00 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] tests: teach verify_parents to check for extra parents

Currently verify_parents only makes sure that the earlier parents of HEAD match the commits given, and does not care if there are more parents. This makes it harder than one would like to check that, for example, parent reduction works correctly when making an octopus.

Fix it by checking that HEAD^(n+1) is not a valid commit name. Noticed while working on a new test that was supposed to create a fast-forward one commit ahead but actually created a merge.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7600-merge.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to t/t7600-merge.sh +1 −0
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index c665acd..9af748a 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -89,6 +89,7 @@ verify_parents () {
 		i=$(expr $i + 1) ||
 		return 1
 	done &&
+	test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
 	test_cmp parents.expected parents.actual
 }
 
-- 
1.7.5.1
Junio C Hamano· May 6, 2011, 21:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 24 quoted lines
> Currently verify_parents only makes sure that the earlier parents of
> HEAD match the commits given, and does not care if there are more
> parents.  This makes it harder than one would like to check that, for
> example, parent reduction works correctly when making an octopus.
>
> Fix it by checking that HEAD^(n+1) is not a valid commit name.
> Noticed while working on a new test that was supposed to create a
> fast-forward one commit ahead but actually created a merge.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  t/t7600-merge.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index c665acd..9af748a 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -89,6 +89,7 @@ verify_parents () {
>  		i=$(expr $i + 1) ||
>  		return 1
>  	done &&
> +	test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
Isn't $i at this point the same as that complex $(()) line noise?
>  	test_cmp parents.expected parents.actual
>  }
Jonathan Nieder· May 6, 2011, 21:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -89,6 +89,7 @@ verify_parents () {
>>  		i=$(expr $i + 1) ||
>>  		return 1
>>  	done &&
>> +	test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
>
> Isn't $i at this point the same as that complex $(()) line noise?
Yes, that sounds better.
Jonathan Nieder· May 6, 2011, 21:32 UTC · re: Junio C Hamano · lore

Re: [PATCH v5] Add default merge options for all branches

Junio C Hamano wrote:
> Subject: [PATCH] merge: fix branch.<name>.mergeoptions
To be more precise, if I understand correctly:
	merge: allow branch.<name>.mergeoptions to override merge.*
> This patch should fix it, even though I now strongly suspect that
> branch.<name>.mergeoptions that gives a single command line that
> needs to be parsed was likely to be an ill-conceived idea to begin
> with.  Sigh...

Yes, and introducing branch.<name>.ff and branch.<name>.log might still be a good idea.

The patch looks mostly good. I see only one actual problem, marked with [*] below.

Show 27 quoted lines
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;
>  static const char **xopts;
>  static size_t xopts_nr, xopts_alloc;
>  static const char *branch;
> +static char *branch_mergeoptions;
>  static int verbosity;
>  static int allow_rerere_auto;
>  
> @@ -474,25 +475,33 @@ cleanup:
>  	strbuf_release(&bname);
>  }
>  
> +static void parse_branch_merge_options(char *bmo)
> +{
> +	const char **argv;
> +	int argc;
> +	char *buf;
> +
> +	if (!bmo)
> +		return;
> +	argc = split_cmdline(bmo, &argv);
> +	if (argc < 0)
> +		die("Bad branch.%s.mergeoptions string", branch);
> +	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> +	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
This is not new code, but it might make sense to do
	argv[0] = "merge.*.options";
for a saner error message when someone tries
	[branch "master"]
		mergeoptions = --nonsense
> +	argc++;
> +	parse_options(argc, argv, NULL, builtin_merge_options,
> +		      builtin_merge_usage, 0);
> +	free(buf);

[*] This buf seems to be left over. (I don't think the intent is to call free on an uninitialized pointer. ;-))

[...]
> -		free(buf);
> +		free(branch_mergeoptions);
> +		branch_mergeoptions = xstrdup(v);
It is tempting to do
	size_t len;
	len = strlen(v);
	branch_mergeoptions = xrealloc(branch_mergeoptions, len + 1);
	memcpy(branch_mergeoptions, v, len + 1);
but free + xstrdup is simpler and clearer.  Makes sense.
Show 5 quoted lines
> +test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
> +	(
> +		git config --remove-section branch.master
> +		git config --remove-section merge
> +	)

Since this patch is meant to apply to a very old git, we cannot use test_might_fail. Makes sense: it can be fixed up later to use &&-friendly syntax as part of a series introducing checks to make sure we don't regress in that.

Thanks and hope that helps, Jonathan

Junio C Hamano· May 6, 2011, 22:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH v5] Add default merge options for all branches

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 22 quoted lines
>> +static void parse_branch_merge_options(char *bmo)
>> +{
>> +	const char **argv;
>> +	int argc;
>> +	char *buf;
>> +
>> +	if (!bmo)
>> +		return;
>> +	argc = split_cmdline(bmo, &argv);
>> +	if (argc < 0)
>> +		die("Bad branch.%s.mergeoptions string", branch);
>> +	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
>> +	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
>
> This is not new code, but it might make sense to do
>
> 	argv[0] = "merge.*.options";
>
> for a saner error message when someone tries
>
> 	[branch "master"]
> 		mergeoptions = --nonsense

Yes, either we stuff a fixed string "branch.*.mergeoptions" to argv[0], or use another static to recall which variable gave us that value and use it. The former is of course easier and less nicer.

Show 8 quoted lines
>> +	argc++;
>> +	parse_options(argc, argv, NULL, builtin_merge_options,
>> +		      builtin_merge_usage, 0);
>> +	free(buf);
>
> [*]
> This buf seems to be left over.  (I don't think the intent is to
> call free on an uninitialized pointer. ;-))
I also forgot to free argv[].

← back to recent threads