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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2011, 00:04 UTC
Message-ID: <7vpqr2oyy3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110208223359.GB17981@elie>

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jared Hance wrote:

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

Drop full-stop '.' at the end.

Show 5 quoted lines

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

I think there is no "prevent" or "breakage"; the patch is to give people a way to turn the feature on; without the configuration, "git merge" will keep the traditional behaviour, no?

Show 13 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...

Yes, your guess is correct.

Show 24 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).

Probably. It is a very good idea to move this code out to its own helper function, nevertheless; I like this part of the patch.

Show 7 quoted lines

> 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", ...);

Much better.