Re: [BUG?] applypatch-msg hook no-longer thinks stdin is a tty

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Chris Packham <judge.packham@gmail.com> writes:
>>
>>> As of git 2.6 this has stopped working and stdin always fails the tty
>>> check.
>>
>> We now run that hook thru run_hook_ve(), which closes the standard
>> input (as the hook is not reading anything).  Perhaps you can check
>> if your output is connected to the tty instead?

>
> s|closes the standard input|opens the standard input for /dev/null|;
>
> Having said that, here are some further thoughts:
>
>  * Hooks run via run_hook_ve() and run_hook_le() have their standard
>    input connected to /dev/null even before these functions were
>    introduced at ae98a008 (Move run_hook() from builtin-commit.c
>    into run-command.c (libgit), 2009-01-16).  The commit
>    consolidated the code to run hooks in "checkout", "commit", "gc",
>    and "merge", all of which run their hooks with their standard
>    input reading from /dev/null.
>
>  * Later at dfa7a6c5 (clone: run post-checkout hook when checking
>    out, 2009-03-03) "git clone" learned to run post-checkout hook
>    the same way.
>
>  * "receive-pack" (which accepts and processes an incoming "git
>    push") has pre-receive and post-receive hooks, and they do get
>    invoked with their standard input open, but they are connected to
>    a pipe to be fed with the information about the push from
>    "receive-pack" process.
>
>  * "post-rewrite" hooks, invoked by "rebase" and "commit", does get
>    invoked with its standard input open, but it is fed with the
>    information about the original and the rewritten commit.
>
> So in that sense, "am", primarily because it was implemented as a
> script, was an oddball.  It should have been connecting the standard
> input to /dev/null in order to be consistent with others, but it did
> not even bother to do so.
>
> We _could_ leave the standard input connected to the original
> process's standard input only for the specific hook by doing
> something along the lines of the attached, but I am not sure if it
> is a good change.  Given that the majority of existing hooks are
> spawned with their standard input connected to /dev/null (and also
> after scanning the output from "git hooks --help", I did not find
> any that would want to read from the standard input of the original
> process that spawns it), I tend to consider that the change in 2.6
> done as part of rewriting "am" in C is a bugfix, even though an
> unintended one, to make things more consistent.
>
> Besides "consistency", a hook that tried to read from "am"'s
> standard input would have been incorrect in the first place, as it
> is a normal mode of operation to feed one or more patch e-mails from
> the standard input of "git am", i.e.
>
>         $ git am <mbox

>
>  builtin/am.c  |  8 +++++++-
>  run-command.c | 30 ++++++++++++++++++++++++++----
>  run-command.h |  9 +++++++++
>  3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 4f77e07..3d160d9 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -510,9 +510,15 @@ static void am_destroy(const struct am_state *state)
>  static int run_applypatch_msg_hook(struct am_state *state)
>  {
>         int ret;
> +       struct child_process custom = CHILD_PROCESS_INIT;
>
>         assert(state->msg);
> -       ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL);
> +
> +       custom.env = NULL;
> +       custom.no_stdin = 0;
> +       custom.stdout_to_stderr = 1;
> +
> +       ret = run_hook_le_opt(&custom, "applypatch-msg", am_path(state, "final-commit"), NULL);
>
>         if (!ret) {
>                 free(state->msg);
> diff --git a/run-command.c b/run-command.c
> index 3277cf7..dee86df 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -793,7 +793,7 @@ const char *find_hook(const char *name)
>         return path.buf;
>  }
>
> -int run_hook_ve(const char *const *env, const char *name, va_list args)
> +int run_hook_ve_opt(struct child_process *custom, const char *name, va_list args)
>  {
>         struct child_process hook = CHILD_PROCESS_INIT;
>         const char *p;
> @@ -805,13 +805,35 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
>         argv_array_push(&hook.args, p);
>         while ((p = va_arg(args, const char *)))
>                 argv_array_push(&hook.args, p);
> -       hook.env = env;
> -       hook.no_stdin = 1;
> -       hook.stdout_to_stderr = 1;
> +       hook.env = custom->env;
> +       hook.no_stdin = custom->no_stdin;
> +       hook.stdout_to_stderr = custom->stdout_to_stderr;
>
>         return run_command(&hook);
>  }
>
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
> +{
> +       struct child_process custom = CHILD_PROCESS_INIT;
> +
> +       custom.env = env;
> +       custom.no_stdin = 1;
> +       custom.stdout_to_stderr = 1;
> +       return run_hook_ve_opt(&custom, name, args);
> +}
> +
> +int run_hook_le_opt(struct child_process *custom, const char *name, ...)
> +{
> +       va_list args;
> +       int ret;
> +
> +       va_start(args, name);
> +       ret = run_hook_ve_opt(custom, name, args);
> +       va_end(args);
> +
> +       return ret;
> +}
> +
>  int run_hook_le(const char *const *env, const char *name, ...)
>  {
>         va_list args;
> diff --git a/run-command.h b/run-command.h
> index 5b4425a..33a0d72 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -62,6 +62,15 @@ LAST_ARG_MUST_BE_NULL
>  extern int run_hook_le(const char *const *env, const char *name, ...);
>  extern int run_hook_ve(const char *const *env, const char *name, va_list args);
>
> +/*
> + * Same as above, but env, no_stdin and stdout_to_stderr are copied from
> + * custom to the child_process structure that spawns the hook.
> + */
> +LAST_ARG_MUST_BE_NULL
> +extern int run_hook_le_opt(struct child_process *custom, const char *name, ...);
> +extern int run_hook_ve_opt(struct child_process *custom, const char *name, va_list args);
> +
> +
>  #define RUN_COMMAND_NO_STDIN 1
>  #define RUN_GIT_CMD         2  /*If this is to be git sub-command */
>  #define RUN_COMMAND_STDOUT_TO_STDERR 4
>
>
>