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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html