git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Orgad Shaneh <orgads@gmail.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	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 11:59:22 +0100	[thread overview]
Message-ID: <87sg94pa45.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com>


On Fri, Nov 20 2020, Orgad Shaneh wrote:

> 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.

  parent reply	other threads:[~2020-11-20 11:01 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
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 [this message]
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=87sg94pa45.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=orgads@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks' \
    /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://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.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 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