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

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

From
Michael Grubb <devel@dailyvoid.com>
Date
May 3, 2011, 16:46 UTC
Message-ID
<4DC03182.505@dailyvoid.com>
In-Reply-To
<20110503090351.GA27862@elie>
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 &&
Previous: Jonathan NiederNext: Junio C Hamano
Message 8 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.