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

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

[Jared Hance]

Adds the option merge.defaultupstream to add support for merging from the
upstream branch by default. The upstream branch is found using
branch.[name].merge.

Also add helper functions `per_branch_config` and `setup_merge_commit` to
reduce reduncancy and impove clarity.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 Documentation/config.txt |    5 +++
 builtin/merge.c          |   81 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c5e1835..25c8292 100644
--- 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.
+
 mergetool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
diff --git a/builtin/merge.c b/builtin/merge.c
index 42fff38..2b4fd4f 100644
--- a/builtin/merge.c
+++ 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
 };
@@ -58,6 +58,8 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int default_upstream;
+static const char *upstream_branch;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -498,11 +500,15 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-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 */
+
+	variable = k + 7 + strlen(branch);
+	if(!strcmp(variable, ".mergeoptions")) {
 		const char **argv;
 		int argc;
 		char *buf;
@@ -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;
 	}
+	else if(strcmp(variable, ".merge")) {
+		return git_config_string(&upstream_branch, k, v);
+	}
+
+	return 1; /* not what I handle */
+}
 
-	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+	int status = per_branch_config(k, v, cb);
+
+	if (status <= 0)
+		return status;
+
+	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);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
@@ -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);
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -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) {
+			setup_merge_commit(&buf, &remotes, upstream_branch);
+		}
+		else {
+			usage_with_options(builtin_merge_usage,
+					builtin_merge_options);
+		}
+	}
 
 	/*
 	 * This could be traditional "merge <msg> HEAD <commit>..."  and
@@ -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)))
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
@@ -1059,19 +1106,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	strbuf_reset(&buf);
 
 	for (i = 0; i < argc; i++) {
-		struct object *o;
-		struct commit *commit;
-
-		o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);
-		if (!o)
-			die("%s - not something we can merge", argv[i]);
-		commit = lookup_commit(o->sha1);
-		commit->util = (void *)argv[i];
-		remotes = &commit_list_insert(commit, remotes)->next;
-
-		strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
-		setenv(buf.buf, argv[i], 1);
-		strbuf_reset(&buf);
+		setup_merge_commit(&buf, &remotes, argv[i]);
 	}
 
 	if (!use_strategies) {
-- 
1.7.4

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

[Jay Soffian]

On Tue, Feb 8, 2011 at 3:48 PM, Jared Hance <jaredhance@gmail.com> wrote:
>
> Adds the option merge.defaultupstream to add support for merging from the

Nit: I agree with Jeff[1] that this should be called merge.defaultToUpstream

[1] http://article.gmane.org/gmane.comp.version-control.git/165745

j.

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

[Jonathan Nieder]

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 :)

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

> +++ 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;
[...]
> -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.

> @@ -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.

[...]
> @@ -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).

> @@ -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(...);

[...]
> @@ -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

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

[Junio C Hamano]

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.

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

>> +++ 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.

>> @@ -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.

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

[PATCH v3 0/3] Updated patch series for default upstream merge

[Jared Hance]

This patch series allows for `git merge` to default to the upstream branch
of the current branch.

Notes/Complications:
	- I'm not sure whether the option should be merge.defaultUpstream
	  or merge.defaultToUpstream
	- Should [remotes] be changed to [branches]? I felt like it was
	  a completely different change and didn't belong in the patch series.
	- I left one of the ifs with unnecessary braces for clarity because
	  of a nested if-else: is this the preferred style?

Jared Hance (3):
  Add setup_merge_commit function to merge/builtin.c.
  Add function per_branch_config.
  Add support for merging from upstream by default.

 Documentation/config.txt |    6 +++
 builtin/merge.c          |   87 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 68 insertions(+), 25 deletions(-)

-- 
1.7.4

[PATCH v3 1/3] Add setup_merge_commit function to merge/builtin.c.

[Jared Hance]

Adds a new function that will be used in a later patch as well as the
current code to reduce redundancy.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/merge.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 42fff38..bc1ae94 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -911,6 +911,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);
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -1059,19 +1077,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	strbuf_reset(&buf);
 
 	for (i = 0; i < argc; i++) {
-		struct object *o;
-		struct commit *commit;
-
-		o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);
-		if (!o)
-			die("%s - not something we can merge", argv[i]);
-		commit = lookup_commit(o->sha1);
-		commit->util = (void *)argv[i];
-		remotes = &commit_list_insert(commit, remotes)->next;
-
-		strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
-		setenv(buf.buf, argv[i], 1);
-		strbuf_reset(&buf);
+		setup_merge_commit(&buf, &remotes, argv[i]);
 	}
 
 	if (!use_strategies) {
-- 
1.7.4

[PATCH v3 2/3] Add function per_branch_config.

[Jared Hance]

Adds a configuration function to be filled up more in the next patch.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/merge.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bc1ae94..818bfc7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -498,11 +498,15 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-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; /* not what I handle */
+
+	variable = k + 7 + strlen(branch);
+	if (!strcmp(variable, ".mergeoptions")) {
 		const char **argv;
 		int argc;
 		char *buf;
@@ -518,8 +522,20 @@ 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;
 	}
 
+	return 1; /* not what I handle */
+}
+
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+	int status = per_branch_config(k, v, cb);
+
+	if (status <= 0)
+		return status;
+
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
-- 
1.7.4

[PATCH v3 3/3] Add support for merging from upstream by default.

[Jared Hance]

Adds 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>
---
 Documentation/config.txt |    6 ++++++
 builtin/merge.c          |   31 +++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 8 deletions(-)

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::
+	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, possibly set by --track or
+	--set-upstream.
+
 mergetool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
diff --git a/builtin/merge.c b/builtin/merge.c
index 818bfc7..cd646fb 100644
--- a/builtin/merge.c
+++ 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
 };
@@ -58,6 +58,8 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int default_upstream;
+static const char *upstream_branch;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -525,6 +527,8 @@ static int per_branch_config(const char *k, const char *v, void *cb)
 
 		return 0;
 	}
+	else if (!strcmp(variable, ".merge"))
+		return git_config_string(&upstream_branch, k, v);
 
 	return 1; /* not what I handle */
 }
@@ -536,7 +540,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);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
@@ -1017,9 +1023,13 @@ 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)
+			usage_with_options(builtin_merge_usage,
+					builtin_merge_options);
+		else
+			setup_merge_commit(&buf, &remotes, upstream_branch);
+	}
 
 	/*
 	 * This could be traditional "merge <msg> HEAD <commit>..."  and
@@ -1082,9 +1092,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (head_invalid || !argc)
-		usage_with_options(builtin_merge_usage,
-			builtin_merge_options);
+	if (head_invalid)
+		usage_msg_opt("cannot use old-style invocation from an unborn"
+				"branch", 
+				builtin_merge_usage, builtin_merge_options);
+
+	if (!argc && !(default_upstream && upstream_branch))
+		usage_msg_opt("no commit to merge specified",
+				builtin_merge_usage, builtin_merge_options);
 
 	strbuf_addstr(&buf, "merge");
 	for (i = 0; i < argc; i++)
-- 
1.7.4

Re: [PATCH v3 0/3] Updated patch series for default upstream merge

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> Notes/Complications:
> 	- I'm not sure whether the option should be merge.defaultUpstream
> 	  or merge.defaultToUpstream

Probably the latter.

> 	- Should [remotes] be changed to [branches]? I felt like it was
> 	  a completely different change and didn't belong in the patch series.

Perhaps at the beginning (just like your 1/3 refactoring) or at the end as
a separate patch?

> 	- I left one of the ifs with unnecessary braces for clarity because
> 	  of a nested if-else: is this the preferred style?

If you are talking about the one at @@ -1017,9 +1023,13 @@, it looks fine
to me.  The new "else" you added to @@ -525,6 +527,8 @@ may probably want
to start on the same line as closing "}" of the "if", though.  IOW, like
this:

	if (cond) {
        	...
	} else {
        	...
	}

not like this:

	if (cond) {
        	...
	}
        else {
        	...
	}

But other than that looks reasonable.

Thanks.

Re: [PATCH v3 2/3] Add function per_branch_config.

[Jonathan Nieder]

Jared Hance wrote:

> [PATCH v3 2/3] Add function per_branch_config.
>
> Adds a configuration function to be filled up more in the next patch.

Micronit: subject should be of the form

	subsystem: do something really great

with no full stop.  So the series might be, roughly:

 - merge: introduce setup_merge_commit helper function
 - merge: introduce per-branch configuration helper function
 - merge: add support for merging from upstream by default

Commit message bodies tend to be in the imperative mood, just like the
patches themselves.

Anyway, I like patch 1.  One tiny nit on this one:

> +++ b/builtin/merge.c
> @@ -498,11 +498,15 @@ cleanup:
>  	strbuf_release(&bname);
>  }
>  
> -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; /* not what I handle */

Alignment: isn't the "||" meant to be one space over?

Based on a quick grep[1], it seems the prevailing style is rather

	if (!branch || prefixcmp(k, "branch.") ||
	    prefixcmp(k + 7, branch))
		return 1;

with the || at the end of the line, though I'm not sure I care
much.  :)

With or without an extra space before the ||,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for a nice cleanup.
Jonathan

[1]
addup () {
	sum=0
	while read term
	do
		: $((sum = sum + term))
	done
	echo $sum
}

 $ git grep --cached -c -e '||$' | cut -d: -f2 | addup
 730
 $ git grep --cached -c -e '  ||' | cut -d: -f2 | addup
 98
 $ git grep --cached -c -e '	||' | cut -d: -f2 | addup
 79

Re: [PATCH v3 3/3] Add support for merging from upstream by default.

[Jonathan Nieder]

Jared Hance wrote:

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

Might make sense to put this in merge-config.txt, to get documentation
in git-merge(1) for free.

> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -525,6 +527,8 @@ static int per_branch_config(const char *k, const char *v, void *cb)
>  
>  		return 0;
>  	}
> +	else if (!strcmp(variable, ".merge"))
> +		return git_config_string(&upstream_branch, k, v);

Style:

	} else if (...) {
		return git_config_string(...);
	}

> @@ -1017,9 +1023,13 @@ 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)
> +			usage_with_options(builtin_merge_usage,
> +					builtin_merge_options);
> +		else
> +			setup_merge_commit(&buf, &remotes, upstream_branch);

Why the "else"?  usage_msg_opt doesn't return to the caller.

Hopefully someone will chime in with some tests to complement
t5520-pull (hint, hint). :)

Jonathan

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c1efaaa..bc8ce25 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash]
 	[-s <strategy>] [-X <strategy-option>]
-	[--[no-]rerere-autoupdate] [-m <msg>] <commit>...
+	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
 
@@ -97,7 +97,10 @@ commit or stash your changes before running 'git merge'.
 	Commits, usually other branch heads, to merge into our branch.
 	You need at least one <commit>.  Specifying more than one
 	<commit> obviously means you are trying an Octopus.
-
++
+By default, if no commit is specified then git will error out.
+However, the `merge.defaultToUpstream` configuration item (see below)
+changes that.
 
 PRE-MERGE CHECKS
 ----------------

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

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

Yes.  One answer to my question is that a person might try

	git fetch git://some/random/repository branch
	git merge

and at least for now it seems sensible to make that error out by
default rather than doing something unexpected.

In other words, another possible rule is "default to the last fetched
branch" (FETCH_HEAD), so the intent behind running "git merge" without
a branchname for the first time is not as obvious as I would hope.

Re: [PATCH v3 1/3] Add setup_merge_commit function to merge/builtin.c.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> Subject: Re: [PATCH v3 1/3] Add setup_merge_commit function to merge/builtin.c.

Please spell the name of the file you are touching correctly ;-).

	Subject: builtin/merge.c: Add setup_merge_commit function

> Adds a new function that will be used in a later patch as well as the
> current code to reduce redundancy.

Redandunt.  If you say "add", we know the function is "new" ;-)

State what that function does instead of a vague "reduce redundancy",
perhaps like...

	Refactor the loop to parse list of commits given from the command
        line into a separate helper function.

The patch itself looks good.

Thanks.

Re: [PATCH v3 2/3] Add function per_branch_config.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> Adds a configuration function to be filled up more in the next patch.

Again, "to be filled up more" is not the primary purpose of the new
function that was refactored from the existing one.  State what it does,
and what it is for instead, perhaps like...

    Split a part of the configuration callback into a separate function
    to handle per-branch configuration.