[PATCH 1/2] Wait for child on signal death for aliases to builtins

[PATCH 1/2] Wait for child on signal death for aliases to builtins

[trygveaa]

From: Trygve Aaberge <trygveaa@gmail.com>

When you hit ^C all the processes in the tree receives it. When a git
command uses a pager, git ignores this and waits until the pager quits.
However, when using an alias there is an additional process in the tree
which didn't ignore the signal. That caused it to exit which in turn
caused the pager to exit. This fixes that for aliases to builtins.

This was originally fixed in 46df6906f3 (see that for a more in
explanation), but broke by a regression in b914084007.

Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>
---
 git.c         | 2 +-
 run-command.c | 1 +
 run-command.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index a2d337eed7..c8336773e3 100644
--- a/git.c
+++ b/git.c
@@ -767,7 +767,7 @@ static int run_argv(int *argcp, const char ***argv)
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
 			i = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT, "git_alias");
+						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/run-command.c b/run-command.c
index 9b3a57d1e3..a735e380a9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
 	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
+	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
 	cmd.dir = dir;
 	cmd.env = env;
 	cmd.trace2_child_class = tr2_class;
diff --git a/run-command.h b/run-command.h
index 191dfcdafe..ef3071a565 100644
--- a/run-command.h
+++ b/run-command.h
@@ -229,6 +229,7 @@ int run_auto_gc(int quiet);
 #define RUN_SILENT_EXEC_FAILURE 8
 #define RUN_USING_SHELL 16
 #define RUN_CLEAN_ON_EXIT 32
+#define RUN_WAIT_AFTER_CLEAN 64
 
 /**
  * Convenience functions that encapsulate a sequence of
-- 
2.27.0

[PATCH 2/2] Wait for child on signal death for aliases to externals

[trygveaa]

From: Trygve Aaberge <trygveaa@gmail.com>

The previous commit fixed this only for aliases to builtins, this commit
does the same for aliases to external commands. While the previous
commit fixed a regression, I don't think this has ever worked properly.

Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>
---

I can't say for sure if this will have any unintended side effects, so
if someone with more knowledge about it can check/verify it, that would
be great.

 git.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git.c b/git.c
index c8336773e3..0797ac6a80 100644
--- a/git.c
+++ b/git.c
@@ -346,6 +346,8 @@ static int handle_alias(int *argcp, const char ***argv)
 			commit_pager_choice();
 
 			child.use_shell = 1;
+			child.clean_on_exit = 1;
+			child.wait_after_clean = 1;
 			child.trace2_child_class = "shell_alias";
 			argv_array_push(&child.args, alias_string + 1);
 			argv_array_pushv(&child.args, (*argv) + 1);
-- 
2.27.0

Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

[Johannes Schindelin]

Hi,

On Sun, 5 Jul 2020, trygveaa@gmail.com wrote:

> From: Trygve Aaberge <trygveaa@gmail.com>
>
> When you hit ^C all the processes in the tree receives it. When a git
> command uses a pager, git ignores this and waits until the pager quits.
> However, when using an alias there is an additional process in the tree
> which didn't ignore the signal. That caused it to exit which in turn
> caused the pager to exit. This fixes that for aliases to builtins.
>
> This was originally fixed in 46df6906f3 (see that for a more in
> explanation), but broke by a regression in b914084007.

Thank you for those references. I did try to make sure that b914084007
would not regress on anything, but apparently I failed to verify the final
form. Since all it did was to remove `#if 0`...`#endif` guards, it was
unfortunately quite easy for this regression to slip in.

IIRC the original code inside those guards was modeled closely after
`execv_dashed_external()`, but it does not really look very much like it
anymore now, does it? (And it looks as if 2b296c93d49
(execv_dashed_external: use child_process struct, 2017-01-06) was
responsible for that.)

In any case, thank you for the patch, it looks good to me!

Ciao,
Dscho

>
> Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>
> ---
>  git.c         | 2 +-
>  run-command.c | 1 +
>  run-command.h | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index a2d337eed7..c8336773e3 100644
> --- a/git.c
> +++ b/git.c
> @@ -767,7 +767,7 @@ static int run_argv(int *argcp, const char ***argv)
>  			 * OK to return. Otherwise, we just pass along the status code.
>  			 */
>  			i = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |
> -						  RUN_CLEAN_ON_EXIT, "git_alias");
> +						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
>  			if (i >= 0 || errno != ENOENT)
>  				exit(i);
>  			die("could not execute builtin %s", **argv);
> diff --git a/run-command.c b/run-command.c
> index 9b3a57d1e3..a735e380a9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
>  	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
>  	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
>  	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
> +	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
>  	cmd.dir = dir;
>  	cmd.env = env;
>  	cmd.trace2_child_class = tr2_class;
> diff --git a/run-command.h b/run-command.h
> index 191dfcdafe..ef3071a565 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -229,6 +229,7 @@ int run_auto_gc(int quiet);
>  #define RUN_SILENT_EXEC_FAILURE 8
>  #define RUN_USING_SHELL 16
>  #define RUN_CLEAN_ON_EXIT 32
> +#define RUN_WAIT_AFTER_CLEAN 64
>
>  /**
>   * Convenience functions that encapsulate a sequence of
> --
> 2.27.0
>
>

Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

[Jeff King]

On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote:

> From: Trygve Aaberge <trygveaa@gmail.com>
> 
> When you hit ^C all the processes in the tree receives it. When a git
> command uses a pager, git ignores this and waits until the pager quits.
> However, when using an alias there is an additional process in the tree
> which didn't ignore the signal. That caused it to exit which in turn
> caused the pager to exit. This fixes that for aliases to builtins.
> 
> This was originally fixed in 46df6906f3 (see that for a more in
> explanation), but broke by a regression in b914084007.

Good catch. The regression is technically in b914084007, but the real
culprit is the extra (commented out) code path added in ee4512ed48
(trace2: create new combined trace facility, 2019-02-22).

Your fix here looks good, but it does make me wonder if we could avoid
or shrink this duplicate code path. I.e., could it just do the logging
necessary but still leave the actual process spawn to the
execv_dashed_external() below. It may be hard to untangle, though, so
certainly this makes sense in the meantime.

A test would be nice, but I don't think it's very feasible for the same
reason mentioned in 46df6906f3.

-Peff

Re: [PATCH 2/2] Wait for child on signal death for aliases to externals

[Jeff King]

On Sun, Jul 05, 2020 at 12:18:38AM +0200, trygveaa@gmail.com wrote:

> From: Trygve Aaberge <trygveaa@gmail.com>
> 
> The previous commit fixed this only for aliases to builtins, this commit
> does the same for aliases to external commands. While the previous
> commit fixed a regression, I don't think this has ever worked properly.
> 
> Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>

Usually when I find myself saying "the previous commit" and "does the
same" in a commit message, it's a good indication that it might be worth
combining the two commits into one. But in this case I think you're
right to keep them separate; the other one was a regression fix, and
this would be a change in behavior.

But it might be worth explaining more what the user-visible behavior
we're trying to solve is.

> I can't say for sure if this will have any unintended side effects, so
> if someone with more knowledge about it can check/verify it, that would
> be great.

Let's try to break down what differences the user might see.

The original is using the full run_command(), so we'd always be waiting
for the alias command to finish before exiting under normal
circumstances. The only difference is that now when die due to a signal,
we'd wait then, too.

And that only matters if:

  - we've spawned a pager that doesn't die due to the signal, and we
    want to wait until it has finished. However, that seems to work
    already for me in a quick test of:

      $ git -p -c alias.foo='!echo foo; sleep 10; echo bar' foo
      ^C

    where the pager keeps running (because it ignores the signal), and
    the main git process doesn't exit either (because it's waiting for
    the pager to finish)

    I think this case is different than the one fixed by 46df6906f3
    (execv_dashed_external: wait for child on signal death, 2017-01-06)
    because the pager is spawned by the main Git process (so it knows to
    wait), rather than the child dashed-external.

    I guess to recreate that you'd need to trigger the pager inside the
    alias itself, like:

      $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo
      ^C

    which does exhibit the annoying behavior (we exit, and pgrp loses
    the tty session leader bit, and the pager gets EIO).

  - the child for some reason decides not to respect the signal.
    Obviously running a pager is one reason it would decide to do so,
    and we'd be improving the behavior there. I have trouble imagining a
    case where waiting for it would do the _wrong_ thing. I.e., if I do:

      $ git -c alias.foo='!trap "echo got signal" INT; sleep 5' foo

    it probably does make sense for us to continue to wait on that alias
    to complete (I'm not sure what anyone would try to accomplish with
    that, but waiting seems like the least-astonishing thing to me).

So I think this is moving in the right direction.

However, there is one weirdness worth thinking about. Because the
wait_after_clean feature relies on actually trying to clean the child,
Git will actually send a signal to the alias shell. For a signal, we'll
try to pass along the same signal, so it just means the shell will get
SIGINT twice in this case (or more likely, they'll race and we'll ignore
the duplicate signal while the child is in its own handler).

We'd likewise send a signal if we hit a normal exit(), but we shouldn't
do so because we're already blocked waiting for the child to exit.

But a more interesting case is if somebody sends the parent git process
a signal but _not_ the child (e.g., kill -TERM). Then we'd pass SIGTERM
along to the child. I'd venture to say that's _probably_ the right thing
to do, if only because it's exactly what we do for a dashed external as
well.

Sorry that ended up so long-winded, but my conclusion is: this is doing
the right thing. We should probably embed some of that discussion in the
commit message, but I think the simplest argument is just: this is what
we do for external commands we run, so treating shell aliases the same
in terms of passing along signals makes things simple and consistent.

-Peff

Re: [PATCH 2/2] Wait for child on signal death for aliases to externals

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Sorry that ended up so long-winded, but my conclusion is: this is doing
> the right thing. We should probably embed some of that discussion in the
> commit message, but I think the simplest argument is just: this is what
> we do for external commands we run, so treating shell aliases the same
> in terms of passing along signals makes things simple and consistent.

Thanks for a great analysis to help the author.

Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Thank you for those references. I did try to make sure that b914084007
> would not regress on anything, but apparently I failed to verify the final
> form. Since all it did was to remove `#if 0`...`#endif` guards, it was
> unfortunately quite easy for this regression to slip in.

Yeah, I recall wondering why it was safe to suddenly enable the
segment of the disabled code all of sudden.

Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote:
>
>> From: Trygve Aaberge <trygveaa@gmail.com>
>> 
>> When you hit ^C all the processes in the tree receives it. When a git
>> command uses a pager, git ignores this and waits until the pager quits.
>> However, when using an alias there is an additional process in the tree
>> which didn't ignore the signal. That caused it to exit which in turn
>> caused the pager to exit. This fixes that for aliases to builtins.
>> 
>> This was originally fixed in 46df6906f3 (see that for a more in
>> explanation), but broke by a regression in b914084007.
>
> Good catch. The regression is technically in b914084007, but the real
> culprit is the extra (commented out) code path added in ee4512ed48
> (trace2: create new combined trace facility, 2019-02-22).

True, as Dscho also mentioned.  I'll just do "b914084007" =>
"ee4512ed48 and b914084007" while queueing.

> Your fix here looks good, but it does make me wonder if we could avoid
> or shrink this duplicate code path. I.e., could it just do the logging
> necessary but still leave the actual process spawn to the
> execv_dashed_external() below. It may be hard to untangle, though, so
> certainly this makes sense in the meantime.

It probably is a bit more involved than a typical low hanging
fruit.  I am OK to leave this for later clean-up.

> A test would be nice, but I don't think it's very feasible for the same
> reason mentioned in 46df6906f3.

True.  Thanks.

Re: [PATCH 2/2] Wait for child on signal death for aliases to externals

[Trygve Aaberge]

On Mon, Jul 06, 2020 at 17:14:03 -0400, Jeff King wrote:
>     I guess to recreate that you'd need to trigger the pager inside the
>     alias itself, like:
> 
>       $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo
>       ^C
> 
>     which does exhibit the annoying behavior (we exit, and pgrp loses
>     the tty session leader bit, and the pager gets EIO).

Yes, that's correct. So it's a rather niche use case. The main thing for me
was the first commit, but I figured I should fix this too while I was at it. I
don't think I have any current use cases where I would need this fix, but I
could imagine some existing. For instance, before stash list got the -p
option, I had this alias:

  stash-p = !git show $(git stash list | cut -d: -f1)

And this is one use case where the pager is invoked inside the alias, so the
first patch doesn't help, but the second one fixes it. While this alias isn't
necessary anymore, there could be similar use cases.

>   - the child for some reason decides not to respect the signal.
>     Obviously running a pager is one reason it would decide to do so,
>     and we'd be improving the behavior there. I have trouble imagining a
>     case where waiting for it would do the _wrong_ thing. I.e., if I do:
> 
>       $ git -c alias.foo='!trap "echo got signal" INT; sleep 5' foo
> 
>     it probably does make sense for us to continue to wait on that alias
>     to complete (I'm not sure what anyone would try to accomplish with
>     that, but waiting seems like the least-astonishing thing to me).

Yeah, I agree. Since it's an alias to an external command, I think it should
behave just as when running that external command by itself where there would
be no parent killing it on ^C.

> However, there is one weirdness worth thinking about. Because the
> wait_after_clean feature relies on actually trying to clean the child,
> Git will actually send a signal to the alias shell. For a signal, we'll
> try to pass along the same signal, so it just means the shell will get
> SIGINT twice in this case (or more likely, they'll race and we'll ignore
> the duplicate signal while the child is in its own handler).

Hm, okay, not sure if anything should be done about this.

> But a more interesting case is if somebody sends the parent git process
> a signal but _not_ the child (e.g., kill -TERM). Then we'd pass SIGTERM
> along to the child. I'd venture to say that's _probably_ the right thing
> to do, if only because it's exactly what we do for a dashed external as
> well.

Hm, not sure. If you run a process in bash and send TERM to bash it seems to
just ignore the signal. The child keeps running, and bash even keeps running
after the child exits. If you don't run a process in bash it respects TERM and
exits. I'm wondering if an alias executing an external command should behave
the same way as when a shell does it.

Though, I also see that this only happens when bash runs interactively. If you
run `bash -c 'echo; sleep 1000'` it exits, but sleep keeps running.

> Sorry that ended up so long-winded, but my conclusion is: this is doing
> the right thing. We should probably embed some of that discussion in the
> commit message, but I think the simplest argument is just: this is what
> we do for external commands we run, so treating shell aliases the same
> in terms of passing along signals makes things simple and consistent.

Thanks, yeah I'll send an updated patch with a better description.

-- 
Trygve Aaberge

Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins

[Trygve Aaberge]

On Mon, Jul 06, 2020 at 18:50:05 -0700, Junio C Hamano wrote:
> True, as Dscho also mentioned.  I'll just do "b914084007" =>
> "ee4512ed48 and b914084007" while queueing.

I noticed that I was missing a word in the message, and I'm sending a new
patch with a better commit message for the second commit, so I'll update this
in the first commit and send a new patchset.

> > A test would be nice, but I don't think it's very feasible for the same
> > reason mentioned in 46df6906f3.

Yeah, I don't think I have a good enough grasp of how to test this, and I saw
that there wasn't any existing tests for it, so that's why I dropped it.

-- 
Trygve Aaberge

[PATCH v2 1/2] Wait for child on signal death for aliases to builtins

[trygveaa]

From: Trygve Aaberge <trygveaa@gmail.com>

When you hit ^C all the processes in the tree receives it. When a git
command uses a pager, git ignores this and waits until the pager quits.
However, when using an alias there is an additional process in the tree
which didn't ignore the signal. That caused it to exit which in turn
caused the pager to exit. This fixes that for aliases to builtins.

This was originally fixed in 46df6906f3 (see that for a more in-depth
explanation), but broke by a regression in ee4512ed48 and b914084007.

Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>
---
 git.c         | 2 +-
 run-command.c | 1 +
 run-command.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 7be7ad34bd..9b8d3c92e6 100644
--- a/git.c
+++ b/git.c
@@ -768,7 +768,7 @@ static int run_argv(int *argcp, const char ***argv)
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
 			i = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT, "git_alias");
+						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/run-command.c b/run-command.c
index f5e1149f9b..5d65335d13 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
 	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
+	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
 	cmd.dir = dir;
 	cmd.env = env;
 	cmd.trace2_child_class = tr2_class;
diff --git a/run-command.h b/run-command.h
index 0f3cc73ab6..1641ccf94b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -224,6 +224,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);
 #define RUN_SILENT_EXEC_FAILURE 8
 #define RUN_USING_SHELL 16
 #define RUN_CLEAN_ON_EXIT 32
+#define RUN_WAIT_AFTER_CLEAN 64
 
 /**
  * Convenience functions that encapsulate a sequence of
-- 
2.26.2.2.g2208536367

[PATCH v2 2/2] Wait for child on signal death for aliases to externals

[trygveaa]

From: Trygve Aaberge <trygveaa@gmail.com>

When we are running an alias to an external command, we want to wait for
that process to exit even after receiving ^C which normally kills the
git process. This is useful when the process is ignoring SIGINT (which
e.g. pagers often do), and then we don't want it to be killed.

Having an alias which invokes a pager is probably not common, but it can
be useful e.g. if you have an alias to a git command which uses a
subshell as one of the arguments (in which case you have to use an
external command, not an alias to a builtin).

This patch is similar to the previous commit, but the previous commit
fixed this only for aliases to builtins, while this commit does the same
for aliases to external commands. In addition to waiting after clean
like the previous commit, this also enables cleaning the child (that was
already enabled for aliases to builtins before the previous commit),
because wait_after_clean relies on it. Lastly, while the previous commit
fixed a regression, I don't think this has ever worked properly.

Signed-off-by: Trygve Aaberge <trygveaa@gmail.com>
---
 git.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git.c b/git.c
index 9b8d3c92e6..c0698c7d45 100644
--- a/git.c
+++ b/git.c
@@ -345,6 +345,8 @@ static int handle_alias(int *argcp, const char ***argv)
 			commit_pager_choice();
 
 			child.use_shell = 1;
+			child.clean_on_exit = 1;
+			child.wait_after_clean = 1;
 			child.trace2_child_class = "shell_alias";
 			argv_array_push(&child.args, alias_string + 1);
 			argv_array_pushv(&child.args, (*argv) + 1);
-- 
2.26.2.2.g2208536367

Re: [PATCH 2/2] Wait for child on signal death for aliases to externals

[Jeff King]

On Tue, Jul 07, 2020 at 12:19:59PM +0200, Trygve Aaberge wrote:

> On Mon, Jul 06, 2020 at 17:14:03 -0400, Jeff King wrote:
> >     I guess to recreate that you'd need to trigger the pager inside the
> >     alias itself, like:
> > 
> >       $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo
> >       ^C
> > 
> >     which does exhibit the annoying behavior (we exit, and pgrp loses
> >     the tty session leader bit, and the pager gets EIO).
> 
> Yes, that's correct. So it's a rather niche use case. The main thing for me
> was the first commit, but I figured I should fix this too while I was at it. I
> don't think I have any current use cases where I would need this fix, but I
> could imagine some existing. For instance, before stash list got the -p
> option, I had this alias:
> 
>   stash-p = !git show $(git stash list | cut -d: -f1)
> 
> And this is one use case where the pager is invoked inside the alias, so the
> first patch doesn't help, but the second one fixes it. While this alias isn't
> necessary anymore, there could be similar use cases.

Thanks for this real-world example. I agree that particular one isn't
necessary anymore, but to me it provides a compelling argument. It's not
all that far-fetched that somebody runs a git command that triggers a
pager inside a shell alias.

-Peff