git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: trygveaa@gmail.com
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 2/2] Wait for child on signal death for aliases to externals
Date: Mon, 6 Jul 2020 17:14:03 -0400	[thread overview]
Message-ID: <20200706211403.GB85133@coredump.intra.peff.net> (raw)
In-Reply-To: <20200704221839.421997-2-trygveaa@gmail.com>

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

  reply	other threads:[~2020-07-06 21:14 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-04 22:18 [PATCH 1/2] Wait for child on signal death for aliases to builtins trygveaa
2020-07-04 22:18 ` [PATCH 2/2] Wait for child on signal death for aliases to externals trygveaa
2020-07-06 21:14   ` Jeff King [this message]
2020-07-07  1:38     ` Junio C Hamano
2020-07-07 10:19     ` Trygve Aaberge
2020-07-07 22:06       ` Jeff King
2020-07-05  2:15 ` [PATCH 1/2] Wait for child on signal death for aliases to builtins Johannes Schindelin
2020-07-07  1:48   ` Junio C Hamano
2020-07-06 20:41 ` Jeff King
2020-07-07  1:50   ` Junio C Hamano
2020-07-07 10:23     ` Trygve Aaberge
2020-07-07 12:17 ` [PATCH v2 " trygveaa
2020-07-07 12:17   ` [PATCH v2 2/2] Wait for child on signal death for aliases to externals trygveaa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200706211403.GB85133@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=trygveaa@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).