Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> 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?
I actually am curious myself, too. I want to see a real-life example.
The "git-gui/*" example you gave is unfortunately not it. It specifies the
branch at the wrong end. Whether I am merging into "master" or "next", I
would want "-s subtree" when I am merging "git-gui/*" project, but all my
merges to "master" or "next" do not necessarily want to use "-s subtree".
It might turn out to be that "branch.<name>.mergeoptions" is a
ill-conceived idea to begin with.
>> 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?
If implemented correctly, it should use the specific one and fall back to
the wildcard one. Another issue is if the values should be cumulative or
overriding, but in the remainder I'd assume we want overriding.
>> @@ -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.
Correct. That is the whole point of using (void *) as a parameter, so
that it can be assigned to the real expected type easily without cast.
>> + 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
> ...
> or even
> ...
> ?
Why not have two string pointers in merge_options_cb structure that are
initialized to NULL and holds the matched config key and the value?
When we are looking at a (k, v) pair in this function, if there is no
previous key in cb, we store (k, v) in cb and return. If there already is
a previous key, we see if k is more specific than that key, and replace
(k, v) in cb with what we currently have. Otherwise we do not do
anything.
When git_config() returns, the caller will find the final value in cb.
>> + 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.)
I actually think test_debug should go inside the previous test. Why not
have it immediately after "git merge c1" above?
--
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