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

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

[Chris Packham]

Hi,

I have a applypatch-msg hook that implements some policy for
acceptable commit messages and reject non-conformant patches. It also
is able to prompt me to override it's rejection. The prompting only
happens when stdin is a tty (as determined by pythons
sys.stdin.isatty())

For example this would reject the patch and cause am stop
 $ cat bad.patch | git am

And this would prompt me to allow the patch and am would proceed if I answer Y
  $ git am bad.patch
  Patch message invalid apply (Y/N)?

As of git 2.6 this has stopped working and stdin always fails the tty
check. Here's a minimal reproduction

$ cat >.git/hooks/applypatch-msg <<EOF
#!/usr/bin/env python
import sys
if sys.stdin.isatty():
    print("TTY")
else:
    print("NOT")
EOF
$ chmod +x .git/hooks/applypatch-msg

$ git checkout -b temp origin/master^
$ git format-patch master -1

# With git 2.6
$ git am 0001-Git-2.6.patch
NOT
Applying: Git 2.6

# With git 2.5.3
TTY
Applying: Git 2.6

Thanks,
Chris

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

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

If you want to go interactive from the hook, you'd have to open and
interact with /dev/tty yourself in your hook anyway.

 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

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

[Chris Packham]

On Sat, Oct 3, 2015 at 6:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

Possibly but I still need to be able to push a prompt out to the user
and receive a response.

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

And my current hook handles that by assuming that the user input is "N"

>
> If you want to go interactive from the hook, you'd have to open and
> interact with /dev/tty yourself in your hook anyway.

That may be what I have to do, although I have absolutely no idea how.

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

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

[Johannes Sixt]

Am 03.10.2015 um 09:37 schrieb Chris Packham:
> On Sat, Oct 3, 2015 at 6:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> If you want to go interactive from the hook, you'd have to open and
>> interact with /dev/tty yourself in your hook anyway.
>
> That may be what I have to do, although I have absolutely no idea how.

Something like this:

exec <>/dev/tty 1>&0 2>&0
printf "what now: "
read answer
echo the answer was: "$answer"

-- Hannes