Silly 'git am' UI issue

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

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

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

On Thu, Mar 3, 2022 at 1:58 PM Junio C Hamano <gitster@pobox.com> wrote:

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

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

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

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.

It is rare to see a completely empty initializer in this codebase, I think.

Looks good.

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

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.

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

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.

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

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