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

[Michael Grubb <devel@dailyvoid.com>]

On 5/3/11 1:16 PM, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> 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.
> 
In my reply to Jonathan I gave the example of turning off fast forward merges
globally for all branches (and then perhaps turning them back on for specific branches).
The same could be done with --log or any other option that might make since to turn on
for all branches without having to specifically name each branch in the config file.

Also as previously cited in my earlier reply to Jonathan, a good real life example can be
found in Vincent Driessen's branching model where he creates ephemeral feature/topic branches
for development then deletes them when they get merged back into the mainline development tree.
However, he wants to keep the history that there was a merge from a branch instead of it just looking
like a straight line.  The link to his work is here: http://nvie.com/posts/a-successful-git-branching-model/


> 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.
> 
There is now a unit test for this scenario.

>>> @@ -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.
> 
This has been corrected in the latest version of the patch.

>>> +	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?
> 
That was a great idea.  I've reworked things around this and also moved
to a voting type method for determining priority of the keys.  So no more
defines needed.  I think keeping all that state in the struct is much better
and scales better as well.

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

Again I followed the existing pattern here. I didn't want to be the odd man out.
Do you want me to make that change?
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>