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.

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.

(quoted diff) +1 -3

-	if (branch && !prefixcmp(k, "branch.") &&
-		!prefixcmp(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
+	if (!strcmp(k, "branch.*.mergeoptions"))

Quoted 3 lines · Michael Grubb

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 <[email protected]>

Hi,

Quoted 4 lines · Michael Grubb

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.

Quoted 2 lines

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

Quoted 4 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?

On to the code. Warning: nitpicks ahead.

[...]

Quoted 5 lines

+++ b/builtin/merge.c

builtin/merge.c +0 -0

@@ -32,6 +32,13 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
 

Review line 36

Quoted 2 lines

(quoted diff) +2 -0

+#define MERGEOPTIONS_DEFAULT (1<<0)
+#define MERGEOPTIONS_BRANCH (1<<1)

Are these bitflags?

Review cleanup: · line 517

Quoted 8 lines

(quoted diff) +3 -0

@@ -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.

[...]

Quoted 9 lines

(quoted diff) +6 -3

-	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;

Review

Quoted 5 lines

(quoted diff) +4 -0

+
+	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;
		...

?

builtin/merge.c static int git_merge_config(const char *k, const char *v, void *cb) · line 547

Quoted 12 lines

builtin/merge.c +4 -3

 		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?

builtin/merge.c static int git_merge_config(const char *k, const char *v, void *cb)

Quoted 12 lines

builtin/merge.c +6 -0

+			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.

[...]

t/t7600-merge.sh test_expect_success 'merge c0 with c1 (no-ff)' ' · line 420

Quoted 9 lines

t/t7600-merge.sh +3 -0

@@ -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.

t/t7600-merge.sh test_expect_success 'merge c0 with c1 (no-ff)' ' · line 429

Quoted 9 lines

t/t7600-merge.sh +9 -0

+	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.)

t/t7600-merge.sh test_expect_success 'merge c0 with c1 (no-ff)' ' · line 433

Quoted 4 lines

t/t7600-merge.sh +4 -0

+
+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.

t/t7600-merge.sh test_expect_success 'merge c0 with c1 (no-ff)' ' · line 443

Quoted 10 lines

t/t7600-merge.sh +10 -0

+	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 <[email protected]>

Thanks.

---
 builtin/merge.c  |   37 ++++++++++++++++++++-----------------
 t/t7600-merge.sh |    4 ++--
 2 files changed, 22 insertions(+), 19 deletions(-)

builtin/merge.c +20 -17

index 9fe129f..7156e92 100644
@@ -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"))

t/t7600-merge.sh +2 -2

index cea2b31..ff807f4 100755
@@ -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 &&

So in the future one might be able to do things like

	[branch "git-gui/*"]
		mergeoptions = -s subtree

Interesting.

Quoted 2 lines

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?

Quoted 3 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?

Review cleanup: · line 517

Quoted 7 lines

(quoted diff) +3 -0

@@ -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.

Quoted 10 lines

(quoted diff) +10 -0

+	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) {

builtin/merge.c static int git_merge_config(const char *k, const char *v, void *cb) · line 537

Quoted 1 line

builtin/merge.c +0 -0

 		const char **argv;

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

Quoted 4 lines

(quoted diff) +4 -0

+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&

Review cleanup: · line 537

Quoted 5 lines

(quoted diff) +5 -0

+	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.)

Quoted 7 lines · Jonathan Nieder

So in the future one might be able to do things like

	[branch "git-gui/*"]
		mergeoptions = -s subtree

Interesting.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 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.

Quoted 1 line

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.

Quoted 9 lines

Review cleanup: · line 517

Quoted 7 lines

(quoted diff) +3 -0

@@ -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.

Quoted 10 lines

Quoted 10 lines

(quoted diff) +10 -0

+	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) {

Quoted 8 lines

builtin/merge.c static int git_merge_config(const char *k, const char *v, void *cb) · line 537

Quoted 1 line

builtin/merge.c +0 -0

 		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.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+	git config "branch.*.mergeoptions" "--no-ff" &&
+	test_tick &&
+	git merge c1 &&
+	git config --remove-section "branch.*" &&

Quoted 8 lines

Review cleanup: · line 537

Quoted 5 lines

(quoted diff) +5 -0

+	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?

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Junio C Hamano

Quoted 2 lines · Jonathan Nieder

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?

Quoted 5 lines · Jonathan Nieder

Quoted 2 lines

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 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.

builtin/merge.c +0 -0

@@ -32,6 +32,21 @@
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)

builtin/merge.c +2 -15

@@ -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));

builtin/merge.c +5 -0

+		return;
+
+	buf = xstrdup(opts->value);
+	argc = split_cmdline(buf, &argv);
+	if (argc < 0)

t/t7600-merge.sh +0 -0

@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '

t/t7600-merge.sh +9 -0

+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 &&

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".

Quoted 2 lines · Jonathan Nieder

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.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Jonathan Nieder

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

Quoted 7 lines

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

Documentation/merge-config.txt +6 -0

index 8920258..2aa4408 100644
@@ -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

builtin/merge.c +3 -1

index d171c63..5194f04 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I take this to mean that my patch is no longer needed/wanted?

Quoted 1 line · Michael Grubb

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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...

Quoted 11 lines · Junio C Hamano

Quoted 2 lines · Jonathan Nieder

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

Quoted 1 line

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

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 <[email protected]>

---
 Documentation/git-merge.txt |    3 +
 builtin/merge.c             |   90 +++++++++++++++++++++++++++++++++---------
 t/t7600-merge.sh            |   39 ++++++++++++++++++
 3 files changed, 112 insertions(+), 20 deletions(-)

Documentation/git-merge.txt +3 -0

index e2e6aba..eaab3e4 100644
@@ -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
 --------

builtin/merge.c +70 -20

index d171c63..d6ce85e 100644
@@ -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)

t/t7600-merge.sh +39 -0

index e84e822..5b1f8e1 100755
@@ -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

+/* 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.
+ */

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.

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;

  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.

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.

Quoted 4 lines · Jeff King

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.

Quoted 3 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 think multiple -L is not quite enough. I want a single "-L" that matches every instance of a pattern, like:

-L "/ ()/,+0"

Quoted 7 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.

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 <[email protected]>

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

builtin/blame.c +0 -1

index 4242e4b..d74e18f 100644
@@ -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.

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. :)

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 <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---
 t/t7600-merge.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

t/t7600-merge.sh +0 -0

index c665acd..9af748a 100755

t/t7600-merge.sh +0 -0

 	test_cmp parents.expected parents.actual
 }

t/t7600-merge.sh verify_parents () { · line 92

Quoted 7 lines · Jonathan Nieder

t/t7600-merge.sh +1 -0

@@ -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?

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

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...

builtin-merge.c +4 -0

@@ -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;

builtin-merge.c cleanup: · line 490

Quoted 13 lines

builtin-merge.c +13 -0

+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

builtin-merge.c cleanup: · line 494

Quoted 4 lines

builtin-merge.c +4 -0

+	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. ;-))

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
...

Documentation/merge-config.txt +1 -0

@@ -16,6 +16,16 @@ merge.defaultToUpstream::
 	to their corresponding remote tracking branches, and the tips of
 	these tracking branches are merged.
 
+merge.ff::

builtin/merge.c +0 -0

@@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		if (is_bool && shortlog_len)

builtin/merge.c cleanup: · line 508

Quoted 12 lines · Michael Grubb

builtin/merge.c +1 -3

index 0bdd19a..9e5b6bd 100644
@@ -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".

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html