Re: [PATCH v3] Add default merge options for all branches
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 3, 2011, 18:16 UTC
- Message-ID
- <7vk4e7ir9v.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110503090351.GA27862@elie>
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> 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.
Show 5 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?
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.
Show 9 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.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.
Show 18 quoted lines
>> + 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.
Show 12 quoted lines
>> + 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?