Re: Silly 'git am' UI issue

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 03 2022, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> [...]
>> +static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct apply_state dummy = { };
>
> It is rare to see a completely empty initializer in this codebase, I
> think.
>
>> +	if (parse_whitespace_option(&dummy, arg))
>> +		return -1;
>> +
>> +	return parse_opt_passthru_argv(opt, arg, unset);
>> +}
>
> Looks good.
>
>>  static int git_am_config(const char *k, const char *v, void *cb)
>>  {
>>  	int status;
>> @@ -2355,9 +2365,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>>  		OPT_CALLBACK_F(0, "quoted-cr", &state.quoted_cr, N_("action"),
>>  			       N_("pass it through git-mailinfo"),
>>  			       PARSE_OPT_NONEG, am_option_parse_quoted_cr),
>> -		OPT_PASSTHRU_ARGV(0, "whitespace", &state.git_apply_opts, N_("action"),
>> +		OPT_CALLBACK(0, "whitespace", &state.git_apply_opts, N_("action"),
>>  			N_("pass it through git-apply"),
>> -			0),
>> +			parse_opt_whitespace),
>>  		OPT_PASSTHRU_ARGV(0, "ignore-space-change", &state.git_apply_opts, NULL,
>>  			N_("pass it through git-apply"),
>>  			PARSE_OPT_NOARG),

Perfect shouldn't be the enemy of the good & all that, and this is an
improvement.

But having looked at this the general problem is that any
OPT_PASSTHRU_ARGV without a PARSE_OPT_NOARG has the same potential issue
in the case of "am", and I don't see how we can resolve the ambiguity
without calling e.g. parse_whitespace_option(), i.e. we need to call
whatever the validation function is for each one.

This change leaves the same problem in place for --exclude, --include,
and also -C, -p (but one is less likely to do -Cname).

A more general solution would be some continuation of this, i.e. we can
use the "defval" in "struct option" as a pointer to a validation
function for any arguments.

diff --git a/builtin/am.c b/builtin/am.c
index 0f4111bafa0..fa922fda200 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2355,9 +2355,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "quoted-cr", &state.quoted_cr, N_("action"),
 			       N_("pass it through git-mailinfo"),
 			       PARSE_OPT_NONEG, am_option_parse_quoted_cr),
-		OPT_PASSTHRU_ARGV(0, "whitespace", &state.git_apply_opts, N_("action"),
+		OPT_PASSTHRU_ARGV_CHECK(0, "whitespace", &state.git_apply_opts, N_("action"),
 			N_("pass it through git-apply"),
-			0),
+			0, parse_whitespace_option),
 		OPT_PASSTHRU_ARGV(0, "ignore-space-change", &state.git_apply_opts, NULL,
 			N_("pass it through git-apply"),
 			PARSE_OPT_NOARG),
diff --git a/parse-options.h b/parse-options.h
index 685fccac137..8348343bf59 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -356,6 +356,8 @@ int parse_opt_tracking_mode(const struct option *, const char *, int);
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru }
 #define OPT_PASSTHRU_ARGV(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv }
+#define OPT_PASSTHRU_ARGV_CHECK(s, l, v, a, h, f, c) \
+	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv, (c) }
 #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \
 	{ OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
 	  PARSE_OPT_LASTARG_DEFAULT | flag, \