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

[Jonathan Nieder <jrnieder@gmail.com>]

Michael Grubb wrote:

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

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

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

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

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

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

>  	}
>  
> +	if (changed) {
> +		merge_options->key = (char *)k;
> +		merge_options->value = (char *)v;

Why not make the struct fields const?

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

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

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

> @@ -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)' '
[...]
> +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