Re: [PATCH v5 4/4] merge: add support for merging from upstream by default

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2011, 01:20 UTC
Message-ID: <7vsjvv1i5u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297381964-7137-5-git-send-email-jaredhance@gmail.com>

Jared Hance <jaredhance@gmail.com> writes:

Show 6 quoted lines

> Add the option merge.defaultupstream to add support for merging from
> the upstream branch by default. The upstream branch is found using
> branch.[name].merge.
>
> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---

Thanks; the first three in the series looks right.

Show 9 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c5e1835..4415691 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1389,6 +1389,12 @@ man.<tool>.path::
>  
>  include::merge-config.txt[]
>  
> +merge.defaultUpstream::

I somehow had an impression that majority of others convinced you to rename this to default-to-upstream, but I may be mistaken.

I think somebody who does not know the history of the development and discussion of this feature would think that this specifies the default upstream remote (i.e. not a boolean variable, but a string) given this name.

Show 9 quoted lines

> @@ -536,7 +539,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  	if (status <= 0)
>  		return status;
>  
> -	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
> +	if (!strcmp(k, "merge.defaultupstream"))
> +		default_upstream = git_config_bool(k, v);
> +	else if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
>  		show_diffstat = git_config_bool(k, v);

It is somewhat rude to reviewers' eyes to turn an existing "if" into "else if" and place new stuff at the beginning; unless there is a good reason that the new stuff has to be at the beginning, that is.

This is not a new issue, but a callback function to git_config() should return once it recognized and handled the variable, instead of cascading the controll out. The "diffstat/stat" code shows a bad example and you inherited the badness from there.