Re: [PATCH] Allow multiple merges to invalid HEAD

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 3, 2011, 22:22 UTC
Message-ID: <20110403222234.GB6537@elie>
In-Reply-To: <1301813216-19507-1-git-send-email-tnachen@gmail.com>

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

[...]

Show 11 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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