Re: [PATCH 1/2] parse-options: make CMDMODE errors more precise

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

> Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK
> as well to allow taking over the responsibility for compatibility
> checking from the callback function.  The next patch will use this
> capability to fix the messages for git am --show-current-patch.
>
> Use a linked list for storing the PARSE_OPT_CMDMODE variables.  This
> somewhat outdated data structure is simple and suffices, as the number
> of elements per command is currently only zero or one.  We do support
> multiple different command modes variables per command, but I don't
> expect that we'd ever use a significant number of them.  Once we do we
> can switch to a hashmap.

Makes quite a lot of sense.  I would have expected to see this in
the parse_options_check() function, where other sanity checks are
done, but I think the reason you added the call to its caller
because parse_options_check() does not take the context.

It is not like we want to perform a sanity check that is independent
from a particular invocation of parse_options() on the options[]
array only just once, and want to reuse the array that is known to
be sane multiple times.  The check is called once for every
invocation, so with or without this change, I do not see a good
reason for parse_options_check() not to take context, though.

> +static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> +				       const struct option *opt,
> +				       enum opt_parsed flags)
> +{
> +	const char *arg = NULL;
> +	enum parse_opt_result result = do_get_value(p, opt, flags, &arg);
> +	struct parse_opt_cmdmode_list *elem = p->cmdmode_list;
> +	char *opt_name, *other_opt_name;
> +
> +	for (; elem; elem = elem->next) {
> +		if (*elem->value_ptr == elem->value)
> +			continue;
> +
> +		if (elem->opt &&
> +		    (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)
> +			break;
> +
> +		elem->opt = opt;
> +		elem->arg = arg;
> +		elem->flags = flags;
> +		elem->value = *elem->value_ptr;
> +	}
> +
> +	if (result || !elem)
> +		return result;
> +
> +	opt_name = optnamearg(opt, arg, flags);
> +	other_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);
> +	error(_("%s is incompatible with %s"), opt_name, other_opt_name);
> +	free(opt_name);
> +	free(other_opt_name);
> +	return -1;
> +}

Looks quite involved but the overhead is number of supported cmdmode
options per each command line option, and the problems outlined in
the proposed log message are worth addressing.  OK.

> @@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,
>  	precompose_argv_prefix(argc, argv, NULL);
>  	free_preprocessed_options(real_options);
>  	free(ctx.alias_groups);
> +	for (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {
> +		struct parse_opt_cmdmode_list *next = elem->next;
> +		free(elem);
> +		elem = next;
> +	}
>  	return parse_options_end(&ctx);
>  }

OK.