| Message ID | 20200704221839.421997-2-trygveaa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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.
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.
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
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);