Re: Silly 'git am' UI issue
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 4, 2022, 07:14 UTC
- Message-ID
- <220304.86r17ifahr.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqq1qzi4bpk.fsf@gitster.g>
On Thu, Mar 03 2022, Junio C Hamano wrote:
Show 32 quoted lines
> 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, \