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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 20, 2020, 06:38 UTC
Message-ID: <CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com>
In-Reply-To: <CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com>

On Fri, Nov 20, 2020 at 12:23 AM Orgad Shaneh <orgads@gmail.com> wrote:

Show 18 quoted lines

> On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > [...] 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 [...]
>
> 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.
> [...]
> 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?

Not at present, and I expect that the answer is that any such mechanism for determining this would be IDE-dependent. (That is, although your IDE doesn't distinguish itself in any way which your hook can detect, other IDE's might, but that doesn't help in the general case.)

What I can say, though, is that the additional information you supplied in your response should be part of the commit message to help reviewers and future readers better understand why this change is wanted. The use-cases presented in the v4 commit message, although helpful, didn't provide sufficient explanation considering that the first question which popped into this reviewer's mind was "why not have the hook read from the console explicitly?". (It is an unfortunate fact that reviewer time is a limited resource, so many reviewers on this list don't bother chasing down links like those you included in the commit message of v1 -- which would have helped justify the change -- but instead base their reviews only on the information presented in the commit message itself. In my case, I was only lightly skimming this series, thus didn't even bother chasing down those links -- but have done so now for this reply.)

I do find it quite concerning that the way this series handles the stdin conflict between the hook and `-F -` can break the hook silently and mysteriously. How confusing for a user to write a hook which works with `git commit -m msg` and `git commit -F file` but breaks silently with `git commit -F -`. What is worse is that this breakage may be outside the user's control. For instance, it is easy to imagine some IDE passing the commit message to git-commit via stdin (using `-F -`) rather than via a file (using `-F file`).

At the very least, this change deserves a documentation update, both to explain that the prepare-commit-msg hook has a valid stdin, and (importantly) that it won't be able to rely upon stdin in conjunction with `-F -`. (This also makes me wonder if it would be possible to signal to the hook whether or not stdin is available. Perhaps this could be done by passing an additional argument to the hook.)

Finally, I realize that you followed Junio's suggestion for organizing the series, however, it feels undesirable for patch [1/2] to leave the command in a somewhat broken state, by which I'm referring to the indeterminate outcome of the hook and `-F -` competing for stdin; a situation which is only resolved in [2/2]. To me, a cleaner organization would be for [1/2] to introduce the underlying mechanism and support by adding `flags` to run_hook_ve() (and perhaps to run_commit_hook()) but not to turn on RUN_HOOK_ALLOW_STDIN, and then have patch [2/2] actually enable RUN_HOOK_ALLOW_STDIN where appropriate _and_ deal with the `-F -` conflict all at the same time. (And the commit message should mention the conflict and how it is handled.)