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