Re: [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck
From: Junio C Hamano <hidden>
Date: 2017-05-11 03:46:41
Jean-Noel Avila [off-list ref] 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.
quoted hunk
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.
quoted hunk
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.
quoted hunk
+ die("you must specify at least one tree to merge"); + break; case 1: opts.fn = opts.prefix ? bind_merge : oneway_merge; break;
Thanks.