{"thread":{"id":"53798","subject":"[PATCH 2/2] Wait for child on signal death for aliases to externals","startedAt":"2020-07-04T22:19:24Z","lastAt":"2020-07-07T22:06:29Z","messageCount":13,"participants":["trygveaa@gmail.com","Johannes Schindelin","Jeff King","Junio C Hamano","Trygve Aaberge"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"401026","messageId":"20200704221839.421997-2-trygveaa@gmail.com","threadId":"53798","inReplyTo":"20200704221839.421997-1-trygveaa@gmail.com","subject":"[PATCH 2/2] Wait for child on signal death for aliases to externals","fromName":"","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-04T22:18:38Z","receivedAt":"2020-07-04T22:19:24Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"From: Trygve Aaberge \n\nThe previous commit fixed this only for aliases to builtins, this commit\ndoes the same for aliases to external commands. While the previous\ncommit fixed a regression, I don't think this has ever worked properly.\n\nSigned-off-by: Trygve Aaberge \n---\n\nI can't say for sure if this will have any unintended side effects, so\nif someone with more knowledge about it can check/verify it, that would\nbe great.\n\n git.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex c8336773e3..0797ac6a80 100644\n--- a/git.c\n+++ b/git.c\n@@ -346,6 +346,8 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tcommit_pager_choice();\n \n \t\t\tchild.use_shell = 1;\n+\t\t\tchild.clean_on_exit = 1;\n+\t\t\tchild.wait_after_clean = 1;\n \t\t\tchild.trace2_child_class = \"shell_alias\";\n \t\t\targv_array_push(&child.args, alias_string + 1);\n \t\t\targv_array_pushv(&child.args, (*argv) + 1);\n-- \n2.27.0\n\n"},{"id":"401027","messageId":"20200704221839.421997-1-trygveaa@gmail.com","threadId":"53798","inReplyTo":null,"subject":"[PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-04T22:18:37Z","receivedAt":"2020-07-04T22:19:24Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"From: Trygve Aaberge \n\nWhen you hit ^C all the processes in the tree receives it. When a git\ncommand uses a pager, git ignores this and waits until the pager quits.\nHowever, when using an alias there is an additional process in the tree\nwhich didn't ignore the signal. That caused it to exit which in turn\ncaused the pager to exit. This fixes that for aliases to builtins.\n\nThis was originally fixed in 46df6906f3 (see that for a more in\nexplanation), but broke by a regression in b914084007.\n\nSigned-off-by: Trygve Aaberge \n---\n git.c | 2 +-\n run-command.c | 1 +\n run-command.h | 1 +\n 3 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git.c b/git.c\nindex a2d337eed7..c8336773e3 100644\n--- a/git.c\n+++ b/git.c\n@@ -767,7 +767,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t\t * OK to return. Otherwise, we just pass along the status code.\n \t\t\t */\n \t\t\ti = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |\n-\t\t\t\t\t\t RUN_CLEAN_ON_EXIT, \"git_alias\");\n+\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/run-command.c b/run-command.c\nindex 9b3a57d1e3..a735e380a9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n \tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n+\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n \tcmd.dir = dir;\n \tcmd.env = env;\n \tcmd.trace2_child_class = tr2_class;\ndiff --git a/run-command.h b/run-command.h\nindex 191dfcdafe..ef3071a565 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -229,6 +229,7 @@ int run_auto_gc(int quiet);\n #define RUN_SILENT_EXEC_FAILURE 8\n #define RUN_USING_SHELL 16\n #define RUN_CLEAN_ON_EXIT 32\n+#define RUN_WAIT_AFTER_CLEAN 64\n \n /**\n * Convenience functions that encapsulate a sequence of\n-- \n2.27.0\n\n"},{"id":"401042","messageId":"nycvar.QRO.7.76.6.2007050410100.50@tvgsbejvaqbjf.bet","threadId":"53798","inReplyTo":"20200704221839.421997-1-trygveaa@gmail.com","subject":"Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-05T02:15:56Z","receivedAt":"2020-07-06T14:38:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 5 Jul 2020, trygveaa@gmail.com wrote:\n\n> From: Trygve Aaberge \n>\n> When you hit ^C all the processes in the tree receives it. When a git\n> command uses a pager, git ignores this and waits until the pager quits.\n> However, when using an alias there is an additional process in the tree\n> which didn't ignore the signal. That caused it to exit which in turn\n> caused the pager to exit. This fixes that for aliases to builtins.\n>\n> This was originally fixed in 46df6906f3 (see that for a more in\n> explanation), but broke by a regression in b914084007.\n\nThank you for those references. I did try to make sure that b914084007\nwould not regress on anything, but apparently I failed to verify the final\nform. Since all it did was to remove `#if 0`...`#endif` guards, it was\nunfortunately quite easy for this regression to slip in.\n\nIIRC the original code inside those guards was modeled closely after\n`execv_dashed_external()`, but it does not really look very much like it\nanymore now, does it? (And it looks as if 2b296c93d49\n(execv_dashed_external: use child_process struct, 2017-01-06) was\nresponsible for that.)\n\nIn any case, thank you for the patch, it looks good to me!\n\nCiao,\nDscho\n\n>\n> Signed-off-by: Trygve Aaberge \n> ---\n> git.c | 2 +-\n> run-command.c | 1 +\n> run-command.h | 1 +\n> 3 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/git.c b/git.c\n> index a2d337eed7..c8336773e3 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -767,7 +767,7 @@ static int run_argv(int *argcp, const char ***argv)\n> \t\t\t * OK to return. Otherwise, we just pass along the status code.\n> \t\t\t */\n> \t\t\ti = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |\n> -\t\t\t\t\t\t RUN_CLEAN_ON_EXIT, \"git_alias\");\n> +\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n> \t\t\tif (i >= 0 || errno != ENOENT)\n> \t\t\t\texit(i);\n> \t\t\tdie(\"could not execute builtin %s\", **argv);\n> diff --git a/run-command.c b/run-command.c\n> index 9b3a57d1e3..a735e380a9 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n> \tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n> \tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n> \tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n> +\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n> \tcmd.dir = dir;\n> \tcmd.env = env;\n> \tcmd.trace2_child_class = tr2_class;\n> diff --git a/run-command.h b/run-command.h\n> index 191dfcdafe..ef3071a565 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -229,6 +229,7 @@ int run_auto_gc(int quiet);\n> #define RUN_SILENT_EXEC_FAILURE 8\n> #define RUN_USING_SHELL 16\n> #define RUN_CLEAN_ON_EXIT 32\n> +#define RUN_WAIT_AFTER_CLEAN 64\n>\n> /**\n> * Convenience functions that encapsulate a sequence of\n> --\n> 2.27.0\n>\n>\n"},{"id":"401052","messageId":"20200706204106.GA85133@coredump.intra.peff.net","threadId":"53798","inReplyTo":"20200704221839.421997-1-trygveaa@gmail.com","subject":"Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-06T20:41:06Z","receivedAt":"2020-07-06T20:41:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote:\n\n> From: Trygve Aaberge \n> \n> When you hit ^C all the processes in the tree receives it. When a git\n> command uses a pager, git ignores this and waits until the pager quits.\n> However, when using an alias there is an additional process in the tree\n> which didn't ignore the signal. That caused it to exit which in turn\n> caused the pager to exit. This fixes that for aliases to builtins.\n> \n> This was originally fixed in 46df6906f3 (see that for a more in\n> explanation), but broke by a regression in b914084007.\n\nGood catch. The regression is technically in b914084007, but the real\nculprit is the extra (commented out) code path added in ee4512ed48\n(trace2: create new combined trace facility, 2019-02-22).\n\nYour fix here looks good, but it does make me wonder if we could avoid\nor shrink this duplicate code path. I.e., could it just do the logging\nnecessary but still leave the actual process spawn to the\nexecv_dashed_external() below. It may be hard to untangle, though, so\ncertainly this makes sense in the meantime.\n\nA test would be nice, but I don't think it's very feasible for the same\nreason mentioned in 46df6906f3.\n\n-Peff\n"},{"id":"401053","messageId":"20200706211403.GB85133@coredump.intra.peff.net","threadId":"53798","inReplyTo":"20200704221839.421997-2-trygveaa@gmail.com","subject":"Re: [PATCH 2/2] Wait for child on signal death for aliases to externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-06T21:14:03Z","receivedAt":"2020-07-06T21:14:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 05, 2020 at 12:18:38AM +0200, trygveaa@gmail.com wrote:\n\n> From: Trygve Aaberge \n> \n> The previous commit fixed this only for aliases to builtins, this commit\n> does the same for aliases to external commands. While the previous\n> commit fixed a regression, I don't think this has ever worked properly.\n> \n> Signed-off-by: Trygve Aaberge \n\nUsually when I find myself saying \"the previous commit\" and \"does the\nsame\" in a commit message, it's a good indication that it might be worth\ncombining the two commits into one. But in this case I think you're\nright to keep them separate; the other one was a regression fix, and\nthis would be a change in behavior.\n\nBut it might be worth explaining more what the user-visible behavior\nwe're trying to solve is.\n\n> I can't say for sure if this will have any unintended side effects, so\n> if someone with more knowledge about it can check/verify it, that would\n> be great.\n\nLet's try to break down what differences the user might see.\n\nThe original is using the full run_command(), so we'd always be waiting\nfor the alias command to finish before exiting under normal\ncircumstances. The only difference is that now when die due to a signal,\nwe'd wait then, too.\n\nAnd that only matters if:\n\n - we've spawned a pager that doesn't die due to the signal, and we\n want to wait until it has finished. However, that seems to work\n already for me in a quick test of:\n\n $ git -p -c alias.foo='!echo foo; sleep 10; echo bar' foo\n ^C\n\n where the pager keeps running (because it ignores the signal), and\n the main git process doesn't exit either (because it's waiting for\n the pager to finish)\n\n I think this case is different than the one fixed by 46df6906f3\n (execv_dashed_external: wait for child on signal death, 2017-01-06)\n because the pager is spawned by the main Git process (so it knows to\n wait), rather than the child dashed-external.\n\n I guess to recreate that you'd need to trigger the pager inside the\n alias itself, like:\n\n $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo\n ^C\n\n which does exhibit the annoying behavior (we exit, and pgrp loses\n the tty session leader bit, and the pager gets EIO).\n\n - the child for some reason decides not to respect the signal.\n Obviously running a pager is one reason it would decide to do so,\n and we'd be improving the behavior there. I have trouble imagining a\n case where waiting for it would do the _wrong_ thing. I.e., if I do:\n\n $ git -c alias.foo='!trap \"echo got signal\" INT; sleep 5' foo\n\n it probably does make sense for us to continue to wait on that alias\n to complete (I'm not sure what anyone would try to accomplish with\n that, but waiting seems like the least-astonishing thing to me).\n\nSo I think this is moving in the right direction.\n\nHowever, there is one weirdness worth thinking about. Because the\nwait_after_clean feature relies on actually trying to clean the child,\nGit will actually send a signal to the alias shell. For a signal, we'll\ntry to pass along the same signal, so it just means the shell will get\nSIGINT twice in this case (or more likely, they'll race and we'll ignore\nthe duplicate signal while the child is in its own handler).\n\nWe'd likewise send a signal if we hit a normal exit(), but we shouldn't\ndo so because we're already blocked waiting for the child to exit.\n\nBut a more interesting case is if somebody sends the parent git process\na signal but _not_ the child (e.g., kill -TERM). Then we'd pass SIGTERM\nalong to the child. I'd venture to say that's _probably_ the right thing\nto do, if only because it's exactly what we do for a dashed external as\nwell.\n\nSorry that ended up so long-winded, but my conclusion is: this is doing\nthe right thing. We should probably embed some of that discussion in the\ncommit message, but I think the simplest argument is just: this is what\nwe do for external commands we run, so treating shell aliases the same\nin terms of passing along signals makes things simple and consistent.\n\n-Peff\n"},{"id":"401062","messageId":"xmqq5zb085pd.fsf@gitster.c.googlers.com","threadId":"53798","inReplyTo":"20200706211403.GB85133@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Wait for child on signal death for aliases to externals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T01:38:38Z","receivedAt":"2020-07-07T01:38:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Sorry that ended up so long-winded, but my conclusion is: this is doing\n> the right thing. We should probably embed some of that discussion in the\n> commit message, but I think the simplest argument is just: this is what\n> we do for external commands we run, so treating shell aliases the same\n> in terms of passing along signals makes things simple and consistent.\n\nThanks for a great analysis to help the author.\n\n"},{"id":"401063","messageId":"xmqq1rlo859k.fsf@gitster.c.googlers.com","threadId":"53798","inReplyTo":"nycvar.QRO.7.76.6.2007050410100.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T01:48:07Z","receivedAt":"2020-07-07T01:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Thank you for those references. I did try to make sure that b914084007\n> would not regress on anything, but apparently I failed to verify the final\n> form. Since all it did was to remove `#if 0`...`#endif` guards, it was\n> unfortunately quite easy for this regression to slip in.\n\nYeah, I recall wondering why it was safe to suddenly enable the\nsegment of the disabled code all of sudden.\n\n"},{"id":"401064","messageId":"xmqqwo3g6qlu.fsf@gitster.c.googlers.com","threadId":"53798","inReplyTo":"20200706204106.GA85133@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T01:50:05Z","receivedAt":"2020-07-07T01:50:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jul 05, 2020 at 12:18:37AM +0200, trygveaa@gmail.com wrote:\n>\n>> From: Trygve Aaberge \n>> \n>> When you hit ^C all the processes in the tree receives it. When a git\n>> command uses a pager, git ignores this and waits until the pager quits.\n>> However, when using an alias there is an additional process in the tree\n>> which didn't ignore the signal. That caused it to exit which in turn\n>> caused the pager to exit. This fixes that for aliases to builtins.\n>> \n>> This was originally fixed in 46df6906f3 (see that for a more in\n>> explanation), but broke by a regression in b914084007.\n>\n> Good catch. The regression is technically in b914084007, but the real\n> culprit is the extra (commented out) code path added in ee4512ed48\n> (trace2: create new combined trace facility, 2019-02-22).\n\nTrue, as Dscho also mentioned. I'll just do \"b914084007\" =>\n\"ee4512ed48 and b914084007\" while queueing.\n\n> Your fix here looks good, but it does make me wonder if we could avoid\n> or shrink this duplicate code path. I.e., could it just do the logging\n> necessary but still leave the actual process spawn to the\n> execv_dashed_external() below. It may be hard to untangle, though, so\n> certainly this makes sense in the meantime.\n\nIt probably is a bit more involved than a typical low hanging\nfruit. I am OK to leave this for later clean-up.\n\n> A test would be nice, but I don't think it's very feasible for the same\n> reason mentioned in 46df6906f3.\n\nTrue. Thanks.\n"},{"id":"401087","messageId":"20200707101959.qsuumtuelepnxore@aaberge.net","threadId":"53798","inReplyTo":"20200706211403.GB85133@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Wait for child on signal death for aliases to externals","fromName":"Trygve Aaberge","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-07T10:19:59Z","receivedAt":"2020-07-07T10:22:11Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"On Mon, Jul 06, 2020 at 17:14:03 -0400, Jeff King wrote:\n> I guess to recreate that you'd need to trigger the pager inside the\n> alias itself, like:\n> \n> $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo\n> ^C\n> \n> which does exhibit the annoying behavior (we exit, and pgrp loses\n> the tty session leader bit, and the pager gets EIO).\n\nYes, that's correct. So it's a rather niche use case. The main thing for me\nwas the first commit, but I figured I should fix this too while I was at it. I\ndon't think I have any current use cases where I would need this fix, but I\ncould imagine some existing. For instance, before stash list got the -p\noption, I had this alias:\n\n stash-p = !git show $(git stash list | cut -d: -f1)\n\nAnd this is one use case where the pager is invoked inside the alias, so the\nfirst patch doesn't help, but the second one fixes it. While this alias isn't\nnecessary anymore, there could be similar use cases.\n\n> - the child for some reason decides not to respect the signal.\n> Obviously running a pager is one reason it would decide to do so,\n> and we'd be improving the behavior there. I have trouble imagining a\n> case where waiting for it would do the _wrong_ thing. I.e., if I do:\n> \n> $ git -c alias.foo='!trap \"echo got signal\" INT; sleep 5' foo\n> \n> it probably does make sense for us to continue to wait on that alias\n> to complete (I'm not sure what anyone would try to accomplish with\n> that, but waiting seems like the least-astonishing thing to me).\n\nYeah, I agree. Since it's an alias to an external command, I think it should\nbehave just as when running that external command by itself where there would\nbe no parent killing it on ^C.\n\n> However, there is one weirdness worth thinking about. Because the\n> wait_after_clean feature relies on actually trying to clean the child,\n> Git will actually send a signal to the alias shell. For a signal, we'll\n> try to pass along the same signal, so it just means the shell will get\n> SIGINT twice in this case (or more likely, they'll race and we'll ignore\n> the duplicate signal while the child is in its own handler).\n\nHm, okay, not sure if anything should be done about this.\n\n> But a more interesting case is if somebody sends the parent git process\n> a signal but _not_ the child (e.g., kill -TERM). Then we'd pass SIGTERM\n> along to the child. I'd venture to say that's _probably_ the right thing\n> to do, if only because it's exactly what we do for a dashed external as\n> well.\n\nHm, not sure. If you run a process in bash and send TERM to bash it seems to\njust ignore the signal. The child keeps running, and bash even keeps running\nafter the child exits. If you don't run a process in bash it respects TERM and\nexits. I'm wondering if an alias executing an external command should behave\nthe same way as when a shell does it.\n\nThough, I also see that this only happens when bash runs interactively. If you\nrun `bash -c 'echo; sleep 1000'` it exits, but sleep keeps running.\n\n> Sorry that ended up so long-winded, but my conclusion is: this is doing\n> the right thing. We should probably embed some of that discussion in the\n> commit message, but I think the simplest argument is just: this is what\n> we do for external commands we run, so treating shell aliases the same\n> in terms of passing along signals makes things simple and consistent.\n\nThanks, yeah I'll send an updated patch with a better description.\n\n-- \nTrygve Aaberge\n"},{"id":"401088","messageId":"20200707102355.2kukfzxt7vhhenfy@aaberge.net","threadId":"53798","inReplyTo":"xmqqwo3g6qlu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] Wait for child on signal death for aliases to builtins","fromName":"Trygve Aaberge","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-07T10:23:55Z","receivedAt":"2020-07-07T10:24:01Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"On Mon, Jul 06, 2020 at 18:50:05 -0700, Junio C Hamano wrote:\n> True, as Dscho also mentioned. I'll just do \"b914084007\" =>\n> \"ee4512ed48 and b914084007\" while queueing.\n\nI noticed that I was missing a word in the message, and I'm sending a new\npatch with a better commit message for the second commit, so I'll update this\nin the first commit and send a new patchset.\n\n> > A test would be nice, but I don't think it's very feasible for the same\n> > reason mentioned in 46df6906f3.\n\nYeah, I don't think I have a good enough grasp of how to test this, and I saw\nthat there wasn't any existing tests for it, so that's why I dropped it.\n\n-- \nTrygve Aaberge\n"},{"id":"401089","messageId":"20200707121716.438318-1-trygveaa@gmail.com","threadId":"53798","inReplyTo":"20200704221839.421997-1-trygveaa@gmail.com","subject":"[PATCH v2 1/2] Wait for child on signal death for aliases to builtins","fromName":"","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-07T12:17:14Z","receivedAt":"2020-07-07T12:17:27Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"From: Trygve Aaberge \n\nWhen you hit ^C all the processes in the tree receives it. When a git\ncommand uses a pager, git ignores this and waits until the pager quits.\nHowever, when using an alias there is an additional process in the tree\nwhich didn't ignore the signal. That caused it to exit which in turn\ncaused the pager to exit. This fixes that for aliases to builtins.\n\nThis was originally fixed in 46df6906f3 (see that for a more in-depth\nexplanation), but broke by a regression in ee4512ed48 and b914084007.\n\nSigned-off-by: Trygve Aaberge \n---\n git.c | 2 +-\n run-command.c | 1 +\n run-command.h | 1 +\n 3 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git.c b/git.c\nindex 7be7ad34bd..9b8d3c92e6 100644\n--- a/git.c\n+++ b/git.c\n@@ -768,7 +768,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t\t * OK to return. Otherwise, we just pass along the status code.\n \t\t\t */\n \t\t\ti = run_command_v_opt_tr2(args.argv, RUN_SILENT_EXEC_FAILURE |\n-\t\t\t\t\t\t RUN_CLEAN_ON_EXIT, \"git_alias\");\n+\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/run-command.c b/run-command.c\nindex f5e1149f9b..5d65335d13 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1039,6 +1039,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n \tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n+\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n \tcmd.dir = dir;\n \tcmd.env = env;\n \tcmd.trace2_child_class = tr2_class;\ndiff --git a/run-command.h b/run-command.h\nindex 0f3cc73ab6..1641ccf94b 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -224,6 +224,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);\n #define RUN_SILENT_EXEC_FAILURE 8\n #define RUN_USING_SHELL 16\n #define RUN_CLEAN_ON_EXIT 32\n+#define RUN_WAIT_AFTER_CLEAN 64\n \n /**\n * Convenience functions that encapsulate a sequence of\n-- \n2.26.2.2.g2208536367\n\n"},{"id":"401090","messageId":"20200707121716.438318-2-trygveaa@gmail.com","threadId":"53798","inReplyTo":"20200707121716.438318-1-trygveaa@gmail.com","subject":"[PATCH v2 2/2] Wait for child on signal death for aliases to externals","fromName":"","fromEmail":"trygveaa@gmail.com","sentAt":"2020-07-07T12:17:15Z","receivedAt":"2020-07-07T12:17:31Z","isPatch":true,"sender":{"key":"trygveaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/601966?v=4"},"body":"From: Trygve Aaberge \n\nWhen we are running an alias to an external command, we want to wait for\nthat process to exit even after receiving ^C which normally kills the\ngit process. This is useful when the process is ignoring SIGINT (which\ne.g. pagers often do), and then we don't want it to be killed.\n\nHaving an alias which invokes a pager is probably not common, but it can\nbe useful e.g. if you have an alias to a git command which uses a\nsubshell as one of the arguments (in which case you have to use an\nexternal command, not an alias to a builtin).\n\nThis patch is similar to the previous commit, but the previous commit\nfixed this only for aliases to builtins, while this commit does the same\nfor aliases to external commands. In addition to waiting after clean\nlike the previous commit, this also enables cleaning the child (that was\nalready enabled for aliases to builtins before the previous commit),\nbecause wait_after_clean relies on it. Lastly, while the previous commit\nfixed a regression, I don't think this has ever worked properly.\n\nSigned-off-by: Trygve Aaberge \n---\n git.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex 9b8d3c92e6..c0698c7d45 100644\n--- a/git.c\n+++ b/git.c\n@@ -345,6 +345,8 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tcommit_pager_choice();\n \n \t\t\tchild.use_shell = 1;\n+\t\t\tchild.clean_on_exit = 1;\n+\t\t\tchild.wait_after_clean = 1;\n \t\t\tchild.trace2_child_class = \"shell_alias\";\n \t\t\targv_array_push(&child.args, alias_string + 1);\n \t\t\targv_array_pushv(&child.args, (*argv) + 1);\n-- \n2.26.2.2.g2208536367\n\n"},{"id":"401160","messageId":"20200707220626.GC2300296@coredump.intra.peff.net","threadId":"53798","inReplyTo":"20200707101959.qsuumtuelepnxore@aaberge.net","subject":"Re: [PATCH 2/2] Wait for child on signal death for aliases to externals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-07T22:06:26Z","receivedAt":"2020-07-07T22:06:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2020 at 12:19:59PM +0200, Trygve Aaberge wrote:\n\n> On Mon, Jul 06, 2020 at 17:14:03 -0400, Jeff King wrote:\n> > I guess to recreate that you'd need to trigger the pager inside the\n> > alias itself, like:\n> > \n> > $ git -c alias.foo='!{ echo foo; sleep 10; echo bar; } | less' foo\n> > ^C\n> > \n> > which does exhibit the annoying behavior (we exit, and pgrp loses\n> > the tty session leader bit, and the pager gets EIO).\n> \n> Yes, that's correct. So it's a rather niche use case. The main thing for me\n> was the first commit, but I figured I should fix this too while I was at it. I\n> don't think I have any current use cases where I would need this fix, but I\n> could imagine some existing. For instance, before stash list got the -p\n> option, I had this alias:\n> \n> stash-p = !git show $(git stash list | cut -d: -f1)\n> \n> And this is one use case where the pager is invoked inside the alias, so the\n> first patch doesn't help, but the second one fixes it. While this alias isn't\n> necessary anymore, there could be similar use cases.\n\nThanks for this real-world example. I agree that particular one isn't\nnecessary anymore, but to me it provides a compelling argument. It's not\nall that far-fetched that somebody runs a git command that triggers a\npager inside a shell alias.\n\n-Peff\n"}]}