Jean-Noel Avila <jn.avila@xxxxxxx> 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.