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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jean-Noel Avila wrote:

> Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Please remove my sign-off.  I didn't write or carry this patch.

If you want to acknowledge my contribution, you can use something
like Helped-by, but it's not necessary.

[...]
> +++ b/Documentation/git-read-tree.txt
> @@ -135,10 +135,10 @@ OPTIONS
>  
>  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.

As I mentioned before, this sentence feels redundant and doesn't fix
the real problem of the `-m` reference elsewhere in this file not
pointing to this section.

[...]
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -132,7 +132,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  		OPT_BOOL(0, "empty", &read_empty,
>  			    N_("only empty the index")),
>  		OPT__VERBOSE(&opts.verbose_update, N_("be verbose")),
> -		OPT_GROUP(N_("Merging")),
> +		OPT_GROUP(N_("Merging (needs at least one tree-ish")),

This also seems a little too much of a special detail to put in the
prominent section title.  If you run "git read-tree -h", where would
you expect to find this information?

The "git read-tree -h" output turns out to not be useful for much more
than a reminder of supported options --- it doesn't give a useful
overview of the usage, since the usage string at the start is very
long.  That's unfortunate but it seems outside the scope of this
patch.  Probably the simplest thing is to drop this hunk from the
patch.


[...]
> @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  		setup_work_tree();
>  
>  	if (opts.merge) {
> -		if (stage < 2)
> -			die("just how do you expect me to merge %d trees?", stage-1);
>  		switch (stage - 1) {
> +		case 0:
> +			die(_("you must specify at least one tree to merge"));
> +			break;

This part looks good.

Thanks for your patient work.
Jonathan