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.
--
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 p...@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
--
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 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);
 
--
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 p...@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
--
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 p...@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

--
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 j...@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.

--
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 p...@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?


--
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 p...@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?

--
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 p...@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
--
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 jrnie...@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.

--
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 p...@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 

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

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

[Junio C Hamano]

Jeff King p...@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.
--
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