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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Nov 20, 2020 at 12:23 AM Orgad Shaneh <orgads@gmail.com> wrote:
> 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.)