Re: [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2017, 03:46 UTC
Message-ID: <xmqq60h8cay3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170503210726.24121-2-jn.avila@free.fr>

Jean-Noel Avila <jn.avila@free.fr> writes:

Show 9 quoted lines

> "git read-tree -m" requires a tree argument to name the tree to be
> merged in.  Git uses a cutesy error message to say so and why:
>
>     $ git read-tree -m
>     warning: read-tree: emptying the index with no arguments is
>     deprecated; use --empty
>     fatal: just how do you expect me to merge 0 trees?
>     $ git read-tree -m --empty
>     fatal: just how do you expect me to merge 0 trees?

This shows another issue. The "emptying ... is deprecated" message shouldn't be given when -m is present.

I am not saying that that needs to be fixed by you and/or as a part of this patch. Just something I noticed while reviewing the patch.

Show 10 quoted lines

>  Merging
>  -------
> -If `-m` is specified, 'git read-tree' can perform 3 kinds of
> -merge, a single tree merge if only 1 tree is given, a
> -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are
> -provided.
> +If `-m` is specified, at least one tree must be given on the command
> +line. 'git read-tree' can perform 3 kinds of merge, a single tree
> +merge if only 1 tree is given, a fast-forward merge with 2 trees, or a
> +3-way merge if 3 trees are provided.

It may not incorrect per-se, but the existing enumeration already say 1, 2 and 3 are the valid choices, so "at least one" may be redundant.

One incorrectness that needs to be changed is "if 3 trees are provided"; it is "if 3 or more trees". Again, not the topic of your change, but this one you may want to address while you are at it.

Show 5 quoted lines

>  	if (opts.merge) {
> -		if (stage < 2)
> -			die("just how do you expect me to merge %d trees?", stage-1);
>  		switch (stage - 1) {
> +		case 0:

Could "stage" be 0 (or negative) when we come here? If so, this rewrite may no longer diagnose the error correctly in such a case.

	... goes and looks ...

I think it begins with either 0 or 1 and then only counts up, so we should be safe. Rolling it in the switch() like this patch does makes it easier to follow what is going on, I think.

Show 5 quoted lines

> +			die("you must specify at least one tree to merge");
> +			break;
>  		case 1:
>  			opts.fn = opts.prefix ? bind_merge : oneway_merge;
>  			break;

Thanks.