[RFC/PATCH] avoid SIGPIPE warnings for aliases

[RFC/PATCH] avoid SIGPIPE warnings for aliases

[Jeff King]

When git executes an alias that specifies an external
command, it will complain if the alias dies due to a signal.
This is usually a good thing, as signal deaths are
unexpected. However, SIGPIPE is not unexpected for many
commands which produce a lot of output; it is intended that
the user closing the pager would kill them them via SIGPIPE.

As a result, the user might see annoying messages in a
scenario like this:

  $ cat ~/.gitconfig
  [alias]
  lgbase = log --some-options
  lg = !git lgbase --more-options
  lg2 = !git lgbase --other-options

  $ git lg -p
  [user hits 'q' to exit pager]
  error: git lgbase --more-options died of signal 13
  fatal: While expanding alias 'lg': 'git lgbase --more-options': Success

Many users won't see this, because we execute the external
command with the shell, and a POSIX shell will silently
rewrite the signal-death exit code into 128+signal, and we
will treat it like a normal exit code. However, this does
not always happen:

  1. If the sub-command does not have shell metacharacters,
     we will skip the shell and exec it directly, getting
     the signal code.

  2. Some shells do not do this rewriting; for example,
     setting SHELL_PATH set to zsh will reveal this problem.

This patch squelches the message, and let's git exit
silently (with the same exit code that a shell would use in
case of a signal).

The first line of the message comes from run-command's
wait_or_whine. To silence that message, we remain quiet
anytime we see SIGPIPE.

The second line comes from handle_alias itself. It calls
die_errno whenever run_command returns a negative value.
However, only -1 indicates a syscall error where errno has
something useful (note that it says the useless "success"
above). Instead, we treat negative returns from run_command
(except for -1) as a normal code to be passed to exit.

Signed-off-by: Jeff King <peff@peff.net>
---
I have two reservations with this patch:

  1. We are ignoring SIGPIPE all the time. For an alias that is calling
     "log", that is fine. But if pack-objects dies on the server side,
     seeing that it died from SIGPIPE is useful data, and we are
     squelching that. Maybe callers of run-command should have to pass
     an "ignore SIGPIPE" flag?

  2. The die_errno in handle_alias is definitely wrong. Even if we want
     to print a message for signal death, showing errno is bogus unless
     the return value was -1. But is it the right thing to just pass the
     negative value straight to exit()? It works, but it is depending on
     the fact that (unsigned char)(ret & 0xff) behaves in a certain way
     (i.e., that we are on a twos-complement platform, and -13 becomes
     141). That is not strictly portable, but it is probably fine in
     practice. I'd worry more about exit() doing something weird on
     Windows.

 git.c         | 2 +-
 run-command.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index d33f9b3..07edb8a 100644
--- a/git.c
+++ b/git.c
@@ -175,7 +175,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			alias_argv[argc] = NULL;
 
 			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
-			if (ret >= 0)   /* normal exit */
+			if (ret != -1)  /* normal exit */
 				exit(ret);
 
 			die_errno("While expanding alias '%s': '%s'",
diff --git a/run-command.c b/run-command.c
index 757f263..01a4c9b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -242,7 +242,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		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
-- 
1.8.1.rc1.16.g6d46841

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

[Johannes Sixt]

Am 04.01.2013 13:47, schrieb Jeff King:
> I have two reservations with this patch:
> 
>   1. We are ignoring SIGPIPE all the time. For an alias that is calling
>      "log", that is fine. But if pack-objects dies on the server side,
>      seeing that it died from SIGPIPE is useful data, and we are
>      squelching that. Maybe callers of run-command should have to pass
>      an "ignore SIGPIPE" flag?

I am of two minds. On the one hand, losing useful debugging information
is not something we should do lightly. On the other hand, the message is
really noise most of the time, even on servers: when pack-objects dies
on the server side, it is most likely due to a connection that breaks
(voluntarily or involuntarily) half-way during a transfer, and is
presumably a frequent event, and as such not worth noting most of the time.

>   2. The die_errno in handle_alias is definitely wrong. Even if we want
>      to print a message for signal death, showing errno is bogus unless
>      the return value was -1. But is it the right thing to just pass the
>      negative value straight to exit()? It works, but it is depending on
>      the fact that (unsigned char)(ret & 0xff) behaves in a certain way
>      (i.e., that we are on a twos-complement platform, and -13 becomes
>      141). That is not strictly portable, but it is probably fine in
>      practice. I'd worry more about exit() doing something weird on
>      Windows.

It did something weird on Windows until we added this line to
compat/mingw.h:

#define exit(code) exit((code) & 0xff)

-- Hannes

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

[Jeff King]

On Fri, Jan 04, 2013 at 05:55:18PM +0100, Johannes Sixt wrote:

> Am 04.01.2013 13:47, schrieb Jeff King:
> > I have two reservations with this patch:
> > 
> >   1. We are ignoring SIGPIPE all the time. For an alias that is calling
> >      "log", that is fine. But if pack-objects dies on the server side,
> >      seeing that it died from SIGPIPE is useful data, and we are
> >      squelching that. Maybe callers of run-command should have to pass
> >      an "ignore SIGPIPE" flag?
> 
> I am of two minds. On the one hand, losing useful debugging information
> is not something we should do lightly. On the other hand, the message is
> really noise most of the time, even on servers: when pack-objects dies
> on the server side, it is most likely due to a connection that breaks
> (voluntarily or involuntarily) half-way during a transfer, and is
> presumably a frequent event, and as such not worth noting most of the time.

Yeah. I'd mostly be worried about a case where pack-objects prints
nothing (because it dies due to pipe), and then the outer process is not
sufficiently verbose (it just says something like "pack-objects died
abnormally", and the user is left scratching their head. I.e., it _is_
uninteresting, but because we are too silent, the user does not even
know it is uninteresting.

Pack-objects is already careful to check all of its writes. I really
think it would be fine to just ignore SIGPIPE, and then it would produce
a useful error message on EPIPE. The downside is that if we accidentally
have an unchecked call, we won't notice the error (we'll probably notice
it later, but we might continue uselessly spewing data in the meantime).
Perhaps we should catch SIGPIPE in such programs and print an error
message.

> >   2. The die_errno in handle_alias is definitely wrong. Even if we want
> >      to print a message for signal death, showing errno is bogus unless
> >      the return value was -1. But is it the right thing to just pass the
> >      negative value straight to exit()? It works, but it is depending on
> >      the fact that (unsigned char)(ret & 0xff) behaves in a certain way
> >      (i.e., that we are on a twos-complement platform, and -13 becomes
> >      141). That is not strictly portable, but it is probably fine in
> >      practice. I'd worry more about exit() doing something weird on
> >      Windows.
> 
> It did something weird on Windows until we added this line to
> compat/mingw.h:
> 
> #define exit(code) exit((code) & 0xff)

Ah, makes sense. I think that hunk of my patch is probably good, then.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I have two reservations with this patch:
>
>   1. We are ignoring SIGPIPE all the time. For an alias that is calling
>      "log", that is fine. But if pack-objects dies on the server side,
>      seeing that it died from SIGPIPE is useful data, and we are
>      squelching that. Maybe callers of run-command should have to pass
>      an "ignore SIGPIPE" flag?

What should this do:

    GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o

Should it behave just like

    cat longfile | head -n 1

or should it behave differently?

I am having a feeling that whatever external command given as the
value of alias.$cmd should choose what error status it wants to be
reported.

>   2. The die_errno in handle_alias is definitely wrong. Even if we want
>      to print a message for signal death, showing errno is bogus unless
>      the return value was -1. But is it the right thing to just pass the
>      negative value straight to exit()? It works, but it is depending on
>      the fact that (unsigned char)(ret & 0xff) behaves in a certain way
>      (i.e., that we are on a twos-complement platform, and -13 becomes
>      141).

Isn't that what POSIX.1 guarantees us, though?

    The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any
    other value, though only the least significant 8 bits (that is,
    status & 0377) shall be available to a waiting parent process.

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > I have two reservations with this patch:
> >
> >   1. We are ignoring SIGPIPE all the time. For an alias that is calling
> >      "log", that is fine. But if pack-objects dies on the server side,
> >      seeing that it died from SIGPIPE is useful data, and we are
> >      squelching that. Maybe callers of run-command should have to pass
> >      an "ignore SIGPIPE" flag?
> 
> What should this do:
> 
>     GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o
> 
> Should it behave just like
> 
>     cat longfile | head -n 1
> 
> or should it behave differently?

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 am having a feeling that whatever external command given as the
> value of alias.$cmd should choose what error status it wants to be
> reported.

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.

> >   2. The die_errno in handle_alias is definitely wrong. Even if we want
> >      to print a message for signal death, showing errno is bogus unless
> >      the return value was -1. But is it the right thing to just pass the
> >      negative value straight to exit()? It works, but it is depending on
> >      the fact that (unsigned char)(ret & 0xff) behaves in a certain way
> >      (i.e., that we are on a twos-complement platform, and -13 becomes
> >      141).
> 
> Isn't that what POSIX.1 guarantees us, though?
> 
>     The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any
>     other value, though only the least significant 8 bits (that is,
>     status & 0377) shall be available to a waiting parent process.

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

[PATCH] run-command: encode signal death as a positive integer

[Jeff King]

On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:

> 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;

After looking at it some more, it is correct, but I think we could make
life slightly easier for callers. See the patch below.  I've tried to
re-state the somewhat rambling argument from my previous email;
hopefully it makes sense.

-- >8 --
Subject: [PATCH] run-command: encode signal death as a positive integer

When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128". This is easy to identify (versus a regular positive
error code), and when cast to an unsigned integer (e.g., by
feeding it to exit), matches what a POSIX shell would return
when reporting a signal death in $? or through its own exit
code.

So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form). E.g., death by SIGPIPE (signal 13) will look like
-115 to us in inside git, but will end up as 141 when we
call exit() with it. And a program killed by SIGPIPE but run
via the shell will come to us with an exit code of 141.

Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms. We
might or might not have actually invoked the shell (because
we optimize out some useless shell calls). If we didn't invoke
the shell, we will will see the sub-process's signal death
directly, and run-command converts it into a negative value.
But if we did invoke the shell, we will see the shell's
128+signal exit status. To be thorough, we would need to
check both, or cast the value to an unsigned char (after
checking that it is not -1, which is a magic error value).

Fortunately, most callsites do not care at all whether the
exit was from a code or from a signal; they merely check for
a non-zero status, and sometimes propagate the error via
exit(). But for the callers that do care, we can make life
slightly easier by just using the consistent positive form.

This actually fixes two minor bugs:

  1. In launch_editor, we check whether the editor died from
     SIGINT or SIGQUIT. But we checked only the negative
     form, meaning that we would fail to notice a signal
     death exit code which was propagated through the shell.

  2. In handle_alias, we assume that a negative return value
     from run_command means that errno tells us something
     interesting (like a fork failure, or ENOENT).
     Otherwise, we simply propagate the exit code. Negative
     signal death codes confuse us, and we print a useless
     "unable to run alias 'foo': Success" message. By
     encoding signal deaths using the positive form, the
     existing code just propagates it as it would a normal
     non-zero exit code.

The downside is that callers of run_command can no longer
differentiate between a signal received directly by the
sub-process, and one propagated. However, no caller
currently cares, and since we already optimize out some
calls to the shell under the hood, that distinction is not
something that should be relied upon by callers.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-run-command.txt | 6 ++----
 editor.c                                    | 2 +-
 run-command.c                               | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index f18b4f4..5d7d7f2 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -55,10 +55,8 @@ The functions above do the following:
   non-zero.
 
 . If the program terminated due to a signal, then the return value is the
-  signal number - 128, ie. it is negative and so indicates an unusual
-  condition; a diagnostic is printed. This return value can be passed to
-  exit(2), which will report the same code to the parent process that a
-  POSIX shell's $? would report for a program that died from the signal.
+  signal number + 128, ie. the same value that a POSIX shell's $? would
+  report.  A diagnostic is printed.
 
 
 `start_async`::
diff --git a/editor.c b/editor.c
index 065a7ab..27bdecd 100644
--- a/editor.c
+++ b/editor.c
@@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
-		sig = ret + 128;
+		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
 		if (sig == SIGINT || sig == SIGQUIT)
diff --git a/run-command.c b/run-command.c
index 757f263..cfb7274 100644
--- a/run-command.c
+++ b/run-command.c
@@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		 * mimics the exit code that a POSIX shell would report for
 		 * a program that died from this signal.
 		 */
-		code -= 128;
+		code += 128;
 	} else if (WIFEXITED(status)) {
 		code = WEXITSTATUS(status);
 		/*
-- 
1.8.1.rc1.16.g6d46841

Re: [PATCH] run-command: encode signal death as a positive integer

[Johannes Sixt]

Am 05.01.2013 15:49, schrieb Jeff King:
> On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
> 
>> 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;
> 
> After looking at it some more, it is correct, but I think we could make
> life slightly easier for callers. See the patch below.  I've tried to
> re-state the somewhat rambling argument from my previous email;
> hopefully it makes sense.
> 
> -- >8 --
> Subject: [PATCH] run-command: encode signal death as a positive integer
> 
> When a sub-command dies due to a signal, we encode the
> signal number into the numeric exit status as "signal -
> 128". This is easy to identify (versus a regular positive
> error code), and when cast to an unsigned integer (e.g., by
> feeding it to exit), matches what a POSIX shell would return
> when reporting a signal death in $? or through its own exit
> code.
> 
> So we have a negative value inside the code, but once it
> passes across an exit() barrier, it looks positive (and any
> code we receive from a sub-shell will have the positive
> form). E.g., death by SIGPIPE (signal 13) will look like
> -115 to us in inside git, but will end up as 141 when we
> call exit() with it. And a program killed by SIGPIPE but run
> via the shell will come to us with an exit code of 141.
> 
> Unfortunately, this means that when the "use_shell" option
> is set, we need to be on the lookout for _both_ forms. We
> might or might not have actually invoked the shell (because
> we optimize out some useless shell calls). If we didn't invoke
> the shell, we will will see the sub-process's signal death
> directly, and run-command converts it into a negative value.
> But if we did invoke the shell, we will see the shell's
> 128+signal exit status. To be thorough, we would need to
> check both, or cast the value to an unsigned char (after
> checking that it is not -1, which is a magic error value).
> 
> Fortunately, most callsites do not care at all whether the
> exit was from a code or from a signal; they merely check for
> a non-zero status, and sometimes propagate the error via
> exit(). But for the callers that do care, we can make life
> slightly easier by just using the consistent positive form.
> 
> This actually fixes two minor bugs:
> 
>   1. In launch_editor, we check whether the editor died from
>      SIGINT or SIGQUIT. But we checked only the negative
>      form, meaning that we would fail to notice a signal
>      death exit code which was propagated through the shell.
> 
>   2. In handle_alias, we assume that a negative return value
>      from run_command means that errno tells us something
>      interesting (like a fork failure, or ENOENT).
>      Otherwise, we simply propagate the exit code. Negative
>      signal death codes confuse us, and we print a useless
>      "unable to run alias 'foo': Success" message. By
>      encoding signal deaths using the positive form, the
>      existing code just propagates it as it would a normal
>      non-zero exit code.
> 
> The downside is that callers of run_command can no longer
> differentiate between a signal received directly by the
> sub-process, and one propagated. However, no caller
> currently cares, and since we already optimize out some
> calls to the shell under the hood, that distinction is not
> something that should be relied upon by callers.

The idea was initially to regard death by signal as an internal error.
But since (1) there are no callers that are really interested in the
difference and (2) we get it wrong in the shell case anyway, this change
makes total sense.

Acked-by: Johannes Sixt <j6t@kdbg.org>

> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/technical/api-run-command.txt | 6 ++----
>  editor.c                                    | 2 +-
>  run-command.c                               | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
> index f18b4f4..5d7d7f2 100644
> --- a/Documentation/technical/api-run-command.txt
> +++ b/Documentation/technical/api-run-command.txt
> @@ -55,10 +55,8 @@ The functions above do the following:
>    non-zero.
>  
>  . If the program terminated due to a signal, then the return value is the
> -  signal number - 128, ie. it is negative and so indicates an unusual
> -  condition; a diagnostic is printed. This return value can be passed to
> -  exit(2), which will report the same code to the parent process that a
> -  POSIX shell's $? would report for a program that died from the signal.
> +  signal number + 128, ie. the same value that a POSIX shell's $? would
> +  report.  A diagnostic is printed.
>  
>  
>  `start_async`::
> diff --git a/editor.c b/editor.c
> index 065a7ab..27bdecd 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  		sigchain_push(SIGINT, SIG_IGN);
>  		sigchain_push(SIGQUIT, SIG_IGN);
>  		ret = finish_command(&p);
> -		sig = ret + 128;
> +		sig = ret - 128;
>  		sigchain_pop(SIGINT);
>  		sigchain_pop(SIGQUIT);
>  		if (sig == SIGINT || sig == SIGQUIT)
> diff --git a/run-command.c b/run-command.c
> index 757f263..cfb7274 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>  		 * mimics the exit code that a POSIX shell would report for
>  		 * a program that died from this signal.
>  		 */
> -		code -= 128;
> +		code += 128;
>  	} else if (WIFEXITED(status)) {
>  		code = WEXITSTATUS(status);
>  		/*
> 

Re: [PATCH] run-command: encode signal death as a positive integer

[Jonathan Nieder]

Hi,

Jeff King wrote:

> When a sub-command dies due to a signal, we encode the
> signal number into the numeric exit status as "signal -
> 128".
[...]
> So we have a negative value inside the code, but once it
> passes across an exit() barrier, it looks positive (and any
> code we receive from a sub-shell will have the positive
> form).
[...]
> Unfortunately, this means that when the "use_shell" option
> is set, we need to be on the lookout for _both_ forms.
[...]
>             for the callers that do care, we can make life
> slightly easier by just using the consistent positive form.

Makes perfect sense.

>  Documentation/technical/api-run-command.txt | 6 ++----
>  editor.c                                    | 2 +-
>  run-command.c                               | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)

t/test-terminal.perl imitates the same logic.  It doesn't check for
anything other than whether the exit status is 0, but maybe it would
be worth squashing in the below as a futureproofing measure
nonetheless.

Aside from the launch_editor bugfix, the only observable effects of
the above patch I can find are some changed error messages:

	error: external filter cat failed -126
	-> error: external filter cat failed 130

	warning: svnrdump, returned -126
	-> warning: svnrdump, returned 130

Those messages are equally senseless before and after the patch, so
for what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git i/t/test-terminal.perl w/t/test-terminal.perl
index 10172aee..1fb373f2 100755
--- i/t/test-terminal.perl
+++ w/t/test-terminal.perl
@@ -31,7 +31,7 @@ sub finish_child {
 	} elsif ($? & 127) {
 		my $code = $? & 127;
 		warn "died of signal $code";
-		return $code - 128;
+		return $code + 128;
 	} else {
 		return $? >> 8;
 	}

Re: [PATCH] run-command: encode signal death as a positive integer

[Jeff King]

On Sat, Jan 05, 2013 at 02:19:09PM -0800, Jonathan Nieder wrote:

> >  Documentation/technical/api-run-command.txt | 6 ++----
> >  editor.c                                    | 2 +-
> >  run-command.c                               | 2 +-
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> t/test-terminal.perl imitates the same logic.  It doesn't check for
> anything other than whether the exit status is 0, but maybe it would
> be worth squashing in the below as a futureproofing measure
> nonetheless.

Yeah, I think so. As you say, it does not matter, but it makes sense to
keep our conventions consistent.

> Aside from the launch_editor bugfix, the only observable effects of
> the above patch I can find are some changed error messages:
> 
> 	error: external filter cat failed -126
> 	-> error: external filter cat failed 130
> 
> 	warning: svnrdump, returned -126
> 	-> warning: svnrdump, returned 130
> 
> Those messages are equally senseless before and after the patch, so
> for what it's worth,

Thanks. I agree that change isn't a big deal (I would argue the positive
return is slightly more coherent as it matches what the shell would
report, but I really think user-facing errors should probably not even
mention the exact exit code, as it is just noise most of the time, and
we already complain about signals in wait_or_whine).

I did try auditing the callers of finish_command (and run_command) to
make sure I wasn't regressing anybody, but there are a lot of call
sites. In some cases we immediately say:

  if (finish_command(&child))
          die("failed...");

which is obviously unaffected. But in many cases we pass the exit code
up through several levels. It usually just ends up in exit() or being
collapsed to an error boolean, which is fine, but I may have missed a
spot where it matters. I'd expecting cooking this patch for a while
would flush out any I missed.

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks (and to J6t) for the review.

-Peff

Re: [PATCH] run-command: encode signal death as a positive integer

[Jonathan Nieder]

Jeff King wrote:

>                        I'd expecting cooking this patch for a while
> would flush out any I missed.

Heh, probably not. ;-)  But I tried to examine all the callsites (and
only found the two messages I mentioned), and among the reviewers, I'm
guessing we hit them all.

Ciao,
Jonathan

Re: [PATCH] run-command: encode signal death as a positive integer

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
> ...
> The downside is that callers of run_command can no longer
> differentiate between a signal received directly by the
> sub-process, and one propagated. However, no caller
> currently cares, and since we already optimize out some
> calls to the shell under the hood, that distinction is not
> something that should be relied upon by callers.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Very nicely explained.  Thanks.

>  Documentation/technical/api-run-command.txt | 6 ++----
>  editor.c                                    | 2 +-
>  run-command.c                               | 2 +-
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
> index f18b4f4..5d7d7f2 100644
> --- a/Documentation/technical/api-run-command.txt
> +++ b/Documentation/technical/api-run-command.txt
> @@ -55,10 +55,8 @@ The functions above do the following:
>    non-zero.
>  
>  . If the program terminated due to a signal, then the return value is the
> -  signal number - 128, ie. it is negative and so indicates an unusual
> -  condition; a diagnostic is printed. This return value can be passed to
> -  exit(2), which will report the same code to the parent process that a
> -  POSIX shell's $? would report for a program that died from the signal.
> +  signal number + 128, ie. the same value that a POSIX shell's $? would
> +  report.  A diagnostic is printed.
>  
>  
>  `start_async`::
> diff --git a/editor.c b/editor.c
> index 065a7ab..27bdecd 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  		sigchain_push(SIGINT, SIG_IGN);
>  		sigchain_push(SIGQUIT, SIG_IGN);
>  		ret = finish_command(&p);
> -		sig = ret + 128;
> +		sig = ret - 128;
>  		sigchain_pop(SIGINT);
>  		sigchain_pop(SIGQUIT);
>  		if (sig == SIGINT || sig == SIGQUIT)
> diff --git a/run-command.c b/run-command.c
> index 757f263..cfb7274 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>  		 * mimics the exit code that a POSIX shell would report for
>  		 * a program that died from this signal.
>  		 */
> -		code -= 128;
> +		code += 128;
>  	} else if (WIFEXITED(status)) {
>  		code = WEXITSTATUS(status);
>  		/*

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When git executes an alias that specifies an external
> command, it will complain if the alias dies due to a signal.
> This is usually a good thing, as signal deaths are
> unexpected. However, SIGPIPE is not unexpected for many
> commands which produce a lot of output; it is intended that
> the user closing the pager would kill them them via SIGPIPE.
>
> As a result, the user might see annoying messages in a
> scenario like this:
>
>   $ cat ~/.gitconfig
>   [alias]
>   lgbase = log --some-options
>   lg = !git lgbase --more-options
>   lg2 = !git lgbase --other-options
>
>   $ git lg -p
>   [user hits 'q' to exit pager]
>   error: git lgbase --more-options died of signal 13
>   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success
>
> Many users won't see this, because we execute the external
> command with the shell, and a POSIX shell will silently
> rewrite the signal-death exit code into 128+signal, and we
> will treat it like a normal exit code. However, this does
> not always happen:

So... with the "flip the sign of the exit code when caught a signal"
patch applied to 'next', do people still see this issue?

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

[Jeff King]

On Wed, Jan 09, 2013 at 12:48:20PM -0800, Junio C Hamano wrote:

> >   $ git lg -p
> >   [user hits 'q' to exit pager]
> >   error: git lgbase --more-options died of signal 13
> >   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success
> >
> > Many users won't see this, because we execute the external
> > command with the shell, and a POSIX shell will silently
> > rewrite the signal-death exit code into 128+signal, and we
> > will treat it like a normal exit code. However, this does
> > not always happen:
> 
> So... with the "flip the sign of the exit code when caught a signal"
> patch applied to 'next', do people still see this issue?

They see half. The patch you've applied clears up the "While
expanding...: Success" message.

But we still say "error: ... died of signal 13", because that comes from
inside wait_or_whine. So it is a separate issue whether or not
wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and
SIGQUIT, as of some recent patches).

The upside is that it is noise in this case that we would no longer see.
The downside is that we may be losing a clue when debugging server
problems, which do not expect to die from SIGPIPE.  Should it be an
optional run-command flag?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But we still say "error: ... died of signal 13", because that comes from
> inside wait_or_whine. So it is a separate issue whether or not
> wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and
> SIGQUIT, as of some recent patches).
>
> The upside is that it is noise in this case that we would no longer see.
> The downside is that we may be losing a clue when debugging server
> problems, which do not expect to die from SIGPIPE.  Should it be an
> optional run-command flag?

Do we know if we are upstream of a pager that reads from us through
a pipe (I think we should, especially in a case where we are the one
who processed the "git -p $alias" option)?  Is there any other case
where we would want to ignore child's death by SIGPIPE?  If the
answers are yes and no, then perhaps we can ask pager_in_use() to
decide this?

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:

>> But we still say "error: ... died of signal 13", because that comes from
>> inside wait_or_whine. So it is a separate issue whether or not
>> wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and
>> SIGQUIT, as of some recent patches).
>>
>> The upside is that it is noise in this case that we would no longer see.
>> The downside is that we may be losing a clue when debugging server
>> problems, which do not expect to die from SIGPIPE.  Should it be an
>> optional run-command flag?
>
> Do we know if we are upstream of a pager that reads from us through
> a pipe (I think we should, especially in a case where we are the one
> who processed the "git -p $alias" option)?  Is there any other case
> where we would want to ignore child's death by SIGPIPE?

When we die early by SIGPIPE because output was piped to "head", I
still think the early end of output is not notable enough to complain
about.

I'm not sure whether there are SIGPIPE instances we really don't want
to be silent about, though.  I suspect not. ;-)

Compare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,
<http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I'm not sure whether there are SIGPIPE instances we really don't want
> to be silent about, though.  I suspect not. ;-)
>
> Compare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,
> <http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.

Yeah, thanks for the pointer to 48665.  Quoting from there:

    So EPIPE really _is_ special: because when you write to a pipe,
    there's no guarantee that you'll get it at all, so whenever you get
    an EPIPE you should ask yourself:

     - what would I have done if the data had fit in the 64kB kernel
       buffer?

     - should I really return a different error message or complain just 
       because I just happened to notice that the reader went away
       _this_ 
       time, even if I might not notice it next time?

    In other words, the "exit(0)" is actually _more_ consistent than
    "exit(1)", because exiting with an error message or with an error
    return is going to depend on luck and timing.

and I think I still agree with the analysis and conclusion:

    So what _should_ you do for EPIPE?

    Here's what EPIPE _really_ means:

     - you might as well consider the write a success, but the
       reader isn't actually interested, so rather than go on, you
       might as well stop early.

    Notice how I very carefull avoided the word "error" anywhere.
    Because it's really not an error. The reader already got
    everything it wanted. So EPIPE should generally be seen as an
    "early success" rather than as a "failure".

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

[Jeff King]

On Wed, Jan 09, 2013 at 04:18:44PM -0800, Jonathan Nieder wrote:

> > Do we know if we are upstream of a pager that reads from us through
> > a pipe (I think we should, especially in a case where we are the one
> > who processed the "git -p $alias" option)?  Is there any other case
> > where we would want to ignore child's death by SIGPIPE?
> 
> When we die early by SIGPIPE because output was piped to "head", I
> still think the early end of output is not notable enough to complain
> about.
> 
> I'm not sure whether there are SIGPIPE instances we really don't want
> to be silent about, though.  I suspect not. ;-)

Some of our plumbing writes over pipes (e.g., pack-objects writing back
to send-pack, which is multiplexing over the network, or receive-pack
writing to index-pack). We _should_ be checking the value of every
write(), and your final close(), and making sure that sub-processes
reports success. But leaving SIGPIPE on is an extra safety measure; in
theory it can catch an unchecked write.

When one of those programs goes wrong, the message can be an extra
debugging aid. If the process died unexpectedly with no message (since
it died by signal), seeing "pack-objects died by signal 13" is much
better than not seeing anything at all. Usually it is accompanied by
other messages (like "remote end hung up unexpectedly" or similar), but
the extra output has helped me track down server-side issues in the
past.

> Compare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,
> <http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.

The argument there seems to be that bash is stupid for complaining about
SIGPIPE. And I would agree for the "alias" case, where we are running
commands from the command-line in a very shell-like manner. But
wait_or_whine is also used for connecting internal bits together.

Maybe the right rule is "if we are using the shell to execute, do not
mention SIGPIPE"? It seems a little iffy at first, but:

  1. It tends to coincide with direct use of internal tools versus
     external tools.

  2. We do not reliably get SIGPIPE there, anyway, since most shells
     will convert it into exit code 141 before we see it.

I.e., something like:

diff --git a/run-command.c b/run-command.c
index 24eaad5..8bd0b08 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0)
+static int wait_or_whine(pid_t pid, const char *argv0, int ignore_sigpipe)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		if (code != SIGINT && code != SIGQUIT)
+		if (code != SIGINT && code != SIGQUIT &&
+		    (!ignore_sigpipe || code != SIGPIPE))
 			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
@@ -433,7 +434,7 @@ fail_pipe:
 		 * At this point we know that fork() succeeded, but execvp()
 		 * failed. Errors have been reported to our stderr.
 		 */
-		wait_or_whine(cmd->pid, cmd->argv[0]);
+		wait_or_whine(cmd->pid, cmd->argv[0], 0);
 		failed_errno = errno;
 		cmd->pid = -1;
 	}
@@ -538,7 +539,7 @@ int finish_command(struct child_process *cmd)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0]);
+	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->use_shell);
 }
 
 int run_command(struct child_process *cmd)
@@ -725,7 +726,7 @@ int finish_async(struct async *async)
 int finish_async(struct async *async)
 {
 #ifdef NO_PTHREADS
-	return wait_or_whine(async->pid, "child process");
+	return wait_or_whine(async->pid, "child process", 0);
 #else
 	void *ret = (void *)(intptr_t)(-1);
 

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Maybe the right rule is "if we are using the shell to execute, do not
> mention SIGPIPE"? It seems a little iffy at first, but:
>
>   1. It tends to coincide with direct use of internal tools versus
>      external tools.
>
>   2. We do not reliably get SIGPIPE there, anyway, since most shells
>      will convert it into exit code 141 before we see it.
>
> I.e., something like:

Hmph.  That may be a good heuristics, but I wonder if we also want
to special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to
pretend as if nothing went wrong, when ignore_sigpipe is in effect?

> diff --git a/run-command.c b/run-command.c
> index 24eaad5..8bd0b08 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)
>  		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>  }
>  
> -static int wait_or_whine(pid_t pid, const char *argv0)
> +static int wait_or_whine(pid_t pid, const char *argv0, int ignore_sigpipe)
>  {
>  	int status, code = -1;
>  	pid_t waiting;
> @@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>  		error("waitpid is confused (%s)", argv0);
>  	} else if (WIFSIGNALED(status)) {
>  		code = WTERMSIG(status);
> -		if (code != SIGINT && code != SIGQUIT)
> +		if (code != SIGINT && code != SIGQUIT &&
> +		    (!ignore_sigpipe || code != SIGPIPE))
>  			error("%s died of signal %d", argv0, code);
>  		/*
>  		 * This return value is chosen so that code & 0xff
> @@ -433,7 +434,7 @@ fail_pipe:
>  		 * At this point we know that fork() succeeded, but execvp()
>  		 * failed. Errors have been reported to our stderr.
>  		 */
> -		wait_or_whine(cmd->pid, cmd->argv[0]);
> +		wait_or_whine(cmd->pid, cmd->argv[0], 0);
>  		failed_errno = errno;
>  		cmd->pid = -1;
>  	}
> @@ -538,7 +539,7 @@ int finish_command(struct child_process *cmd)
>  
>  int finish_command(struct child_process *cmd)
>  {
> -	return wait_or_whine(cmd->pid, cmd->argv[0]);
> +	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->use_shell);
>  }
>  
>  int run_command(struct child_process *cmd)
> @@ -725,7 +726,7 @@ int finish_async(struct async *async)
>  int finish_async(struct async *async)
>  {
>  #ifdef NO_PTHREADS
> -	return wait_or_whine(async->pid, "child process");
> +	return wait_or_whine(async->pid, "child process", 0);
>  #else
>  	void *ret = (void *)(intptr_t)(-1);
>  

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

[Jeff King]

On Thu, Jan 10, 2013 at 12:22:49PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Maybe the right rule is "if we are using the shell to execute, do not
> > mention SIGPIPE"? It seems a little iffy at first, but:
> >
> >   1. It tends to coincide with direct use of internal tools versus
> >      external tools.
> >
> >   2. We do not reliably get SIGPIPE there, anyway, since most shells
> >      will convert it into exit code 141 before we see it.
> >
> > I.e., something like:
> 
> Hmph.  That may be a good heuristics, but I wonder if we also want
> to special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to
> pretend as if nothing went wrong, when ignore_sigpipe is in effect?

We could, but I don't see much point. There is very little to gain (because
nobody is complaining about the exit code, only the message), and we
might possibly fail to propagate an error condition (unlikely, but more
serious consequences). To be honest, I am having doubts about touching
it at all. I had to really work to provoke the error without setting
SHELL_PATH=zsh, so I am worried that we are getting worked up over
something that just doesn't happen in practice. And I am not sure that
setting SHELL_PATH=zsh is a sane thing (I only knew about it because
Bart mentioned it he was using zsh).

Bart, do you actually set up SHELL_PATH like that? Is /bin/sh on your
system zsh? If the latter, I wonder if this is actually a bug in zsh.

-Peff

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

[Johannes Sixt]

Am 10.01.2013 21:22, schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> 
>> Maybe the right rule is "if we are using the shell to execute, do not
>> mention SIGPIPE"? It seems a little iffy at first, but:
>>
>>   1. It tends to coincide with direct use of internal tools versus
>>      external tools.
>>
>>   2. We do not reliably get SIGPIPE there, anyway, since most shells
>>      will convert it into exit code 141 before we see it.
>>
>> I.e., something like:
> 
> Hmph.  That may be a good heuristics, but I wonder if we also want
> to special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to
> pretend as if nothing went wrong, when ignore_sigpipe is in effect?

The purpose of Peff's patch is to remove the error message, but not to
pretend success (the return code remains 141).

I looked at all instances with use_shell=1 or RUN_USING_SHELL:

Most of the time, we do not care where the output of the command goes
to, which I regard as the same case as when a shell runs a command: We
don't need to report the SIGPIPE death.

The interesting cases are when git reads back the output of the command.
Here, a SIGPIPE death of the child would indicate a bug in git, I think,
and some diagnostic would be worth it. But we can just as well declare
that git doesn't have bugs ;)

These are the interesting cases:
connect.c:640:          conn->use_shell = 1;
    a connection to a local repository
convert.c:372:  child_process.use_shell = 1;
    clean/smudge filter
credential.c:216:       helper.use_shell = 1;
    credential helper
diff.c:4851:    child.use_shell = 1;
    textconv

All in all, I think the heuristics makes sense.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> The interesting cases are when git reads back the output of the command.
> Here, a SIGPIPE death of the child would indicate a bug in git, I think,
> and some diagnostic would be worth it. But we can just as well declare
> that git doesn't have bugs ;)
>
> These are the interesting cases:
> connect.c:640:          conn->use_shell = 1;
>     a connection to a local repository
> convert.c:372:  child_process.use_shell = 1;
>     clean/smudge filter
> credential.c:216:       helper.use_shell = 1;
>     credential helper
> diff.c:4851:    child.use_shell = 1;
>     textconv
>
> All in all, I think the heuristics makes sense.

Fair enough.  Thanks for grepping.

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

[Jeff King]

On Wed, Jan 09, 2013 at 01:49:41PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But we still say "error: ... died of signal 13", because that comes from
> > inside wait_or_whine. So it is a separate issue whether or not
> > wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and
> > SIGQUIT, as of some recent patches).
> >
> > The upside is that it is noise in this case that we would no longer see.
> > The downside is that we may be losing a clue when debugging server
> > problems, which do not expect to die from SIGPIPE.  Should it be an
> > optional run-command flag?
> 
> Do we know if we are upstream of a pager that reads from us through
> a pipe (I think we should, especially in a case where we are the one
> who processed the "git -p $alias" option)?  Is there any other case
> where we would want to ignore child's death by SIGPIPE?  If the
> answers are yes and no, then perhaps we can ask pager_in_use() to
> decide this?

The answer to the first is unfortunately no. Consider an alias like
"[alias]foo = !git log" (which yes, you could implement as just "log",
but there are cases where you want to manipulate the environment and we
do not allow it).

Your process tree for running "git foo" looks like:

  git foo               (A)
    git log             (B)
      less              (C)

The user hits 'q', which kills process C. Process B then dies due to
SIGPIPE, and process A sees that the alias command died due to a signal.
But process A has no clue that a pager is in effect; only process B,
which spawned the pager, can know that. So A cannot see death-by-SIGPIPE
and make a decision on whether a pager was in use.

If anything, it is process B's responsibility to say "Oops, I was killed
by SIGPIPE. But that's OK, it's not a big deal to me". Which it could do
by installing a SIGPIPE handler that just calls exit(0).

-Peff

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

[mimimimi]

I set up a git alias like this:

git config --global alias.popmerge '!git stash pop && git merge master'

Then I call it, like this:

git popmerge

The "git stash pop" is executed, but the "git merge master" is ignored.

If I run "git merge master" right after the "git popmerge"... it sumply runs
as expected, performing the merge.

I have other aliases with long sequences of commands... and they run
flawlessly. It seems something at "git stash pop" makes the alias process to
halt... Is it possible to avoid this behavior? How?

Thanks.
code 128
<http://www.keepdynamic.com/barcoding/asp-net-barcode-generator.shtml>  







--
View this message in context: http://git.661346.n2.nabble.com/RFC-PATCH-avoid-SIGPIPE-warnings-for-aliases-tp7574160p7615524.html
Sent from the git mailing list archive at Nabble.com.