[PATCH] Allow multiple merges to invalid HEAD

[PATCH] Allow multiple merges to invalid HEAD

[Timothy Chen]

Currently git merge only allows one branch when current HEAD
is not yet pointing to a valid commit.

This patch will allow multiple branches to be passed in,
and first updates current HEAD to the first branch's head then subsequently
merge the rest of the branches.
---
 builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index d54e7dd..290e0d4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1090,9 +1090,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * to forbid "git merge" into a branch yet to be born.
 		 * We do the same for "git pull".
 		 */
-		if (argc != 1)
-			die(_("Can merge only exactly one commit into "
-				"empty head"));
 		if (squash)
 			die(_("Squash commit into empty head not supported yet"));
 		if (!allow_fast_forward)
@@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
 		if (!remote_head)
 			die(_("%s - not something we can merge"), argv[0]);
-		read_empty(remote_head->sha1, 0);
 		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
 				DIE_ON_ERR);
-		return 0;
-	} else {
-		struct strbuf merge_names = STRBUF_INIT;
 
-		/* We are invoked directly as the first-class UI. */
+		if (argc < 2)
+			return 0;
+
+		hashcpy(head, remote_head->sha1);
+		read_empty(remote_head->sha1, 0);
+		head_arg = argv[0];
+		argc--;
+		argv++;
+	}
+
+	struct strbuf merge_names = STRBUF_INIT;
+
+	/* We are invoked directly as the first-class UI. */
+	if(!head_invalid)
 		head_arg = "HEAD";
 
-		/*
-		 * All the rest are the commits being merged;
-		 * prepare the standard merge summary message to
-		 * be appended to the given message.  If remote
-		 * is invalid we will die later in the common
-		 * codepath so we discard the error in this
-		 * loop.
-		 */
-		for (i = 0; i < argc; i++)
-			merge_name(argv[i], &merge_names);
-
-		if (!have_message || shortlog_len) {
-			fmt_merge_msg(&merge_names, &merge_msg, !have_message,
-				      shortlog_len);
-			if (merge_msg.len)
-				strbuf_setlen(&merge_msg, merge_msg.len - 1);
-		}
+	/*
+	 * All the rest are the commits being merged;
+	 * prepare the standard merge summary message to
+	 * be appended to the given message.  If remote
+	 * is invalid we will die later in the common
+	 * codepath so we discard the error in this
+	 * loop.
+	 */
+	for (i = 0; i < argc; i++)
+		merge_name(argv[i], &merge_names);
+
+	if (!have_message || shortlog_len) {
+		fmt_merge_msg(&merge_names, &merge_msg, !have_message,
+				  shortlog_len);
+		if (merge_msg.len)
+			strbuf_setlen(&merge_msg, merge_msg.len - 1);
 	}
 
-	if (head_invalid || !argc)
+	if (!argc)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
-- 
1.7.3.4

Re: [PATCH] Allow multiple merges to invalid HEAD

[Junio C Hamano]

Timothy Chen <tnachen@gmail.com> writes:

> Subject: Re: [PATCH] Allow multiple merges to invalid HEAD

I thought somebody already suggested to reword "invalid HEAD" to something
more sensible, like "unborn branch", but I may be misremembering things...

> Currently git merge only allows one branch when current HEAD
> is not yet pointing to a valid commit.
>
> This patch will allow multiple branches to be passed in,
> and first updates current HEAD to the first branch's head then subsequently
> merge the rest of the branches.
> ---

Missing sign-off.

What's the point of being able to do this in the first place?

The _only_ reason "git pull $there $branch" into an unborn branch exists
is because it feels like

	$ git clone $there $directory

could be broken down as this command sequence:

	$ mkdir $directory
        $ cd $directory
        $ git init
        $ git remote add origin $there
	$ git fetch
        $ git pull $there ;# or "git merge origin/master"

And all the above steps actually do work, except for the last step and
that is why we added the "pull into emptiness" hack.

Even though it does a very different thing at the mechanical level, having
the hack helps these two sequences (a single "clone" vs the "init/remote
add/fetch/merge") that are conceptually similar to behave the same.

I however don't think of a halfway sane rationale similar to why we have
"merge one into emptiness" to justify this patch.  It very much feels like
"because I can make things more complex", not "because this is an often
desired missing feature that is a major pain point".

Re: [PATCH] Allow multiple merges to invalid HEAD

[Junio C Hamano]

Timothy Chen <tnachen@gmail.com> writes:

> This patch will allow multiple branches to be passed in,
> and first updates current HEAD to the first branch's head then subsequently
> merge the rest of the branches.

I've questioned the motivation of the patch already, but let's comment on
the mechanics as well while I am waiting for some builds to finish ;-)

> diff --git a/builtin/merge.c b/builtin/merge.c
> index d54e7dd..290e0d4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1090,9 +1090,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 * to forbid "git merge" into a branch yet to be born.
>  		 * We do the same for "git pull".
>  		 */
>  		if (squash)
>  			die(_("Squash commit into empty head not supported yet"));
>  		if (!allow_fast_forward)
> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
>  		if (!remote_head)
>  			die(_("%s - not something we can merge"), argv[0]);
>  		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
>  				DIE_ON_ERR);

You are going to perform a series of operations that is a lot more complex
than what we traditionally have done at this point.  I do not think it is
safe at all to update the ref this early before even knowing if the rest
of the command succeeds.

> +		if (argc < 2)
> +			return 0;
> +
> +		hashcpy(head, remote_head->sha1);
> +		read_empty(remote_head->sha1, 0);
> +		head_arg = argv[0];
> +		argc--;
> +		argv++;
> +	}
> +
> +	struct strbuf merge_names = STRBUF_INIT;

Decl-after-statement.

> +	/* We are invoked directly as the first-class UI. */
> +	if(!head_invalid)

SP after syntactic keyword and the open paren associated with it.

>  		head_arg = "HEAD";
>  
> +	/*
> +	 * All the rest are the commits being merged;
> +	 * prepare the standard merge summary message to
> +	 * be appended to the given message.  If remote
> +	 * is invalid we will die later in the common
> +	 * codepath so we discard the error in this
> +	 * loop.
> +	 */
> +	for (i = 0; i < argc; i++)
> +		merge_name(argv[i], &merge_names);
> +
> +	if (!have_message || shortlog_len) {
> +		fmt_merge_msg(&merge_names, &merge_msg, !have_message,
> +				  shortlog_len);
> +		if (merge_msg.len)
> +			strbuf_setlen(&merge_msg, merge_msg.len - 1);
>  	}
>  
> +	if (!argc)
>  		usage_with_options(builtin_merge_usage,
>  			builtin_merge_options);

Re: [PATCH] Allow multiple merges to invalid HEAD

[Jonathan Nieder]

(+cc: Martin, who also requested something like this long ago[1] iirc)
Hi,

The following is only about motivation; I'm ignoring the mechanics for
now.

Junio C Hamano wrote:

> What's the point of being able to do this in the first place?
>
> The _only_ reason "git pull $there $branch" into an unborn branch exists
> is because it feels like
>
> 	$ git clone $there $directory
>
> could be broken down as this command sequence:
>
> 	$ mkdir $directory
>         $ cd $directory
>         $ git init
>         $ git remote add origin $there
> 	$ git fetch
>         $ git pull $there ;# or "git merge origin/master"

I'm to blame for the suggestion, I'm afraid.  

I haven't wanted to be able to "git init && git remote add origin
$there && git fetch && git merge origin/topic-1 origin/topic-2" for a
long time, but there was a time (around 3 years ago, I think) I wanted
it a lot.

In fact I can't remember the details of why.  The facts I remember:

 - /etc was kept in a git repository managed by etckeeper;
 - history was not precious --- it was just a way of understanding the
   final result;
 - the user wasn't sure how to manage topic branches yet.  The usual
   workflow involved

    (1) cherry-picking each automatic commit to an appropriate topic
        branch and writing a sensible description
    (2) bundling the topic branches together with an octopus merge
    (3) diff-ing against the automatically committed latest state to
        see if anything was missing
    (4) force a push.

So I suspect my use case was "start with a bundle of these branches".
I could have (and did) used "git checkout -b" to grab the first one
and merged the rest, but somehow a merge into nothingness seemed more
intuitive.  Remember, this is before the days of checkout --orphan
and rebase -i --everything.

That said.

I doubt the above is a very compelling use case, to justify the
maintenance cost and easily explain in documentation.  I would find
something like the following more compelling:

	Currently git merge has a special case to "pull into
	emptiness", to support the following sequence of commands:

		git init repo
		cd repo
		git remote add origin $url
		git fetch origin
		git merge origin/master

	But it is very much a special case.  It does not support
	octopus merges, --squash, --stat, alternate strategies,
	or any of the rest of merge machinery.

	It would be more intuitive if the above features _did_ work
	where they make sense, and if this essentially worked as
	though performing an ordinary merge with merge base being an
	empty tree that is ancestor of everything.

	Here is a first step --- support for octopus merges.  It
	also makes it easier to support the others, by [...].

> It very much feels like
> "because I can make things more complex", not "because this is an often
> desired missing feature that is a major pain point".

Therefore a good goal would be to make this make the code more simple
at the same time as making the semantics so.

(But also: Tim, please don't feel attached to this topic if you're not
interested in it; there are many more out there.)

Thanks for some useful feedback.
Jonathan

[1] http://bugs.debian.org/432558

Re: [PATCH] Allow multiple merges to invalid HEAD

[Jonathan Nieder]

Hi,

Timothy Chen wrote:

>  builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------
>  1 files changed, 31 insertions(+), 26 deletions(-)

Now for mechanics.

> --- a/builtin/merge.c
> +++ b/builtin/merge.c
[...]
> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
>  		if (!remote_head)
>  			die(_("%s - not something we can merge"), argv[0]);
> -		read_empty(remote_head->sha1, 0);
>  		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
>  				DIE_ON_ERR);
> -		return 0;
> +
> +		if (argc < 2)
> +			return 0;

When argc == 1, this means read_empty never gets called.  Is that
intended?

It breaks 7607.13.  Running "make test" is a good way to find some
breakages.

> +
> +		hashcpy(head, remote_head->sha1);
> +		read_empty(remote_head->sha1, 0);
> +		head_arg = argv[0];
> +		argc--;
> +		argv++;

As always when pretending something, I think a comment would be
helpful.  Something to the effect of:

	/*
	 * We were called as "git merge <branch1> <branch2> <branch3>...".
	 *
	 * Now HEAD has advanced to <branch1>, and we can pretend
	 * we were called as "git merge <branch2> <branch3>...".
	 */

Though I think I prefer the more explicit comment I suggested last
time[1].

> +	}
> +
> +	struct strbuf merge_names = STRBUF_INIT;
> +
> -	} else {
> -		struct strbuf merge_names = STRBUF_INIT;
> -
> -		/* We are invoked directly as the first-class UI. */
> +	/* We are invoked directly as the first-class UI. */

Won't this break

	git merge "message here" $(git rev-parse HEAD) foo bar

?  Previously this code was in an "else" block so it wasn't reached in
the is_old_style_invocation case.

> -	if (head_invalid || !argc)
> +	if (!argc)
>  		usage_with_options(builtin_merge_usage,
>  			builtin_merge_options);

What happens with

	git merge "message here" HEAD foo bar

from an unborn branch?

Hope that helps.
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/170456

Re: [PATCH] Allow multiple merges to invalid HEAD

[Timothy Chen]

Thanks for all the review.

Yes I agree it does brings more complexity with a small gain in feature.

So please disregard this patch (also please forgive my missing signed-off as it's my first patch), as I'm gonna move on to work on some fixes on submodules as I originally intended.

Thanks!

Tim

On Apr 3, 2011, at 3:22 PM, Jonathan Nieder wrote:

> Hi,
> 
> Timothy Chen wrote:
> 
>> builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------
>> 1 files changed, 31 insertions(+), 26 deletions(-)
> 
> Now for mechanics.
> 
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
> [...]
>> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>> 		remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
>> 		if (!remote_head)
>> 			die(_("%s - not something we can merge"), argv[0]);
>> -		read_empty(remote_head->sha1, 0);
>> 		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
>> 				DIE_ON_ERR);
>> -		return 0;
>> +
>> +		if (argc < 2)
>> +			return 0;
> 
> When argc == 1, this means read_empty never gets called.  Is that
> intended?
> 
> It breaks 7607.13.  Running "make test" is a good way to find some
> breakages.
> 
>> +
>> +		hashcpy(head, remote_head->sha1);
>> +		read_empty(remote_head->sha1, 0);
>> +		head_arg = argv[0];
>> +		argc--;
>> +		argv++;
> 
> As always when pretending something, I think a comment would be
> helpful.  Something to the effect of:
> 
> 	/*
> 	 * We were called as "git merge <branch1> <branch2> <branch3>...".
> 	 *
> 	 * Now HEAD has advanced to <branch1>, and we can pretend
> 	 * we were called as "git merge <branch2> <branch3>...".
> 	 */
> 
> Though I think I prefer the more explicit comment I suggested last
> time[1].
> 
>> +	}
>> +
>> +	struct strbuf merge_names = STRBUF_INIT;
>> +
>> -	} else {
>> -		struct strbuf merge_names = STRBUF_INIT;
>> -
>> -		/* We are invoked directly as the first-class UI. */
>> +	/* We are invoked directly as the first-class UI. */
> 
> Won't this break
> 
> 	git merge "message here" $(git rev-parse HEAD) foo bar
> 
> ?  Previously this code was in an "else" block so it wasn't reached in
> the is_old_style_invocation case.
> 
>> -	if (head_invalid || !argc)
>> +	if (!argc)
>> 		usage_with_options(builtin_merge_usage,
>> 			builtin_merge_options);
> 
> What happens with
> 
> 	git merge "message here" HEAD foo bar
> 
> from an unborn branch?
> 
> Hope that helps.
> Jonathan
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/170456