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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 11, 2017, 19:08 UTC
Message-ID: <20170511190809.GB12516@aiede.svl.corp.google.com>
In-Reply-To: <20170511120634.17683-2-jn.avila@free.fr>

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.

[...]

Show 11 quoted lines

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

[...]

Show 8 quoted lines

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

[...]

Show 10 quoted lines

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