Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias

[Jeff King <peff@peff.net>]

On Thu, Dec 24, 2015 at 04:35:33PM +0700, Duy Nguyen wrote:

> On Thu, Dec 24, 2015 at 4:31 AM, Jeff King <peff@peff.net> wrote:
> >   2. I doubt anybody is actually seeing this in practice anymore. But
> >      maybe I am misunderstanding something in Duy's series that changes
> >      this.
> 
> There are two parts in your patch, one (that you two seemed to focus
> on) about return code with "!" aliases. Another, suppressing SIGPIPE,
> affects more than "!" aliases.

Sorry if I was confusing; most of the examples in my previous message
are about the SIGPIPE thing. I was having trouble triggering the message
in practice, even for externals (because the error message goes to the
pager, too!).

> In my case it's execv_dashed_external(). Non-"!" aliases are now
> forced to use that function.

Thanks, this is the part I was missing.

The outer git wrapper doesn't start the pager, so its stderr still gets
seen by the user. But the _inner_ git-log does start the pager, and then
dies of SIGPIPE.

So yeah, I think we want something like this on top of
nd/clear-gitenv-upon-use-of-alias.

-- >8 --
Subject: [PATCH] run-command: don't warn on SIGPIPE deaths

When git executes a sub-command, we print a warning if the
command dies due to a signal, but make an exception for
"uninteresting" cases like SIGINT and SIGQUIT (since the
user presumably just hit ^C).

We should make a similar exception for SIGPIPE, because it's
an expected and uninteresting return in most cases; it
generally means the user quit the pager before git had
finished generating all output.  This used to be very hard
to trigger in practice, because:

  1. We only complain if we see a real SIGPIPE death, not
     the shell-induced 141 exit code. This means that
     anything we run via the shell does not trigger the
     warning, which includes most non-trivial aliases.

  2. The common case for SIGPIPE is the user quitting the
     pager before git has finished generating all output.
     But if the user triggers a pager with "-p", we redirect
     the git wrapper's stderr to that pager, too.  Since the
     pager is dead, it means that the message goes nowhere.

  3. You can see it if you run your own pager, like
     "git foo | head". But that only happens if "foo" is a
     non-builtin (so it doesn't work with "log", for
     example).

However, it may become more common after 86d26f2, which
teaches alias to re-exec builtins rather than running them
in the same process. This case doesn't trigger (1), as we
don't need a shell to run a git command. It doesn't trigger
(2), because the pager is not started by the original git,
but by the inner re-exec of git. And it doesn't trigger (3),
because builtins are treated more like non-builtins in this
case.

Given how flaky this message already is (e.g., you cannot
even know whether you will see it, as git optimizes out some
shell invocations behind the scenes based on the contents of
the command!), and that it is unlikely to ever provide
useful information, let's suppress it for all cases of
SIGPIPE.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 13fa452..694a6ff 100644
--- a/run-command.c
+++ b/run-command.c
@@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		if (code != SIGINT && code != SIGQUIT)
+		if (code != SIGINT && code != SIGQUIT && code != SIGPIPE)
 			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
-- 
2.7.0.rc3.367.g09631da