[RFC PATCH] receive-pack: run post-receive before reporting status

[RFC PATCH] receive-pack: run post-receive before reporting status

[Robin Jarry]

When a remote client exits while the pre-receive hook is running,
receive-pack is not killed by SIGPIPE because the signal is ignored.
This is a side effect of commit ec7dbd145bd8 ("receive-pack: allow hooks
to ignore its standard input stream").

The pre-receive hook is not interrupted and does not receive any error
since its stdout is a pipe which is read in an async thread and output
back to the client socket in a side band channel. When writing the data
in the socket, the async thread gets a SIGPIPE which also seems ignored.
This may be a race between the main and the async threads. I do not know
the code well enough to be sure.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

Before running the post-receive hook, status info is reported back to
the client. Since the client has died, receive-pack is killed by SIGPIPE
and post-receive is never executed.

The post-receive hook is often used to send email notifications (see
contrib/hooks/post-receive-email), update bug trackers, start automatic
builds, etc. Not executing it after an interrupted yet "successful" push
can lead to inconsistencies.

Execute the post-receive hook before reporting status to the client to
avoid this issue. This is not an ideal solution but I don't know if
allowing hooks to be killed when a client exits is a good idea. Maybe
for pre-receive but definitely not for post-receive.

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 builtin/receive-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d96052..df8bedf71319 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2564,14 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si,
 				 &push_options);
+		run_receive_hook(commands, "post-receive", 1,
+				 &push_options);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status_v2)
 			report_v2(commands, unpack_status);
 		else if (report_status)
 			report(commands, unpack_status);
-		run_receive_hook(commands, "post-receive", 1,
-				 &push_options);
 		run_update_post_hook(commands);
 		string_list_clear(&push_options, 0);
 		if (auto_gc) {
-- 
2.30.2

Re: [RFC PATCH] receive-pack: run post-receive before reporting status

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 04 2021, Robin Jarry wrote:

> When a remote client exits while the pre-receive hook is running,
> receive-pack is not killed by SIGPIPE because the signal is ignored.
> This is a side effect of commit ec7dbd145bd8 ("receive-pack: allow hooks
> to ignore its standard input stream").

FWIW we include the date when mentioning commits. E.g. ec7dbd145bd
(receive-pack: allow hooks to ignore its standard input stream,
2014-09-12).

> The pre-receive hook is not interrupted and does not receive any error
> since its stdout is a pipe which is read in an async thread and output
> back to the client socket in a side band channel. When writing the data
> in the socket, the async thread gets a SIGPIPE which also seems ignored.
> This may be a race between the main and the async threads. I do not know
> the code well enough to be sure.
>
> After the pre-receive has exited the SIGPIPE default handler is restored
> and if the hook did not report any error, objects are migrated from
> temporary to permanent storage.
>
> Before running the post-receive hook, status info is reported back to
> the client. Since the client has died, receive-pack is killed by SIGPIPE
> and post-receive is never executed.
>
> The post-receive hook is often used to send email notifications (see
> contrib/hooks/post-receive-email), update bug trackers, start automatic
> builds, etc. Not executing it after an interrupted yet "successful" push
> can lead to inconsistencies.
>
> Execute the post-receive hook before reporting status to the client to
> avoid this issue. This is not an ideal solution but I don't know if
> allowing hooks to be killed when a client exits is a good idea. Maybe
> for pre-receive but definitely not for post-receive.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  builtin/receive-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 49b846d96052..df8bedf71319 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2564,14 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		use_keepalive = KEEPALIVE_ALWAYS;
>  		execute_commands(commands, unpack_status, &si,
>  				 &push_options);
> +		run_receive_hook(commands, "post-receive", 1,
> +				 &push_options);
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);
>  		if (report_status_v2)
>  			report_v2(commands, unpack_status);
>  		else if (report_status)
>  			report(commands, unpack_status);
> -		run_receive_hook(commands, "post-receive", 1,
> -				 &push_options);
>  		run_update_post_hook(commands);
>  		string_list_clear(&push_options, 0);
>  		if (auto_gc) {

I think the discussion at [1] is current to everything you're seeing
here.

tl;dr: Even with this change you're not guaranteed to run your hook.

Personally I've implemented something (independent of) what Junio
suggested downthread[2] of that. I.e. to simply insert a DB record on
pre-receive/post-receive, and have all "real" work done async by a job
that's following that.

I used MySQL as a dumb queue, but this can also be done with a text
file. I'd end up with 3x records:

 A. pre-receive: what the client wants to push
 B. pre-receive (at the very end): what we accepted the client pushing (after running all checks)
 C. post-receive: logging the same rev range, hopefully

As you've found you won't always get a "C", so such a following job
currently needs to repair such records after the fact, i.e. be ready to
inspect the repo and see if the push actually happened. You also won't
get "C" if you OK'd a push, but had a race for the *.lock file, or other
similar push contention.

I think one objection some might have to this is that we'd like to not
wait for the post-receive hook, which I falsely recalled would be
impacted by this, but as Jeff King points out at [3] we'd do the same
either way, so this change won't impact that either way.

But I think one thing that will be negatively impacted is touched upon
by your:

    "Since the client has died[...]Not executing it after an interrupted
    yet "successful" push can lead to inconsistencies".

You don't know if it died, you just got killed by SIGPIPE. That can
happen for any number of reasons, the client might have gotten its data,
you just can't reach it anymore.

I think you're right that it's inconsistent, but wrong about this
"fixing" the inconsistency. I.e. the inconsistency is just being moved
from the server-side to the client-side.

I'd think that in this case we'd very much want to give the client the
benefit of the doubt, because the server can more easily work around
issues with its hook workflow.

But a client inherently can't work around not getting an "OK & flush"
while waiting for the hook to execute, and in the meantime the cat
unplugged the WiFi, so we won't be getting the "OK" at all.

I.e. if put a "sleep 30" in a post-receive hook, push, and in the middle
of that sleep have the client disconnect from the network the push will
have gone through.

But aren't we changing what gets shown to the client from being a
successful push to a non-successful one, since they never got the
report() (and therefore flush) they were expecting? *Goes and tests*

Yes, e.g. with this:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d9605..fc273e7dc4d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2567,9 +2567,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
                if (pack_lockfile)
                        unlink_or_warn(pack_lockfile);
                if (report_status_v2)
-                       report_v2(commands, unpack_status);
+                       exit(0);
                else if (report_status)
-                       report(commands, unpack_status);
+                       exit(0);
                run_receive_hook(commands, "post-receive", 1,
                                 &push_options);
                run_update_post_hook(commands);

I've made an attempt to emulate that, and running that we'll get various
test suite failures with e.g.:
    
    + git push dest HEAD
    Enumerating objects: 4, done.
    Counting objects: 100% (4/4), done.
    Delta compression using up to 8 threads
    Compressing objects: 100% (3/3), done.
    Writing objects: 100% (4/4), 1.25 KiB | 1.25 MiB/s, done.
    Total 4 (delta 0), reused 0 (delta 0), pack-reused 0
    send-pack: unexpected disconnect while reading sideband packet
    fatal: the remote end hung up unexpectedly
    error: last command exited with $?=128

Which is a race we'll definitely see now, but would increase in
frequency if we wait longer in sending the OK.

But as noted in [1] there's a way forward to having our cake & eating it
too. I.e. when we into that on the server-side we can try a little
harder not to die, and run post-receive anyway, and in either case it
would be nice if we'd run it after disconnecting from the client, so it
doesn't have to wait for it.

1. https://lore.kernel.org/git/5795EB1C.1080102@nokia.com/
2. https://lore.kernel.org/git/xmqqlh0d8w6v.fsf@gitster.mtv.corp.google.com/
3. https://lore.kernel.org/git/20160803193018.ydhmxntikhyowmjz@sigill.intra.peff.net/

Re: [RFC PATCH] receive-pack: run post-receive before reporting status

[Robin Jarry]

Hi Ævar,

Ævar Arnfjörð Bjarmason, Nov 06, 2021 at 06:03:
> FWIW we include the date when mentioning commits. E.g. ec7dbd145bd
> (receive-pack: allow hooks to ignore its standard input stream,
> 2014-09-12).

OK, noted.

> I think the discussion at [1] is current to everything you're seeing
> here.

Thanks I had missed this thread.

> tl;dr: Even with this change you're not guaranteed to run your hook.
[snip]
> But as noted in [1] there's a way forward to having our cake & eating it
> too. I.e. when we into that on the server-side we can try a little
> harder not to die, and run post-receive anyway, and in either case it
> would be nice if we'd run it after disconnecting from the client, so it
> doesn't have to wait for it.

OK, I will try to propose a v2 masking SIGPIPE when reporting status to
the client to avoid dying before running post-receive.

Cheers,

-- 
Robin

[PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client

[Robin Jarry]

When a remote client exits while the pre-receive hook is running,
receive-pack is not killed by SIGPIPE because the signal is ignored.
This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks
to ignore its standard input stream, 2014-09-12).

The pre-receive hook itself is not interrupted and does not receive any
error since its stdout is a pipe which is read in an async thread and
output back to the client socket in a side band channel.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

Before running the post-receive hook, status info is reported back to
the client. Since the client has died, receive-pack is killed by SIGPIPE
and post-receive is never executed.

The post-receive hook is often used to send email notifications (see
contrib/hooks/post-receive-email), update bug trackers, start automatic
builds, etc. Not executing it after an interrupted yet "successful" push
can lead to inconsistencies.

Ignore SIGPIPE before reporting status to the client to increase the
chances of post-receive running if pre-receive was successful. This does
not guarantee 100% consistency but it should resist early disconnection
by the client.

Signed-off-by: Robin Jarry <robin@jarry.cc>
---
Ideally, pre-receive should not be allowed to succeed if the client has
disconnected before objects have been migrated from temporary to
permanent storage. But that is another topic, and I think it would
complement this patch.

 builtin/receive-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d96052..5fe7992028d4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2564,12 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si,
 				 &push_options);
+		sigchain_push(SIGPIPE, SIG_IGN);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status_v2)
 			report_v2(commands, unpack_status);
 		else if (report_status)
 			report(commands, unpack_status);
+		sigchain_pop(SIGPIPE);
 		run_receive_hook(commands, "post-receive", 1,
 				 &push_options);
 		run_update_post_hook(commands);
-- 
2.30.2

Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client

[Junio C Hamano]

Robin Jarry <robin@jarry.cc> writes:

> When a remote client exits while the pre-receive hook is running,
> receive-pack is not killed by SIGPIPE because the signal is ignored.
> This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks
> to ignore its standard input stream, 2014-09-12).
>
> The pre-receive hook itself is not interrupted and does not receive any
> error since its stdout is a pipe which is read in an async thread and
> output back to the client socket in a side band channel.
>
> After the pre-receive has exited the SIGPIPE default handler is restored
> and if the hook did not report any error, objects are migrated from
> temporary to permanent storage.

All of the above talks about the pre-receive hook, but it is unclear
how that is relevant to this change.  The first paragraph says
"... is not killed", and if that was a bad thing (in other words, it
should be killed but is not, and that is a bug worth fixing), and if
this patch changes the behaviour, then that paragraph is worth
saying.  Similarly for the other two.

> Before running the post-receive hook, status info is reported back to
> the client. Since the client has died, receive-pack is killed by SIGPIPE
> and post-receive is never executed.

In other words, regardless of what happens (or does not happen) to
the pre-receive hook, which may not even exist, if "git push" dies
before the post-receive hook runs, this paragraph applies, no?  

What I am getting at is that this can (and should) be the first
paragraph of the description without losing clarity.

> Ignore SIGPIPE before reporting status to the client to increase the
> chances of post-receive running if pre-receive was successful. This does
> not guarantee 100% consistency but it should resist early disconnection
> by the client.

OK.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 49b846d96052..5fe7992028d4 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2564,12 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		use_keepalive = KEEPALIVE_ALWAYS;
>  		execute_commands(commands, unpack_status, &si,
>  				 &push_options);
> +		sigchain_push(SIGPIPE, SIG_IGN);
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);

Shouldn't we start ignoring SIGPIPE here, not before we try to
unlink the lockfile?

>  		if (report_status_v2)
>  			report_v2(commands, unpack_status);
>  		else if (report_status)
>  			report(commands, unpack_status);
> +		sigchain_pop(SIGPIPE);

In other words, push/pop pair should surround the part that reports
the status, as the proposed commit log message said.

>  		run_receive_hook(commands, "post-receive", 1,
>  				 &push_options);
>  		run_update_post_hook(commands);

Other than these, looks good to me.

Thanks.

Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client

[Robin Jarry]

Hi Junio,

Junio C Hamano, Nov 09, 2021 at 22:10:
> All of the above talks about the pre-receive hook, but it is unclear
> how that is relevant to this change.  The first paragraph says
> "... is not killed", and if that was a bad thing (in other words, it
> should be killed but is not, and that is a bug worth fixing), and if
> this patch changes the behaviour, then that paragraph is worth
> saying.  Similarly for the other two.
>
> > Before running the post-receive hook, status info is reported back to
> > the client. Since the client has died, receive-pack is killed by SIGPIPE
> > and post-receive is never executed.
>
> In other words, regardless of what happens (or does not happen) to
> the pre-receive hook, which may not even exist, if "git push" dies
> before the post-receive hook runs, this paragraph applies, no?  
>
> What I am getting at is that this can (and should) be the first
> paragraph of the description without losing clarity.

You're right. I wanted to give context to better explain why
receive-pack is not killed while running the pre-receive hook but
afterwards. This should go into another commit which fixes that issue.

I will reword accordingly.

> >  		execute_commands(commands, unpack_status, &si,
> >  				 &push_options);
> > +		sigchain_push(SIGPIPE, SIG_IGN);
> >  		if (pack_lockfile)
> >  			unlink_or_warn(pack_lockfile);
>
> Shouldn't we start ignoring SIGPIPE here, not before we try to
> unlink the lockfile?

I initially wanted to avoid getting SIGPIPE'd while printing a warning
if the lockfile cannot be unlinked. Maybe this means the repository
integrity is compromised and we are well beyond ensuring post-receive is
executed or not. I do not know git internals well enough to be sure.

What do you think?

-- 
Robin

Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client

[Junio C Hamano]

"Robin Jarry" <robin@jarry.cc> writes:

>> > +		sigchain_push(SIGPIPE, SIG_IGN);
>> >  		if (pack_lockfile)
>> >  			unlink_or_warn(pack_lockfile);
>>
>> Shouldn't we start ignoring SIGPIPE here, not before we try to
>> unlink the lockfile?
>
> I initially wanted to avoid getting SIGPIPE'd while printing a warning
> if the lockfile cannot be unlinked. Maybe this means the repository
> integrity is compromised and we are well beyond ensuring post-receive is
> executed or not. I do not know git internals well enough to be sure.
>
> What do you think?

I think that push/pop pair should surround the part that reports the
status, as the proposed commit log message said.

Thanks.

[RFC PATCH] receive-pack: interrupt pre-receive when client disconnects

[Robin Jarry]

When hitting ctrl-c on the client while a remote pre-receive hook is
running, receive-pack is not killed by SIGPIPE because the signal is
ignored. This is a side effect of commit ec7dbd145bd8 ("receive-pack:
allow hooks to ignore its standard input stream").

The pre-receive hook itself is not interrupted and does not receive any
error since its stdout is a pipe which is read in an async thread and
output back to the client socket in a side band channel.

After the pre-receive has exited the SIGPIPE default handler is restored
and if the hook did not report any error, objects are migrated from
temporary to permanent storage.

This can be confusing for most people and may even be considered a bug.
When receive-pack cannot forward pre-receive output to the client, do
not ignore the error and kill the hook process so that the push does not
complete.

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
Note that if pre-receive does not produce any output, any disconnection
of the client will not cause the hook to be killed. This is not ideal
but as far as I can see, there is no way to check if the client is alive
without writing in the side band channel. Maybe by sending keepalive
packets before cleaning up. I am not comfortable enough with git
internals to be sure.

 builtin/receive-pack.c | 55 ++++++++++++++++++++++++++++++++++++------
 sideband.c             | 31 +++++++++++++++++++++---
 sideband.h             |  4 +++
 3 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2f4a38adfe2c..5668b8273486 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -469,6 +469,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 {
 	char data[128];
 	int keepalive_active = 0;
+	struct child_process *proc = arg;
 
 	if (keepalive_in_sec <= 0)
 		use_keepalive = KEEPALIVE_NEVER;
@@ -494,7 +495,11 @@ static int copy_to_sideband(int in, int out, void *arg)
 			} else if (ret == 0) {
 				/* no data; send a keepalive packet */
 				static const char buf[] = "0005\1";
-				write_or_die(1, buf, sizeof(buf) - 1);
+				if (proc && proc->pid > 0) {
+					if (write_in_full(1, buf, sizeof(buf) - 1) < 0)
+						goto error;
+				} else
+					write_or_die(1, buf, sizeof(buf) - 1);
 				continue;
 			} /* else there is actual data to read */
 		}
@@ -512,8 +517,21 @@ static int copy_to_sideband(int in, int out, void *arg)
 				 * with it.
 				 */
 				keepalive_active = 1;
-				send_sideband(1, 2, data, p - data, use_sideband);
-				send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);
+				if (proc && proc->pid > 0) {
+					if (send_sideband2(1, 2, data, p - data,
+							   use_sideband) < 0)
+						goto error;
+					if (send_sideband2(1, 2, p + 1,
+							   sz - (p - data + 1),
+							   use_sideband) < 0)
+						goto error;
+				} else {
+					send_sideband(1, 2, data, p - data,
+						      use_sideband);
+					send_sideband(1, 2, p + 1,
+						      sz - (p - data + 1),
+						      use_sideband);
+				}
 				continue;
 			}
 		}
@@ -522,10 +540,24 @@ static int copy_to_sideband(int in, int out, void *arg)
 		 * Either we're not looking for a NUL signal, or we didn't see
 		 * it yet; just pass along the data.
 		 */
-		send_sideband(1, 2, data, sz, use_sideband);
+		if (proc && proc->pid > 0) {
+			if (send_sideband2(1, 2, data, sz, use_sideband) < 0)
+				goto error;
+		} else
+			send_sideband(1, 2, data, sz, use_sideband);
 	}
 	close(in);
 	return 0;
+error:
+	close(in);
+	if (proc && proc->pid > 0) {
+		/*
+		 * SIGPIPE would be more relevant but we want to make sure that
+		 * the hook does not ignore the signal.
+		 */
+		kill(proc->pid, SIGKILL);
+	}
+	return -1;
 }
 
 static void hmac_hash(unsigned char *out,
@@ -807,7 +839,8 @@ struct receive_hook_feed_state {
 };
 
 typedef int (*feed_fn)(void *, const char **, size_t *);
-static int run_and_feed_hook(const char *hook_name, feed_fn feed,
+static int run_and_feed_hook(const char *hook_name,
+			     int isolate_sigpipe, feed_fn feed,
 			     struct receive_hook_feed_state *feed_state)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
@@ -843,6 +876,10 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	if (use_sideband) {
 		memset(&muxer, 0, sizeof(muxer));
 		muxer.proc = copy_to_sideband;
+		if (isolate_sigpipe)
+			muxer.data = NULL;
+		else
+			muxer.data = &proc;
 		muxer.in = -1;
 		code = start_async(&muxer);
 		if (code)
@@ -923,6 +960,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 static int run_receive_hook(struct command *commands,
 			    const char *hook_name,
 			    int skip_broken,
+			    int isolate_sigpipe,
 			    const struct string_list *push_options)
 {
 	struct receive_hook_feed_state state;
@@ -936,7 +974,8 @@ static int run_receive_hook(struct command *commands,
 		return 0;
 	state.cmd = commands;
 	state.push_options = push_options;
-	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
+	status = run_and_feed_hook(hook_name, isolate_sigpipe,
+				   feed_receive_hook, &state);
 	strbuf_release(&state.buf);
 	return status;
 }
@@ -1970,7 +2009,7 @@ static void execute_commands(struct command *commands,
 		}
 	}
 
-	if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
+	if (run_receive_hook(commands, "pre-receive", 0, 0, push_options)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "pre-receive hook declined";
@@ -2572,7 +2611,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		else if (report_status)
 			report(commands, unpack_status);
 		sigchain_pop(SIGPIPE);
-		run_receive_hook(commands, "post-receive", 1,
+		run_receive_hook(commands, "post-receive", 1, 1,
 				 &push_options);
 		run_update_post_hook(commands);
 		string_list_clear(&push_options, 0);
diff --git a/sideband.c b/sideband.c
index 85bddfdcd4f5..27f8d653eb24 100644
--- a/sideband.c
+++ b/sideband.c
@@ -247,11 +247,25 @@ int demultiplex_sideband(const char *me, int status,
 	return 1;
 }
 
+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,
+			      int packet_max, int ignore_errors);
+
 /*
  * fd is connected to the remote side; send the sideband data
  * over multiplexed packet stream.
  */
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)
+{
+	(void)send_sideband_priv(fd, band, data, sz, packet_max, 1);
+}
+
+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max)
+{
+	return send_sideband_priv(fd, band, data, sz, packet_max, 0);
+}
+
+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,
+			      int packet_max, int ignore_errors)
 {
 	const char *p = data;
 
@@ -265,13 +279,24 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 		if (0 <= band) {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
 			hdr[4] = band;
-			write_or_die(fd, hdr, 5);
+			if (ignore_errors)
+				write_or_die(fd, hdr, 5);
+			else if (write_in_full(fd, hdr, 5) < 0)
+				return -1;
 		} else {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
-			write_or_die(fd, hdr, 4);
+			if (ignore_errors)
+				write_or_die(fd, hdr, 4);
+			else if (write_in_full(fd, hdr, 4) < 0)
+				return -1;
 		}
-		write_or_die(fd, p, n);
+		if (ignore_errors)
+			write_or_die(fd, p, n);
+		else if (write_in_full(fd, p, n) < 0)
+			return -1;
 		p += n;
 		sz -= n;
 	}
+
+	return 0;
 }
diff --git a/sideband.h b/sideband.h
index 5a25331be55d..cb92777418e1 100644
--- a/sideband.h
+++ b/sideband.h
@@ -29,5 +29,9 @@ int demultiplex_sideband(const char *me, int status,
 			 enum sideband_type *sideband_type);
 
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
+/*
+ * Do not die on write errors, return -1 instead.
+ */
+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif
-- 
2.30.2

Re: [RFC PATCH] receive-pack: interrupt pre-receive when client disconnects

[Robin Jarry]

Hi,

do you have any remarks on this patch? Is it going in the right
direction? Should I send a non-RFC?

Happy holidays all.

Thanks!

[PATCH v3] receive-pack: ignore SIGPIPE while reporting status to client

[Robin Jarry]

Before running the post-receive hook, status info is reported back to
the client. If a remote client exits before or during the status report,
receive-pack is killed by SIGPIPE and post-receive is never executed.

The post-receive hook is often used to send email notifications (see
contrib/hooks/post-receive-email), update bug trackers, start automatic
builds, etc. Not executing it after an interrupted yet "successful" push
can lead to inconsistencies.

Ignore SIGPIPE before reporting status to the client to increase the
chances of post-receive running if pre-receive was successful. This does
not guarantee 100% consistency but it should resist early disconnection
by the client.

Signed-off-by: Robin Jarry <robin@jarry.cc>
---
Changes since v2:

* Updated commit log with more pertinent info.
* Only ignore SIGPIPE while reporting status, *after* removing the lock
  file.

 builtin/receive-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d96052..2f4a38adfe2c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2566,10 +2566,12 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 				 &push_options);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
+		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
 			report_v2(commands, unpack_status);
 		else if (report_status)
 			report(commands, unpack_status);
+		sigchain_pop(SIGPIPE);
 		run_receive_hook(commands, "post-receive", 1,
 				 &push_options);
 		run_update_post_hook(commands);
-- 
2.34.0.rc2.2.gbcf7eca935e4

Re: [PATCH v3] receive-pack: ignore SIGPIPE while reporting status to client

[Robin Jarry]

Hi,

did you get a chance to look at v3? Were there additional remarks?

Thanks.