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

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
May 3, 2011, 20:44 UTC
Message-ID
<20110503204442.GI1019@elie>
In-Reply-To
<4DC0608F.9040208@dailyvoid.com>
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

Previous: Michael GrubbNext: Junio C Hamano
Message 15 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.