Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases

On Fri, Jan 04, 2013 at 02:20:52PM -0800, Junio C Hamano wrote:

With respect to error messages, I'd think they should behave the same. But they don't necessarily. The latter does not print any message at all. But consider this version of the former:

  $ cat foo
  #!/bin/sh
  exec cat longfile

  $ git -c alias.o='!./foo' o | head -n 1
  error: ./foo died of signal 13
  fatal: While expanding alias 'o': './foo': Success

So why don't we see that message more often? There are two reasons.

One reason is that we piped it ourselves here. When git pipes to the pager, it sends stderr along the same channel. So even if you did:

  GIT_PAGER='head -n 1' git -p -c alias.o='!./foo' o

git writes the error, but it goes to the pager, which has already ended (since that is what caused the SIGPIPE in the first place). But imagine that your sub-command is actually invoking git itself, and it is the sub-git which starts the pager. Meaning the outer git wrapper's stderr is _not_ redirected. Like this:

  $ cat foo
  #!/bin/sh
  exec git log -p

  $ GIT_PAGER='head -n 1' git -c alias.o='!./foo' o
  error: ./foo died of signal 13
  fatal: While expanding alias 'o': './foo': Success

The second reason is that most shells will "eat" the SIGPIPE exit status, and convert it into a high, positive error code. You can see that effect here:

  $ GIT_PAGER='head -n 1' git log -p
  $ echo $?
  141

And since we execute aliases via the shell, we end up seeing the converted exit code (141) instead of the signal death. _Unless_ we optimize out the shell call (which is why we see it by putting the command inside "./foo", which git executes directly, but not when we give the literal "cat longfile", which git will feed to the shell).

Or at least that's _one_ way to see it. Another way is to use a shell that does not do such conversion. Setting SHELL_PATH to zsh seems to do so, and I think that is how Bart ran into it (my patch is a followup to a Google+ posting he made).

I suppose. It means that our "do not run the shell if there are no meta-characters" optimization is leaky, since the exit code behaves differently depending on whether we run the shell (and depending on your exact shell). One solution would be to fix that leakiness, and if use_shell is in effect for run-command, to convert a signal death into the value that the shell would otherwise give us.

In fact, I really wonder if this code from wait_or_whine is actually correct:

  code = WTERMSIG(status);
  /*
   * This return value is chosen so that code & 0xff
   * mimics the exit code that a POSIX shell would report for
   * a program that died from this signal.
   */
  code -= 128;

If we get signal 13, we end up with -115, because "code" is signed. When the lower 8 bits are taken, and then converted into an unsigned value, we get 141: the shell value.

But do we really want to return a negative value here? Should this instead be:

  code += 128

which yields the same code when fed to exit, but internally looks like the shell version to us? So we get a consistent result whether the shell was actually used or not.

That makes more sense to me, and would mean that whether we converted the signal number or whether it was done by a subshell, it looks the same to us. Callers which care about signals (e.g., the recent changes to launch_editor to detect SIGINT) would have to be adjusted. But I think it fixes an obscure bug there. Right now launch_editor is actually checking the whether the _shell_ died from a signal, and will fail to notice when an editor invoked by the shell is killed by those signals (this would be pretty rare, though, because typically SIGINT is delivered to the shell as well as the editor).

This would also fix the code in handle_alias. It looks for a negative return code from run_command as the sign that there was an internal error running the command, and that errno would be valid. But right now a negative return can also come from signal death.

Sort of. I was worried about:

  1. Not-quite-POSIX platforms (i.e., Windows). But JSixt has said that
     is fine, because we already have a compatibility wrapper which
     masks off only the low byte.

  2. We are relying on the specifics of how a negative value is treated
     by exit(). The cast I gave above is guaranteed to work in standard
     C, but we do not know the implementation details of exit(). Still,
     I think that is being overly paranoid. Any sane implementation will
     do what we expect.

-Peff