git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
May 3, 2011, 09:03 UTC
Message-ID
<20110503090351.GA27862@elie>
In-Reply-To
<4DBF94E9.2010502@dailyvoid.com>
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(-)
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
Previous: Michael GrubbNext: Jonathan Nieder
Message 6 of 46 in “Add default merge options for all branches”
  1. Add default merge options for all branchesMichael Grubb, May 2, 2011
  2. Miklos VajnaMay 2, 2011
  3. Junio C HamanoMay 2, 2011
  4. Michael GrubbMay 3, 2011
  5. Add default merge options for all branchesMichael Grubb, May 3, 2011
  6. Jonathan NiederMay 3, 2011
  7. Jonathan NiederMay 3, 2011
  8. Michael GrubbMay 3, 2011
  9. Junio C HamanoMay 3, 2011
  10. Michael GrubbMay 3, 2011
  11. Jonathan NiederMay 3, 2011
  12. Jens LehmannMay 3, 2011
  13. Add default merge options for all branchesMichael Grubb, May 3, 2011
  14. Michael GrubbMay 3, 2011
  15. Jonathan NiederMay 3, 2011
  16. Junio C HamanoMay 3, 2011
  17. Junio C HamanoMay 4, 2011
  18. Michael GrubbMay 4, 2011
  19. Jonathan NiederMay 4, 2011
  20. Michael GrubbMay 4, 2011
  21. Junio C HamanoMay 4, 2011
  22. John SzakmeisterMay 4, 2011
  23. Junio C HamanoMay 3, 2011
  24. Add default merge options for all branchesMichael Grubb, May 3, 2011
  25. Add default merge options for all branchesMichael Grubb, May 4, 2011
  26. Junio C HamanoMay 5, 2011
  27. Junio C HamanoMay 6, 2011
  28. Jonathan NiederMay 6, 2011
  29. 0/2 tests: make verify_merge check that the number of parents is rightJonathan Nieder, May 6, 2011
  30. 1/2 tests: eliminate unnecessary setup test assertionsJonathan Nieder, May 6, 2011
  31. Jeff KingMay 6, 2011
  32. Jeff KingMay 6, 2011
  33. Junio C HamanoMay 6, 2011
  34. Jeff KingMay 6, 2011
  35. Junio C HamanoMay 7, 2011
  36. 0/3 blame --line-porcelainJeff King, May 9, 2011
  37. 1/3 add tests for various blame formatsJeff King, May 9, 2011
  38. 2/3 blame: refactor porcelain outputJeff King, May 9, 2011
  39. Thiago FarinaMay 9, 2011
  40. 3/3 blame: add --line-porcelain output formatJeff King, May 9, 2011
  41. Jonathan NiederMay 6, 2011
  42. 2/2 tests: teach verify_parents to check for extra parentsJonathan Nieder, May 6, 2011
  43. Junio C HamanoMay 6, 2011
  44. Jonathan NiederMay 6, 2011
  45. Jonathan NiederMay 6, 2011
  46. Junio C HamanoMay 6, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.