Re: [PATCHv2 02/10] pkt-line: drop safe_write function
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:56:10
Jeff King wrote:
This is just write_or_die by another name. Signed-off-by: Jeff King <redacted> --- Actually, they are not quite the same. write_or_die will exit(0) when it sees EPIPE.
That information definitely belongs in the commit message.
Which makes me a little nervous.
Me, too. Let's see: [...]
quoted hunk ↗ jump to hunk
--- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c@@ -908,7 +908,7 @@ static void report(struct command *commands, const char *unpack_status) if (use_sideband) send_sideband(1, 1, buf.buf, buf.len, use_sideband); else - safe_write(1, buf.buf, buf.len); + write_or_die(1, buf.buf, buf.len);
If the connection to send-pack is lost and stdout becomes a broken pipe and I am updating enough refs to overflow the pipe buffer, receive-pack will die with SIGPIPE. So unless the sadistic caller has set the inherited SIGPIPE action to SIG_IGN (for example by wrapping git with an uncautious Python wrapper that uses subprocess.Popen), the change to EPIPE handling is not a behavior change. Since the pipe is closed, presumably the calling send-pack has hung up and won't notice the exit status, so this should be safe. Arguably it would be more friendly to stay alive to run the post-receive and post-update hooks, though, given that a ref update has occurred. Maybe transport commands like this one should always set the disposition of SIGPIPE to SIG_IGN. [...]
quoted hunk ↗ jump to hunk
--- a/builtin/send-pack.c +++ b/builtin/send-pack.c@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref) } strbuf_addch(&buf, '\n'); - safe_write(1, buf.buf, buf.len); + write_or_die(1, buf.buf, buf.len);
A signal will kill send-pack before write_or_die has a chance to intervene so this change is a no-op unless the caller is sadistic (as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might be a regression, since "git push" should not declare success when its connection to receive-pack closes early. [1] http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html [...]
quoted hunk ↗ jump to hunk
--- a/fetch-pack.c +++ b/fetch-pack.c@@ -245,7 +245,7 @@ static void send_request(struct fetch_pack_args *args, send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX); packet_flush(fd); } else - safe_write(fd, buf->buf, buf->len); + write_or_die(fd, buf->buf, buf->len);
Also a no-op except when the parent process is insane enough to let us inherit signal(SIGPIPE, SIG_IGN). In that case, if triggerable this looks like a bad change: if upload-pack has gone missing, the fetch should not be considered a success. [...]
quoted hunk ↗ jump to hunk
--- a/http-backend.c +++ b/http-backend.c@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...) if (n >= sizeof(buffer)) die("protocol error: impossibly long line"); - safe_write(fd, buffer, n); + write_or_die(fd, buffer, n);
Etc. I'm stopping here.
I'm thinking before a patch like this we should make the following
change:
1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make
the behavior a little more predictable.
Perhaps the following as well:
2. in write_or_die(), when encountering EPIPE, set the signal action
of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status
reflects the broken pipe. If the parent process is unnecessarily
noisy about that, that's a bug in the parent process (hopefully
uncommon).
Or alternatively:
2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
do not call write_or_die()
What do you think?
Jonathan