Re: [PATCH v2] Add support for merging from upstream by default.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 8, 2011, 22:33 UTC
Message-ID: <20110208223359.GB17981@elie>
In-Reply-To: <1297198129-3403-1-git-send-email-jaredhance@gmail.com>

Jared Hance wrote:

> Adds the option merge.defaultupstream to add support for merging from the
> upstream branch by default.

Could you give an example of breakage this configurability is designed to prevent?

Not that it is a bad idea to be careful anyway, mind you --- just looking for a clear answer for when people ask "and why would I ever want to set merge.defaultToUpstream to false?"

> reduce reduncancy and impove clarity.

redundancy, improve :)

Show 10 quoted lines

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1389,6 +1389,11 @@ man.<tool>.path::
>  
>  include::merge-config.txt[]
>  
> +merge.defaultUpstream::
> +	If merge is called without any ref arguments, merge from the branch
> +	specified in branch.<current branch>.merge, which is considered to be
> +	the upstream branch for the current branch.

I'd prefer to say "merge from the branch configured with --track or --set-upstream" (but that's just me).

Show 9 quoted lines

> +++ b/builtin/merge.c
> @@ -37,7 +37,7 @@ struct strategy {
>  };
>  
>  static const char * const builtin_merge_usage[] = {
> -	"git merge [options] <remote>...",
> +	"git merge [options] [<remote>...]",
>  	"git merge [options] <msg> HEAD <remote>",
>  	NULL

Side note: these should probably say "<commit>" or "<branch>" rather than "<remote>". I'm guessing the usage string comes from the days before the separate-remotes ref layout...

> @@ -58,6 +58,8 @@ static int option_renormalize;

[...]

Show 10 quoted lines

> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static int per_branch_config(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 *variable;
> +	if(!branch || prefixcmp(k, "branch.")
> +	   || prefixcmp(k + 7, branch))
> +		return 1; /* ignore me */

Style: missing space after "if" keyword.

Clarity: we are not supposed to _ignore_ the non-branch.*
configuration, just leave it for other functions to handle, no?
Maybe the comment should say "let others handle it" or something?

> +
> +	variable = k + 7 + strlen(branch);

'7' can be written in a more self-explanatory way as 'strlen("branch.")' and the optimizer will take care of translating it to 7. Don't worry about it if that makes the diff or code harder to read, though; I'm just mentioning the trick for future reference.

> +	if(!strcmp(variable, ".mergeoptions")) {

Style: missing space after "if" keyword.

Show 6 quoted lines

> @@ -518,9 +524,26 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		parse_options(argc, argv, NULL, builtin_merge_options,
>  			      builtin_merge_usage, 0);
>  		free(buf);
> +
> +		return 0;

I'd group the cleanup with the return.

	parse_options(...);

	free(buf);
	return 0;

>  	}
> +	else if(strcmp(variable, ".merge")) {

Style: missing space after "if".  Uncuddled brace.

[...]

Show 21 quoted lines

> @@ -911,6 +934,24 @@ static int evaluate_result(void)
>  	return cnt;
>  }
>  
> +static void setup_merge_commit(struct strbuf *buf,
> +	struct commit_list ***remotes, const char *s)
> +{
> +	struct object *o;
> +	struct commit *commit;
> +
> +	o = peel_to_type(s, 0, NULL, OBJ_COMMIT);
> +	if (!o)
> +		die("%s - not something we can merge", s);
> +	commit = lookup_commit(o->sha1);
> +	commit->util = (void *)s;
> +	*remotes = &commit_list_insert(commit, *remotes)->next;
> +
> +	strbuf_addf(buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
> +	setenv(buf->buf, s, 1);
> +	strbuf_reset(buf);
> +}

Would be easier to review if this code movement were in a separate patch (separating cleanup from semantic changes).

Show 9 quoted lines

> @@ -983,9 +1024,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	if (!allow_fast_forward && fast_forward_only)
>  		die("You cannot combine --no-ff with --ff-only.");
>  
> -	if (!argc)
> -		usage_with_options(builtin_merge_usage,
> -			builtin_merge_options);
> +	if (!argc) {
> +		if(default_upstream && upstream_branch) {

Style: missing space after "if".  Unnecessary braces.  To avoid
keeping the reader in suspense, it's usually best to handle the
(simple) error case first, like so:

		if (!default_upstream || !upstream_branch)
			usage_with_options(...);
		setup_merge_commit(...);

[...]

Show 6 quoted lines

> @@ -1048,7 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> -	if (head_invalid || !argc)
> +	if (head_invalid || (!argc && !(default_upstream && upstream_branch)))

Might be clearer to split the line?

	if (head_invalid
	    || (!argc && (!default_upstream || !upstream_branch)))
		usage_with_options(...);
    
Even better would be to use descriptive messages, like so:

 if (head_invalid)
	usage_msg_opt("cannot use old-style invocation from an unborn branch",
		...);
 if (!argc && ...)
	usage_msg_opt("no commit to merge specified", ...);

Thanks for making this happen. :) Jonathan