Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 20, 2020, 10:59 UTC
Message-ID: <87sg94pa45.fsf@evledraar.gmail.com>
In-Reply-To: <CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com>

On Fri, Nov 20 2020, Orgad Shaneh wrote:

Show 58 quoted lines

> On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>> > Let hooks receive user input if applicable.
>> > [...]
>> > This allows for example prompting the user to choose an issue
>> > in prepare-commit-msg, and add "Fixes #123" to the commit message.
>> >
>> > Another possible use-case is running sanity test on pre-commit,
>> > and having a prompt like "This and that issue were found in your
>> > changes. Are you sure you want to commit? [Y/N]".
>>
>> These use-cases really help readers understand the motivation for this
>> change. Good.
>>
>> > Allow stdin only for commit-related hooks. Some of the other
>> > hooks pass their own input to the hook, so don't change them.
>> >
>> > Note: If pre-commit reads from stdin, and git commit is executed
>> > with -F - (read message from stdin), the message is not read
>> > correctly. This is fixed in the follow-up commit.
>>
>> Rather than making such a fundamental change and having to deal with
>> the fallout by introducing complexity to handle various special-cases
>> which pop up now and in the future, I wonder if it makes more sense to
>> instead just update documentation to tell hook authors to read
>> explicitly from the console rather than expecting stdin to be
>> available (since stdin may already be consumed for other purposes when
>> dealing with hooks or commands which invoke the hooks).
>
> On the first revision I had several links in the commit message to
> users who solved it this way. This solution however is not optimal.
> I have a prepare-commit-msg hook that requires user interaction for
> choosing an issue. This hook must work from the terminal and also
> from GUI applications like IDE.
>
> Currently the hook always pops a GUI window, but when using it
> from the terminal this is inconvenient (and when running over
> remote SSH without X forwarding it can't work), so I'd like it to be
> usable also from the terminal.
>
> To achieve that, I created 2 classes - one for terminal and one
> for GUI, and trying to choose the correct class by checking if
> stdin is a tty. The condition looks like this (Ruby):
> client = STDIN.tty? ? Terminal.new : GUI.new
>
> At this point I was surprised to discover that Git closes stdin,
> so the condition is never satisfied, and I always end up with GUI.
>
> As I mentioned, I need it to work also when executed from
> GUI applications, so just reading from the console will not work
> in my case. I tried other ways to detect "running from terminal"
> without the tty condition, but couldn't. The environment variables
> are identical when running in a GUI terminal and in the IDE.
>
> Can you suggest an alternative way to determine if I can accept user
> input from the console or not?

Like Eric noted in his reply I can't think of a way to do that particular thing reliably either, and agree with his comments that if such a way is found / some aspect of this change is kept having this explanation in the patch/commit message is really helpful.

I think what you're trying to do here isn't a good fit for most git workflows. Instead of trying to interactively compose a commit message why not change the commit template to start with e.g.:

    # You must replace XXX with an issue number here!:
    Issue #XXX:

That gives the user the same thing to fill out, but in their editor instead of via some terminal/GUI prompt. They need to write the rest of the commit message anyway in the editor, so even if you could why open up two UIs?

Projects that have these conventions also typically settle on just not trying to solve this problem on the client-side, but e.g. having a pre-receive hook that does the validation, or do it via CI / before a merge to master happens etc.