From: Johannes Sixt <j6t@kdbg.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, Jonathan Nieder <jrnieder@gmail.com>,
git@vger.kernel.org, Bart Trojanowski <bart@jukie.net>
Subject: Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases
Date: Thu, 10 Jan 2013 22:52:10 +0100 [thread overview]
Message-ID: <50EF380A.9010809@kdbg.org> (raw)
In-Reply-To: <7vbocw23fq.fsf@alter.siamese.dyndns.org>
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
next prev parent reply other threads:[~2013-01-10 21:52 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-04 12:47 [RFC/PATCH] avoid SIGPIPE warnings for aliases Jeff King
2013-01-04 16:55 ` Johannes Sixt
2013-01-04 21:25 ` Jeff King
2013-01-04 22:20 ` Junio C Hamano
2013-01-05 14:03 ` Jeff King
2013-01-05 14:49 ` [PATCH] run-command: encode signal death as a positive integer Jeff King
2013-01-05 19:50 ` Johannes Sixt
2013-01-05 22:19 ` Jonathan Nieder
2013-01-05 23:12 ` Jeff King
2013-01-05 23:58 ` Jonathan Nieder
2013-01-06 7:05 ` Junio C Hamano
2013-01-09 20:48 ` [RFC/PATCH] avoid SIGPIPE warnings for aliases Junio C Hamano
2013-01-09 20:51 ` Jeff King
2013-01-09 21:49 ` Junio C Hamano
2013-01-10 0:18 ` Jonathan Nieder
2013-01-10 0:39 ` Junio C Hamano
2013-01-10 11:26 ` Jeff King
2013-01-10 20:22 ` Junio C Hamano
2013-01-10 21:39 ` Jeff King
2013-01-10 21:52 ` Johannes Sixt [this message]
2013-01-10 22:51 ` Junio C Hamano
2013-01-10 10:49 ` Jeff King
2014-07-21 6:45 ` mimimimi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50EF380A.9010809@kdbg.org \
--to=j6t@kdbg.org \
--cc=bart@jukie.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.