git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Orgad Shaneh <orgads@gmail.com>
Subject: Re: [PATCH] hooks: allow input from stdin
Date: Tue, 17 Nov 2020 11:59:30 -0800	[thread overview]
Message-ID: <xmqqh7pn69gd.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.790.git.1605625363309.gitgitgadget@gmail.com> (Orgad Shaneh via GitGitGadget's message of "Tue, 17 Nov 2020 15:02:43 +0000")

"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Orgad Shaneh <orgads@gmail.com>
>
> Let hooks receive user input if applicable.
>
> Closing stdin originates in f5bbc3225 (Port git commit to C,
> 2007). Looks like the original shell implementation did have
> stdin open. Not clear why the author chose to close it on
> the C port (maybe copy&paste).

For "git commit" leaving the standard input open and let hook
interact with the end-user sitting at the terminal might be OK, but
doing this for any and all hooks probably breaks callers of hooks
that deal with transport protocols where their standard input is
*not* supposed to be molested (think: the main Git process is about
to read a packstream over the network and spawns a hook using the
standard run_hook*() interface---if the hook reads standard input,
the main process would lose the initial part of the pack stream).

> The only hook that passes internal information to the hook
> via stdin is pre-push, which has its own logic.

Hmph, doesn't "pre-receive" hook gets information from its standard
input?  In any case, it is natural that such hooks can and have to
be able to read from their standard input---they are spawned with
their input connected to Git process that feeds them input that are
necessary for them to make decision, so comparing them with random
other hooks that do not use information from Git to do their thing
is comparing apples and oranges.

So I think the patch we see as-is is probably not a good idea,
primarily because it lets run_hook_ve() to pretend that it still is
a generic interface to run any hooks by sitting in run_command.c,
but now its behaviour has been tweaked to fit only needs by "git
commit" and the like.  You probably need to add a new "options"
parameter to run_hook_ve() that allows the callers to say "this hook
is allowed to read from the standard input" etc., if we want to keep
it in run_command.c and let it pretend to be generally useful
interface.

Another possibility is to remove run_hook_ve(), and open code its
body in commit.c::run_commit_hook(), but that is less than ideal.

> Some references of users requesting this feature. Some of
> them use acrobatics to gain access to stdin:
> [1] https://stackoverflow.com/q/1067874/764870
> [2] https://stackoverflow.com/q/47477766/764870
> [3] https://stackoverflow.com/q/3417896/764870
> [4] https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165
> [5] https://github.com/typicode/husky/issues/442

Instead of wasting 7 lines here, it would be a much better approach
to just add a test or two that demonstrate sample usages and that
protect this new feature from accidentally getting broken at the
same time.

>     The only hook that passes internal information to the hook via stdin is
>     pre-push, which has its own logic.

>     
>     Some references of users requesting this feature. Some of them use
>     acrobatics to gain access to stdin: [1] 
>     https://stackoverflow.com/q/1067874/764870[2] 
>     https://stackoverflow.com/q/47477766/764870[3] 
>     https://stackoverflow.com/q/3417896/764870[4] 
>     https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] 
>     https://github.com/typicode/husky/issues/442
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/790
>
>  run-command.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 2ee59acdc8..a17b613216 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1356,7 +1356,6 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
>  	while ((p = va_arg(args, const char *)))
>  		strvec_push(&hook.args, p);
>  	hook.env = env;
> -	hook.no_stdin = 1;
>  	hook.stdout_to_stderr = 1;
>  	hook.trace2_hook_name = name;
>  
>
> base-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500

  reply	other threads:[~2020-11-17 20:00 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 [this message]
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
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=xmqqh7pn69gd.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).