Silly 'git am' UI issue

Silly 'git am' UI issue

[Linus Torvalds]

Ok, so I'm "intellectually challenged" sometimes.

I tend to apply patches with

    $ git am -s --whitespace=fix <filename>

and just a moment ago I had a senior moment, and instead did

    $ git am -s --whitespace <filename>

and then wondered what the heck was wrong with my machine for being so slow.

I ^C'd it, and tried again, because I really am not a smart man. On
the *third* try, it finally dawned on me what a maroon I was.

The problem is obvious when I list those "right way" and "wrong way"
next to each other, but even then the *behavior* is most certainly not
obvious.

And this is really just a problem with "git am" (and possibly "git
rebase"), because they know that the "whitespace" option needs a
value, but they don't actually _check_ that value. They just know it's
a "passthru" argument to "git apply" (and "git am" in the case of
rebase).

So what happens is that "git am" decides that <filename> is the
argument to the "--whitespace" option, then doesn't see an argument at
all, and expects the input from stdin. So it "hangs", waiting for
input. No error messages, no nothing.

If you actually do the same thing to "git apply", you don't see the
problem, and it clarifies things:

    $ git apply --whitespace <filename>
    error: unrecognized whitespace option '<filename>'

and it's all very obvious what is going on.

But "git am" never even gets to that stage, because it is busy waiting
for input that will never come from the terminally confused person
sitting in front of the keyboard.

I don't think this is a bug, exactly, but it did make me go "Whaa?".
But that may be because my other neuron hasn't had enough coffee yet.

              Linus

Re: Silly 'git am' UI issue

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So what happens is that "git am" decides that <filename> is the
> argument to the "--whitespace" option, then doesn't see an argument at
> all, and expects the input from stdin. So it "hangs", waiting for
> input. No error messages, no nothing.

Ouch.  I suspect there are other (e.g. "pull -> fetch") passthru
that share similar issues, but "am" may be quite special in that
it is prepared to read its input from the standard input stream.

I wonder if something like this would have helped.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: am/apply: warn if we end up reading patches from terminal

In an interactive session, "git am" without arguments, or even
worse, "git am --whitespace file", waits silently for the user to
feed the patches from the standard input (presumably by typing or
copy-pasting).  Give a feedback message to the user when this
happens, as it is unlikely that the user meant to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/mailsplit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git c/builtin/mailsplit.c w/builtin/mailsplit.c
index 7baef30569..c45f542341 100644
--- c/builtin/mailsplit.c
+++ w/builtin/mailsplit.c
@@ -223,6 +223,9 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
 	FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
 	int file_done = 0;
 
+	if (isatty(fileno(f)))
+		warning(_("reading patches from stdin/tty..."));
+
 	if (!f) {
 		error_errno("cannot open mbox %s", file);
 		goto out;

Re: Silly 'git am' UI issue

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Thu, Mar 3, 2022 at 1:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I wonder if something like this would have helped.

That would certainly have helped, yes.

But I think we can do better.

How about just parsing the "--whitespace" option in 'git am' before
passing it through?

Something like the attached patch seems to work for me.

With this one, I simply get

    $ git am --whitespace 0001-Dummy.patch
    error: unrecognized whitespace option '0001-Dummy.patch'

when I make the mistake of not giving that whitespace argument.

I'm not claiming this is extensively tested, but I did *some* testing
on it, and it's not a complicated patch.

             Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2149 bytes --]

 apply.c      |  2 +-
 apply.h      |  2 ++
 builtin/am.c | 14 ++++++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index 0912307bd9..28e6fe0cfa 100644
--- a/apply.c
+++ b/apply.c
@@ -36,7 +36,7 @@ static void git_apply_config(void)
 	git_config(git_xmerge_config, NULL);
 }
 
-static int parse_whitespace_option(struct apply_state *state, const char *option)
+int parse_whitespace_option(struct apply_state *state, const char *option)
 {
 	if (!option) {
 		state->ws_error_action = warn_on_ws_error;
diff --git a/apply.h b/apply.h
index 4052da50c0..c931786a2a 100644
--- a/apply.h
+++ b/apply.h
@@ -173,6 +173,8 @@ int parse_git_diff_header(struct strbuf *root,
 			  unsigned int size,
 			  struct patch *patch);
 
+int parse_whitespace_option(struct apply_state *state, const char *option);
+
 /*
  * Some aspects of the apply behavior are controlled by the following
  * bits in the "options" parameter passed to apply_all_patches().
diff --git a/builtin/am.c b/builtin/am.c
index 0f4111bafa..542c6c5cab 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2298,6 +2298,16 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 	return 0;
 }
 
+static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)
+{
+	struct apply_state dummy = { };
+
+	if (parse_whitespace_option(&dummy, arg))
+		return -1;
+
+	return parse_opt_passthru_argv(opt, arg, unset);
+}
+
 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),

Re: Silly 'git am' UI issue

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Mar 3, 2022 at 1:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I wonder if something like this would have helped.
>
> That would certainly have helped, yes.
>
> But I think we can do better.
>
> How about just parsing the "--whitespace" option in 'git am' before
> passing it through?
>
> Something like the attached patch seems to work for me.
>
> With this one, I simply get
>
>     $ git am --whitespace 0001-Dummy.patch
>     error: unrecognized whitespace option '0001-Dummy.patch'
>
> when I make the mistake of not giving that whitespace argument.
>
> I'm not claiming this is extensively tested, but I did *some* testing
> on it, and it's not a complicated patch.
>
>              Linus

Yup, reusing what apply.c already uses gives reviewers a solid
assurance.  It already is named appropriately for a global
namespace, so the patch text, as far as I can see, looks good
enough.

> +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),

Re: Silly 'git am' UI issue

[Ævar Arnfjörð Bjarmason]

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

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

I don't think we use them at all. It's GNU GCC-specific syntax that
translates to the equivalent of { 0 }, which we can use instead.

Re: Silly 'git am' UI issue

[Ævar Arnfjörð Bjarmason]

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, \

Re: Silly 'git am' UI issue

[Linus Torvalds]

On Thu, Mar 3, 2022 at 11:22 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> 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.

I wasn't familiar enough with the option parsing code to do this, but
yes, I think your approach is nicer and makes it easier to add
checking to the other passthrough cases

So Ack from me on that approach instead.

I do suspect that you'll notice when trying to code up a proper patch
that it's probably complicated by the fact that the "validation"
function wants a different argument (in this case a 'struct
apply_state') than the "passthrough" function does (it wants that
"struct strvec").

Again, I'm not familiar with the argument parsing code, it all
post-dates my work, so what do I know..

                Linus