Duy Nguyen <pclouds@xxxxxxxxx> writes:
> So an alternative is simply outsource the ambiguity decision back to
> git-clone. If the same situation appears again elsewhere, we'll need
> to sit back and fix it for real. But this way we don't potentially
> introduce any new traps.
Sounds like a sensibly safe approach.
> +/*
> + * Avoid ambiguation error between --recursive and --recurse-submodule
> + * because they are the same. --recurs can be expanded to any of them
> + * and it still works.
> + */
> +static int is_abbrev_ambiguous(const struct option *prev,
> + const struct option *next)
> +{
> + const struct option *opts[] = { prev, next };
By looking at its caller, I think the caller keeps track of the
candidate it saw so far, and ask this function to see if the one it
is looking at right now (i.e. "this one") should be flagged as
conflicting with the other one (or "the other one"). So it probably
makes more sense to call them <it, the_other> or <it, prev> [*1*].
Side note: *1* I would have used "this" instead of "it", but
I vaguely recall there are those who want to use C++ aware
static checkers and avoiding the identifier "this" is easy
enough so ...
> + int i, found = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(opts); i++) {
> + if (!opts[i]->long_name)
> + continue;
> + if (!strcmp(opts[i]->long_name, "recursive"))
> + found |= 1 << 0;
> + if (!strcmp(opts[i]->long_name, "recurse-submodules"))
> + found |= 1 << 1;
> + }
> +
> + return found != 3;
> +}
For any two options that share the prefix, unless they are
"recursive" and "recurse-submodules" pair, we say "that's
ambiguous". But when they are these two specifically singled out,
we say it is OK. Makes sense.
The above may be sufficient for this purpose, but I would have
expected that these groups of aliases would be the ones in the
table, not the <it, the_other> pair. IOW, with helpers like these:
static const char *recurse_submodules[] = {
"recurse-submodules", "recursive", NULL,
};
static struct {
const char **aliases;
} disamb[] = {
recurse_submodules,
};
static int has_string(const char *it, const char **array)
{
while (*array)
if (!strcmp(it, *(array++)))
return 1;
return 0;
}
the body would look something like:
int j;
for (j = 0; j < ARRAY_SIZE(disamb); j++) {
/* it and other are from the same family? */
if (it->long_name &&
has_string(it->long_name, disamb[j].aliases) &&
other->long_name &&
has_string(other->long_name, disamb[j].aliases))
return 0;
}
return 1;
Perhaps?
I suspect that renaming the function (and the field in the context
struct) to .is_alias() and reverse the polarity of its return value
may make the flow of the logic at the caller side easier to follow?
> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..c0354e5a92 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
> if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
> !strncmp(long_name, arg, arg_end - arg)) {
> is_abbreviated:
> - if (abbrev_option) {
> + if (abbrev_option &&
> + p->is_abbrev_ambiguous(abbrev_option, options)) {
The above suggestion would make the guard here to
if (abbrev_option &&
!p->is_alias(abbrev_option, options)) {
That is, at this point, we know that the user may have used the
given string to name the "abbrev_option" we earlier saw (and made
sure it prefix matches the string), and now we are looking at
another one, options[0], that also prefix matches the string. We
usually flag this case as a problematic ambiguity inside the body of
this if statement, but if one is an alias to the other, we do not do
so.
> /*
> * If this is abbreviated, it is
> * ambiguous. So when there is no
