| Message ID | 20200704221839.421997-1-trygveaa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Wait for child on signal death for aliases to builtins | expand |
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 > >
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
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.
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.
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.
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