[PATCH v5 0/4] Updated patch series for default upstream merge

[PATCH v5 0/4] Updated patch series for default upstream merge

[Jared Hance]

This patch series allows merge, when given proper configuration, to default
to the upstream of the current branch.

The last one I sent was malformed due to two versions of the patch series
being in the tree at once; this is fixed and has some minor changes from
v4.

Jared Hance (4):
  merge: update the usage information to be more modern
  merge: introduce setup_merge_commit helper function
  merge: introduce per-branch-configuration helper function
  merge: add support for merging from upstream by default

 Documentation/config.txt |    6 +++
 builtin/merge.c          |   90 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 69 insertions(+), 27 deletions(-)

-- 
1.7.4

[PATCH v5 1/4] merge: update the usage information to be more modern

[Jared Hance]

Apparantly, merge's usage information was outdated and used old terminology.
Fix it.

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

diff --git a/builtin/merge.c b/builtin/merge.c
index 42fff38..439d2c7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -37,8 +37,8 @@ struct strategy {
 };
 
 static const char * const builtin_merge_usage[] = {
-	"git merge [options] <remote>...",
-	"git merge [options] <msg> HEAD <remote>",
+	"git merge [options] <branch>...",
+	"git merge [options] <msg> HEAD <branch>",
 	NULL
 };
 
-- 
1.7.4

Re: [PATCH v5 1/4] merge: update the usage information to be more modern

[Drew Northup]

On Thu, 2011-02-10 at 18:52 -0500, Jared Hance wrote:
> Apparantly, merge's usage information was outdated and used old terminology.
> Fix it.
> 
> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---

Jared,
I suspect you meant "Apparently" there...

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

[PATCH v5 2/4] merge: introduce setup_merge_commit helper function

[Jared Hance]

Add a function to set up a merge commit given a branch
or commit, which is currently used when parsing argv in a loop.

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

diff --git a/builtin/merge.c b/builtin/merge.c
index 439d2c7..cd23880 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;
@@ -911,6 +915,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 +1081,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 v5 2/4] merge: introduce setup_merge_commit helper function

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

>  	for (i = 0; i < argc; i++) {
> -		struct object *o;
> ...
> -		strbuf_reset(&buf);
> +		setup_merge_commit(&buf, &remotes, argv[i]);
>  	}

Micronit; the body becomes a single statement and you should lose the {}.

Thanks.  No need to resend.

[PATCH v5 3/4] merge: introduce per-branch-configuration helper function

[Jared Hance]

Create a function that will hold configuration code for configuration
values that are specified per branch, as suggested by Junio.

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

diff --git a/builtin/merge.c b/builtin/merge.c
index cd23880..71b392d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -522,8 +522,20 @@ static int per_branch_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 v5 4/4] merge: add support for merging from upstream by default

[Jared Hance]

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>
---
 Documentation/config.txt |    6 ++++++
 builtin/merge.c          |   32 +++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 9 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 71b392d..a3ca6a0 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] <branch>...",
+	"git merge [options] [<branch>...]",
 	"git merge [options] <msg> HEAD <branch>",
 	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 },
@@ -524,7 +526,8 @@ static int per_branch_config(const char *k, const char *v, void *cb)
 		free(buf);
 
 		return 0;
-	}
+	} else if (!strcmp(variable, ".merge"))
+		return git_config_string(&upstream_branch, k, v);
 
 	return 1; /* not what I handle */
 }
@@ -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);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
@@ -1017,9 +1022,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);
+
+		setup_merge_commit(&buf, &remotes, upstream_branch);
+	}
 
 	/*
 	 * This could be traditional "merge <msg> HEAD <commit>..."  and
@@ -1082,9 +1091,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 v5 4/4] merge: add support for merging from upstream by default

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

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

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

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