Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

Duy Nguyen <pclouds@gmail.com> writes:

Sounds like a sensibly safe approach.

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

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?

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.