git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Orgad Shaneh <orgads@gmail.com>
Cc: Orgad Shaneh via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>
Subject: Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks
Date: Fri, 20 Nov 2020 01:38:58 -0500
Message-ID: <CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com> (raw)
In-Reply-To: <CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.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.)

  reply	other threads:[~2020-11-20  6:41 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-17 15:02 [PATCH] hooks: allow input from stdin Orgad Shaneh via GitGitGadget
2020-11-17 19:59 ` Junio C Hamano
2020-11-19 15:50 ` [PATCH v2] " Orgad Shaneh via GitGitGadget
2020-11-19 15:56   ` [PATCH v3] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget
2020-11-19 19:16     ` Junio C Hamano
2020-11-19 20:41       ` Orgad Shaneh
2020-11-19 20:56     ` [PATCH v4 0/2] " Orgad Shaneh via GitGitGadget
2020-11-19 20:56       ` [PATCH v4 1/2] " Orgad Shaneh via GitGitGadget
2020-11-19 21:23         ` Eric Sunshine
2020-11-19 21:32           ` Junio C Hamano
2020-11-20  5:23           ` Orgad Shaneh
2020-11-20  6:38             ` Eric Sunshine [this message]
2020-11-20  6:48               ` Eric Sunshine
2020-11-20  7:16                 ` Orgad Shaneh
2020-11-20 18:13               ` Junio C Hamano
2020-11-20 10:59             ` Ævar Arnfjörð Bjarmason
2020-11-20 12:34               ` Orgad Shaneh
2020-11-19 20:56       ` [PATCH v4 2/2] commit: fix stdin conflict between message and hook Orgad Shaneh via GitGitGadget
2020-12-09 20:06       ` [PATCH v5 0/2] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget
2020-12-09 20:06         ` [PATCH v5 1/2] hooks: lay foundations for passing stdin to hooks Orgad Shaneh via GitGitGadget
2020-12-09 20:06         ` [PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget
2020-12-09 22:37           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=orgads@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git