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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

Thanks.