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
  







--
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Sixt  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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Sixt]

Am 10.01.2013 21:22, schrieb Junio C Hamano:
> Jeff King  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  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);
>  
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 ,
> .

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);
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jonathan Nieder  writes:

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

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".

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jeff King  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 ,
.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  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?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  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?


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[Junio C Hamano]

Jeff King  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html