Re: [PATCHv2 02/10] pkt-line: drop safe_write function

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> This is just write_or_die by another name.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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:

[...]
> --- 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.

[...]
> --- 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

[...]
> --- 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.

[...]
> --- 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