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

[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 a pre-receive hook 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.

 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 9f4a0b816cf9..0f41fe8c6a85 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,
@@ -809,7 +841,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;
@@ -842,6 +875,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)
@@ -922,6 +959,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;
@@ -935,7 +973,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;
 }
@@ -1963,7 +2002,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";
@@ -2566,7 +2605,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.34.1

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

[Jiang Xin]

On Wed, Jan 26, 2022 at 12:09 AM Robin Jarry <robin.jarry@6wind.com> wrote:
>
> 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.

We used to ignore the SIGPIPE signal when calling "pre-receive" hook,
so we could tolerant a buggy "pre-receive" implementation which didn't
consume all the input from "receive-pack". On the other side, "ctrl-c"
from the client side will terminate "receive-pack", only if we do not
ignore the SIGPIPE signal when running "pre-receive".

Wouldn't this be much simpler: add a new configuration variable
"receive.loosePreReceiveImplementation", and only ignore SIGPIPE when
"receive-pack" turns off the config variable?

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf9..0f41fe8c6a85 100644
> @@ -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;
>  }

Kill the "pre-receive" process, so the calling of
"finish_command(&proc)" at the end of "run_and_feed_hook()" will
terminate "receive-pack".

> diff --git a/sideband.c b/sideband.c
> index 85bddfdcd4f5..27f8d653eb24 100644
> --- a/sideband.c
> +++ b/sideband.c
> +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)

"ignore_errors" or "die_on_errors"?

> +                               write_or_die(fd, hdr, 5);
> +                       else if (write_in_full(fd, hdr, 5) < 0)
> +                               return -1;

--
Jiang Xin

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

[Robin Jarry]

Jiang Xin, Jan 26, 2022 at 08:17:
> We used to ignore the SIGPIPE signal when calling "pre-receive" hook,
> so we could tolerant a buggy "pre-receive" implementation which didn't
> consume all the input from "receive-pack". On the other side, "ctrl-c"
> from the client side will terminate "receive-pack", only if we do not
> ignore the SIGPIPE signal when running "pre-receive".
>
> Wouldn't this be much simpler: add a new configuration variable
> "receive.loosePreReceiveImplementation", and only ignore SIGPIPE when
> "receive-pack" turns off the config variable?

I had not thought of this. Yes it would be much simpler. I'll prepare
another patch with this approach.

Thanks!

[PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

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

Add a new receive.strictPreReceiveImpl config option to *not* ignore
SIGPIPE when running pre-receive. If set to true, and the hook output
cannot be forwarded to the client, receive-pack will be killed via
SIGPIPE and the push will be aborted. Add a signal handler to kill and
reap the hook process before exiting. This option only affects
pre-receive.

This does not guarantee that all client disconnections will abort
a push. If there is no pre-receive hook or if it does not produce any
output, receive-pack will not be killed via SIGPIPE and the push will
complete.

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
v1 -> v2:
  Changed approach following Jiang Xin advice. Adding an option makes
  more sense and also makes a much simpler patch.

 Documentation/config/receive.txt | 13 +++++++++++++
 builtin/receive-pack.c           | 26 +++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt
index 85d5b5a3d2d8..7174168541dc 100644
--- a/Documentation/config/receive.txt
+++ b/Documentation/config/receive.txt
@@ -143,3 +143,16 @@ receive.updateServerInfo::
 receive.shallowUpdate::
 	If set to true, .git/shallow can be updated when new refs
 	require new shallow roots. Otherwise those refs are rejected.
+
+receive.strictPreReceiveImpl::
+	If a pre-receive hook does not consume its standard input fully, it may
+	kill receive-pack via SIGPIPE. This can lead to obscure push failures.
+	To avoid potential death-by-SIGPIPE due to poorly written hooks,
+	receive-pack ignores SIGPIPE while running the pre-receive hook.
++
+If this option is set to true, SIGPIPE will `not` be ignored by receive-pack
+while running the "pre-receive" hook. This has a side-effect: If the hook
+outputs something and the client has disconnected, receive-pack will be killed
+and the push will be aborted.
++
+SIGPIPE is always ignored while running "post-receive".
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf9..8718a6dd91b4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -74,6 +74,7 @@ static const char *head_name;
 static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
+static int strict_pre_receive_impl;
 static const char *alt_shallow_file;
 static struct strbuf push_cert = STRBUF_INIT;
 static struct object_id push_cert_oid;
@@ -219,6 +220,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.strictprereceiveimpl") == 0) {
+		strict_pre_receive_impl = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.certnonceseed") == 0)
 		return git_config_string(&cert_nonce_seed, var, value);
 
@@ -800,6 +806,19 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 	}
 }
 
+static volatile pid_t hook_pid;
+
+static void kill_hook(int signum)
+{
+	if (hook_pid != 0) {
+		kill(hook_pid, signum);
+		waitpid(hook_pid, NULL, 0);
+		hook_pid = 0;
+	}
+	sigchain_pop(signum);
+	raise(signum);
+}
+
 struct receive_hook_feed_state {
 	struct command *cmd;
 	struct ref_push_report *report;
@@ -858,7 +877,11 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 		return code;
 	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
+	hook_pid = proc.pid;
+	if (strict_pre_receive_impl && strcmp(hook_name, "pre-receive") == 0)
+		sigchain_push(SIGPIPE, kill_hook);
+	else
+		sigchain_push(SIGPIPE, SIG_IGN);
 
 	while (1) {
 		const char *buf;
@@ -872,6 +895,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	if (use_sideband)
 		finish_async(&muxer);
 
+	hook_pid = 0;
 	sigchain_pop(SIGPIPE);
 
 	return finish_command(&proc);
-- 
2.35.0.1.g8273a50afc47

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Jiang Xin]

On Thu, Jan 27, 2022 at 10:03 AM Robin Jarry <robin.jarry@6wind.com> wrote:
> @@ -800,6 +806,19 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>         }
>  }
>
> +static volatile pid_t hook_pid;

Can we use a flag instead of hook_pid to distinguish the source of the
SIGPIPE signal?
1. "pre-receive" hook exits early without consuming stdin.
2. "pre-receive" hook hangs after receiving commands from stdin, until
client quits by receiving a "ctrl-c".

> +static void kill_hook(int signum)
> +{
> +       if (hook_pid != 0) {
> +               kill(hook_pid, signum);
> +               waitpid(hook_pid, NULL, 0);
> +               hook_pid = 0;

Can we let the signal handler in "pre-receive" to do it job? And we
can show some user friendly error message here. E.g.:

    die("broken pipe: seems like the pre-receive hook exits early
without consuming its stdin");

--
Jiang Xin

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Robin Jarry]

Jiang Xin, Jan 27, 2022 at 04:21:
> Can we use a flag instead of hook_pid to distinguish the source of the
> SIGPIPE signal?
> 1. "pre-receive" hook exits early without consuming stdin.
> 2. "pre-receive" hook hangs after receiving commands from stdin, until
> client quits by receiving a "ctrl-c".

Also there is:

3. the client has exited and receive-pack got SIGPIPE while forwarding
   pre-receive output in the socket.

I don't think we can differentiate from these three situations from the
receive-pack point of view.

However, using a flag in the signal handler to note that SIGPIPE was
received (for whatever reason) may be better than my current
implementation.

> Can we let the signal handler in "pre-receive" to do it job? And we
> can show some user friendly error message here. E.g.:
>
>     die("broken pipe: seems like the pre-receive hook exits early
> without consuming its stdin");

If that flag is set after pre-receive has exited, we can indeed:

    die("broken pipe: ...").

Of course, if 3. the error message will never reach the client.

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Junio C Hamano]

Robin Jarry <robin.jarry@6wind.com> writes:

> 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).

I somehow feel that it is unrealistic to expect the command to be
killed via SIGPIPE because there is no guarantee that the command
has that many bytes to send out to to get the signal in the first
place.  Such an expectation is simply wrong, isn't it?

> This can be confusing for most people and may even be considered a bug.

So, there is not much I see is confusing, and I expect "most people"
would not get confused or consider it a bug.  Killing a local
process may or may not have any immediate effect on what happens on
the other side of the connection.

On the other hand, the SIGPIPE death by a poorly written pre-receive
hook was a source of real confusion.  The pushing end cannot do
anything about it to fix if the hook disconnected before reading all
of the proposed updates.

> Add a new receive.strictPreReceiveImpl config option to *not* ignore

I guess that the receiving end must know if its hook is loosely written
or not, so having a knob to revert to the older mode of operation
may probably be OK.

Do not abbreviate "Implementation" in the name of a configuration
variable, if that is the word you meant, by the way.  We try to
spell things out for clarity.

Also, "strict implementation" is way too vague.  What you want to
say here is that the hook will not stop reading its input in the
middle, causing the feeder to be killed by SIGPIPE, and from other
aspects its implementation may not be strict at all.

A name that goes well with a statement "This hook reads all of its
input" would work much better.

Should this cover only one hook, or should we introduce just one
configuration to say "all hooks that read from their standard input
stream are clean and will read their input to the end"?  Or do we
need to have N different variables for each of N hooks that may stop
reading from their standard input in the middle (not necessarily
limited to the receive-pack command)?  I think there are a handful
other hooks that take input from their standard input stream and I
am not sure if pre-receive should be singled out like this.

If this Boolean "This hook reads all of its input to the end" is to
be added per hook, I suspect that the namespace of the configuration
variable should be coordinated with the other effort to "define" hooks
in the configuration file(s) in the first place.  Emily, do you have
a suggestion?

> +static volatile pid_t hook_pid;
> +
> +static void kill_hook(int signum)
> +{
> +	if (hook_pid != 0) {
> +		kill(hook_pid, signum);
> +		waitpid(hook_pid, NULL, 0);
> +		hook_pid = 0;

Is it safe to kill(2) from within a signal handler?

Why does this patch do anything more than a partial reversion of
ec7dbd14 (receive-pack: allow hooks to ignore its standard input
stream, 2014-09-12), i.e. "if the configuration says do not be
lenient to hooks that do not consume their input, do not ignore
sigpipe at all".

> +	}
> +	sigchain_pop(signum);
> +	raise(signum);
> +}
> +
>  struct receive_hook_feed_state {
>  	struct command *cmd;
>  	struct ref_push_report *report;
> @@ -858,7 +877,11 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
>  		return code;
>  	}
>  
> -	sigchain_push(SIGPIPE, SIG_IGN);
> +	hook_pid = proc.pid;
> +	if (strict_pre_receive_impl && strcmp(hook_name, "pre-receive") == 0)
> +		sigchain_push(SIGPIPE, kill_hook);
> +	else
> +		sigchain_push(SIGPIPE, SIG_IGN);
>  
>  	while (1) {
>  		const char *buf;
> @@ -872,6 +895,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
>  	if (use_sideband)
>  		finish_async(&muxer);
>  
> +	hook_pid = 0;
>  	sigchain_pop(SIGPIPE);
>  
>  	return finish_command(&proc);

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Robin Jarry]

Junio C Hamano, Jan 27, 2022 at 05:36:
> I somehow feel that it is unrealistic to expect the command to be
> killed via SIGPIPE because there is no guarantee that the command
> has that many bytes to send out to to get the signal in the first
> place.  Such an expectation is simply wrong, isn't it?

Maybe I did not word that properly. Indeed, this only applies if
pre-receive has bytes to send out in the first place. This is what
I referred to with the last paragraph:

> > This does not guarantee that all client disconnections will abort
> > a push. If there is no pre-receive hook or if it does not produce
> > any output, receive-pack will not be killed via SIGPIPE and the push
> > will complete.

It would be much better not to rely on pre-receive to have bytes to send
and to expect that receive-pack will receive SIGPIPE when forwarding
them after the client has disconnected.

I thought of sending a "keepalive packet" in the socket *after* the
pre-receive hook has completed. I do not know the protocol details.
Would something like this be suitable:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8718a6dd91b4..2e0ddd1a59fe 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1990,16 +1990,28 @@ static void execute_commands(struct command *commands,
 	if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "pre-receive hook declined";
 		}
 		return;
 	}
 
+	/*
+	 * Send a keepalive packet to ensure that the client has not
+	 * disconnected while pre-receive was running.
+	 */
+	{
+		static const char buf[] = "0001";
+		if (use_sideband)
+			send_sideband(1, 1, buf, sizeof(buf) - 1, use_sideband);
+		else
+			write_or_die(1, buf, sizeof(buf) - 1);
+	}
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.
 	 */
 	if (tmp_objdir_migrate(tmp_objdir) < 0) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "unable to migrate objects to permanent storage";

In that situation, if the client has exited, receive-pack should be
killed via SIGPIPE before completing the push.

> Is it safe to kill(2) from within a signal handler?

Even if it is, it is probably not a good idea. I did that to avoid
leaving a zombie after receive-pack has died. Maybe setting a flag in
the signal handler and checking the flag after the process has exited
would have been better.

> Why does this patch do anything more than a partial reversion of
> ec7dbd14 (receive-pack: allow hooks to ignore its standard input
> stream, 2014-09-12), i.e. "if the configuration says do not be
> lenient to hooks that do not consume their input, do not ignore
> sigpipe at all".

Indeed it is a partial reversion of that commit. Maybe the "keepalive
before migrating to permanent storage" solution is better.

What do you think?

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Junio C Hamano]

"Robin Jarry" <robin.jarry@6wind.com> writes:

> Indeed it is a partial reversion of that commit. Maybe the "keepalive
> before migrating to permanent storage" solution is better.
>
> What do you think?

Sorry, but I was (and am) questioning why we want to do more than
"let it be killed by SIGPIPE, just like we used to do before
ec7dbd14 (receive-pack: allow hooks to ignore its standard input
stream, 2014-09-12) introduced the current behaviour", so the answer
is still "why do we even need to complicate the thing with keepalive
or anything we don't have, and we didn't have before ec7dbd14, in
the code paths that are involved?"

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Robin Jarry]

Junio C Hamano, Jan 27, 2022 at 19:26:
> Sorry, but I was (and am) questioning why we want to do more than
> "let it be killed by SIGPIPE, just like we used to do before
> ec7dbd14 (receive-pack: allow hooks to ignore its standard input
> stream, 2014-09-12) introduced the current behaviour", so the answer
> is still "why do we even need to complicate the thing with keepalive
> or anything we don't have, and we didn't have before ec7dbd14, in
> the code paths that are involved?"

My main goal is to abort a push if a user hits ctrl-c (or is
disconnected) before the objects have been moved to permanent storage.

(partially) reverting to previous behavior would only allow aborting
pushes *if* the pre-receive hook sends some output and this output
cannot be forwarded to the client. There is no guarantee that the hook
will send any output. Also, it would restore hard to track issue with
poorly written pre-receive hooks.

I wonder if it would be possible to not rely on the pre-receive hook
sending output and this output somehow not being forwarded to the
client.

Instead, explicitly check if the client is still connected and alive
after the pre-receive hook has exited but before completing the push
transaction. That was my intent with that (invalid by the way) keepalive
example.

I do not know git internals to say if it feasible without any protocol
breakage. My attempts work well for aborting pushes:

 Writing objects: 100% (3/3), 321 bytes | 321.00 KiB/s, done.
 Total 3 (delta 1), reused 1 (delta 0), pack-reused 0
 remote: pre-receive start^C
 <--- client has disconnected
      receive-pack fails to send the "keepalive" packet
      the temp objects are *not* migrated to permanent storage

But this always leads to errors on the client side when receive-pack
sends the "keepalive packet":

 Writing objects: 100% (3/3), 321 bytes | 321.00 KiB/s, done.
 Total 3 (delta 1), reused 1 (delta 0), pack-reused 0
 remote: pre-receive start
 remote: pre-receive end OK
 error: unexpected flush packet while reading remote unpack status
 error: invalid ref status from remote: unpack
 remote: post-receive start
 remote: post-receive end OK
 To git@host:repo.git
  ! [remote failure]    main -> main (remote failed to report status)
 error: failed to push some refs to 'git@host:repo.git'

Am I chasing rainbows or is that possible in the current state of the
git protocol? Maybe I need to send the keepalive packet in a sideband?
I have read the technical docs several times but I cannot understand how
everything works properly.

Thank you for your time.

[PATCH v3] receive-pack: check if client is alive before completing the push

[Robin Jarry]

Abort the push operation (i.e. do not migrate the objects from temporary
to permanent storage) if the client has disconnected while the
pre-receive hook was running.

This reduces the risk of inconsistencies on network errors or if the
user hits ctrl-c while the pre-receive hook is running.

Send a keepalive packet (empty) on sideband 2 (the one to report
progress). If the client has exited, receive-pack will be killed via
SIGPIPE and the push will be aborted. This only works when sideband*
capabilities are advertised by the client.

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
v2 -> v3:
    I had missed Documentation/technical/pack-protocol.txt. Using
    sideband 2 to send the keepalive packet works.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf9..8b0d56897c9f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	/*
+	 * Send a keepalive packet on sideband 2 (progress info) to ensure that
+	 * the client has not disconnected while pre-receive was running.
+	 */
+	if (use_sideband) {
+		static const char buf[] = "0005\2";
+		write_or_die(1, buf, sizeof(buf) - 1);
+	}
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.
-- 
2.35.0.4.gfdf4c72cdf3d

Re: [PATCH v3] receive-pack: check if client is alive before completing the push

[Junio C Hamano]

Robin Jarry <robin.jarry@6wind.com> writes:

> Abort the push operation (i.e. do not migrate the objects from temporary
> to permanent storage) if the client has disconnected while the
> pre-receive hook was running.
>
> This reduces the risk of inconsistencies on network errors or if the
> user hits ctrl-c while the pre-receive hook is running.
>
> Send a keepalive packet (empty) on sideband 2 (the one to report
> progress). If the client has exited, receive-pack will be killed via
> SIGPIPE and the push will be aborted. This only works when sideband*
> capabilities are advertised by the client.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> v2 -> v3:
>     I had missed Documentation/technical/pack-protocol.txt. Using
>     sideband 2 to send the keepalive packet works.

Yes, as long as sideband capability is supported (which is true
everywhere these days), this would be good.

Simple and sensible.

Thanks.



>  builtin/receive-pack.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf9..8b0d56897c9f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}
>  
> +	/*
> +	 * Send a keepalive packet on sideband 2 (progress info) to ensure that
> +	 * the client has not disconnected while pre-receive was running.
> +	 */
> +	if (use_sideband) {
> +		static const char buf[] = "0005\2";
> +		write_or_die(1, buf, sizeof(buf) - 1);
> +	}
> +
>  	/*
>  	 * Now we'll start writing out refs, which means the objects need
>  	 * to be in their final positions so that other processes can see them.

Re: [PATCH v3] receive-pack: check if client is alive before completing the push

[Robin Jarry]

Junio C Hamano, Jan 28, 2022 at 02:19:
> Yes, as long as sideband capability is supported (which is true
> everywhere these days), this would be good.
>
> Simple and sensible.
>
> Thanks.

Thank you for your patience :)

Re: [PATCH v3] receive-pack: check if client is alive before completing the push

[Junio C Hamano]

Robin Jarry <robin.jarry@6wind.com> writes:

> Abort the push operation (i.e. do not migrate the objects from temporary
> to permanent storage) if the client has disconnected while the
> pre-receive hook was running.
>
> This reduces the risk of inconsistencies on network errors or if the
> user hits ctrl-c while the pre-receive hook is running.
>
> Send a keepalive packet (empty) on sideband 2 (the one to report
> progress). If the client has exited, receive-pack will be killed via
> SIGPIPE and the push will be aborted. This only works when sideband*
> capabilities are advertised by the client.

If they have already exited but the fact hasn't reached us over the
network, the write() will succeed to deposit the packet in the send
buffer.  So I am not sure how much this would actually help, but it
should be safe to send an unsolicited keepalive as long as the other
side is expecting to hear from us.  When either report_status or
report_status_v2 capabilities is in effect, we will make a report()
or report_v2() call later, so we should be safe.

> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> v2 -> v3:
>     I had missed Documentation/technical/pack-protocol.txt. Using
>     sideband 2 to send the keepalive packet works.
>
>  builtin/receive-pack.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf9..8b0d56897c9f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}
>  
> +	/*
> +	 * Send a keepalive packet on sideband 2 (progress info) to ensure that
> +	 * the client has not disconnected while pre-receive was running.
> +	 */

I suspect that any keepalive, unless it expects an active "yes, I am
still alive" response from the other side, is too weak to "ensure".

I guess "to notice a client that has disconnected (e.g. killed with
^C)" is more appropriate.

> +	if (use_sideband) {
> +		static const char buf[] = "0005\2";
> +		write_or_die(1, buf, sizeof(buf) - 1);
> +	}

Observing how execute_commands() and helper functions report an
error to the callers higher in the call chain, and ask them to abort
the remainder of the operation, I am not sure if write_or_die() is
appropriate.

    Side note: inside copy_to_sideband(), which runs in async, it is
    a different matter (i.e. the main process on our side is not
    what gets killed by that _or_die() part of the call), but this
    one kills the main process.

The convention around this code path seems to be to fill explanation
of error in cmd->error_string and return to the caller.  In this
case, the error_strings may not reach the pusher via report() or
report_v2() as they may have disconnected, but calling the report()
functions is not the only thing the caller will want to do after
calling us, so giving it a chance to clean up may be a better
design, e.g.

	if (write_in_full(...) < 0) {
		for (cmd = commands; cmd; cmd = cmd->next)
	        	cmd->error_string = "pusher went away";
		return;
	}

Yes, the current code will not actually use the error string in any
useful way in this particular case, since report() or report_v2()
will have nobody listening to them.  But being consistent will help
maintaining the caller, as it can later be extended to use it
locally (e.g. log the request and its outcome, check which cmd has
succeeded and failed using the NULL-ness of cmd->error_string, etc.)

Re: [PATCH v3] receive-pack: check if client is alive before completing the push

[Robin Jarry]

Junio C Hamano, Jan 28, 2022 at 18:52:
> If they have already exited but the fact hasn't reached us over the
> network, the write() will succeed to deposit the packet in the send
> buffer.  So I am not sure how much this would actually help, but it
> should be safe to send an unsolicited keepalive as long as the other
> side is expecting to hear from us.  When either report_status or
> report_status_v2 capabilities is in effect, we will make a report()
> or report_v2() call later, so we should be safe.

This is not perfect but I think this is the best we can do without
adding a new capability so that the client sends a reply to the
keepalive packet.

> I suspect that any keepalive, unless it expects an active "yes, I am
> still alive" response from the other side, is too weak to "ensure".
>
> I guess "to notice a client that has disconnected (e.g. killed with
> ^C)" is more appropriate.

OK, I will change that.

> > +	if (use_sideband) {
> > +		static const char buf[] = "0005\2";
> > +		write_or_die(1, buf, sizeof(buf) - 1);
> > +	}
>
> Observing how execute_commands() and helper functions report an
> error to the callers higher in the call chain, and ask them to abort
> the remainder of the operation, I am not sure if write_or_die() is
> appropriate.
>
>     Side note: inside copy_to_sideband(), which runs in async, it is
>     a different matter (i.e. the main process on our side is not
>     what gets killed by that _or_die() part of the call), but this
>     one kills the main process.
>
> The convention around this code path seems to be to fill explanation
> of error in cmd->error_string and return to the caller.  In this
> case, the error_strings may not reach the pusher via report() or
> report_v2() as they may have disconnected, but calling the report()
> functions is not the only thing the caller will want to do after
> calling us, so giving it a chance to clean up may be a better
> design, e.g.
>
> 	if (write_in_full(...) < 0) {
> 		for (cmd = commands; cmd; cmd = cmd->next)
> 	        	cmd->error_string = "pusher went away";
> 		return;
> 	}
>
> Yes, the current code will not actually use the error string in any
> useful way in this particular case, since report() or report_v2()
> will have nobody listening to them.  But being consistent will help
> maintaining the caller, as it can later be extended to use it
> locally (e.g. log the request and its outcome, check which cmd has
> succeeded and failed using the NULL-ness of cmd->error_string, etc.)

The main receive-pack process will be killed by SIGPIPE anyway but I can
fill the error_string fields and return for code consistency.

I'll send a v4, thanks for the review.

[PATCH v4] receive-pack: check if client is alive before completing the push

[Robin Jarry]

Abort the push operation (i.e. do not migrate the objects from temporary
to permanent storage) if the client has disconnected while the
pre-receive hook was running.

This reduces the risk of inconsistencies on network errors or if the
user hits ctrl-c while the pre-receive hook is running.

Send a keepalive packet (empty) on sideband 2 (the one to report
progress). If the client has exited the write() operation should fail
and the push will be aborted. This only works when sideband*
capabilities are advertised by the client.

Note: if the write() operation fails, receive-pack will likely be killed
via SIGPIPE and even so, since the client is likely gone already, the
error strings will go nowhere. I only added them for code consistency.

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
---
v3 -> v4:
  - reworded the comment block s/ensure/notice/
  - used write_in_full() instead of write_or_die()
  - set error_string fields for code consistency

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9f4a0b816cf9..f8b9a9312733 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1971,6 +1971,22 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	/*
+	 * Send a keepalive packet on sideband 2 (progress info) to notice
+	 * a client that has disconnected (e.g. killed with ^C) while
+	 * pre-receive was running.
+	 */
+	if (use_sideband) {
+		static const char buf[] = "0005\2";
+		if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
+			for (cmd = commands; cmd; cmd = cmd->next) {
+				if (!cmd->error_string)
+					cmd->error_string = "pusher went away";
+			}
+			return;
+		}
+	}
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.
-- 
2.35.0.4.g44a5d4affccf

Re: [PATCH v4] receive-pack: check if client is alive before completing the push

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 28 2022, Robin Jarry wrote:

> Abort the push operation (i.e. do not migrate the objects from temporary
> to permanent storage) if the client has disconnected while the
> pre-receive hook was running.
>
> This reduces the risk of inconsistencies on network errors or if the
> user hits ctrl-c while the pre-receive hook is running.
>
> Send a keepalive packet (empty) on sideband 2 (the one to report
> progress). If the client has exited the write() operation should fail
> and the push will be aborted. This only works when sideband*
> capabilities are advertised by the client.
>
> Note: if the write() operation fails, receive-pack will likely be killed
> via SIGPIPE and even so, since the client is likely gone already, the
> error strings will go nowhere. I only added them for code consistency.
>
> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> v3 -> v4:
>   - reworded the comment block s/ensure/notice/
>   - used write_in_full() instead of write_or_die()
>   - set error_string fields for code consistency
>
>  builtin/receive-pack.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf9..f8b9a9312733 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1971,6 +1971,22 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}
>  
> +	/*
> +	 * Send a keepalive packet on sideband 2 (progress info) to notice
> +	 * a client that has disconnected (e.g. killed with ^C) while
> +	 * pre-receive was running.
> +	 */
> +	if (use_sideband) {
> +		static const char buf[] = "0005\2";
> +		if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
> +			for (cmd = commands; cmd; cmd = cmd->next) {
> +				if (!cmd->error_string)
> +					cmd->error_string = "pusher went away";
> +			}
> +			return;
> +		}
> +	}
> +
>  	/*
>  	 * Now we'll start writing out refs, which means the objects need
>  	 * to be in their final positions so that other processes can see them.

I've read the upthread, but I still don't quite get why it's a must to
unconditionally abort the push because the pusher went away.

At this point we've passed the pre-receive hook, are about to migrate
the objects, still have proc-receive left to run, and finally will
update the refs.

Is the motivation purely a UX change where it's considered that the user
*must* be shown the output, or are we doing the wrong thing and not
continuing at all if we run into SIGPIPE here (then presumably only for
hooks that produce output?).

I admit this is somewhat contrived, but aren't we now doing worse for
users where the pre-receive hook takes 10s, but they already asked for
their push to be performed. Then they disconnect from WiFi unexpectedly,
and find that that it didn't go through?

Anyway, I see you made this opt-in configurable in earlier iterations. I
wonder if that's still something worth doing, or if we should just take
this change as-is.

What I don't get is *if* we're doing this for the UX reason why are we
singling out the pre-receive hook in particular, and not covering
proc-receive? I.e. we'll also produce output the user might see there,
as you can see with this ad-hoc testing change (showhing changed "git
push" output when I add to the hook output):

	diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
	index cc08506cf0b..933f0599497 100644
	--- a/t/helper/test-proc-receive.c
	+++ b/t/helper/test-proc-receive.c
	@@ -188,6 +188,7 @@ int cmd__proc_receive(int argc, const char **argv)
	                if (returns.nr)
	                        for_each_string_list_item(item, &returns)
	                                fprintf(stderr, "proc-receive> %s\n", item->string);
	+               fprintf(stderr, "showing a custom message\n");
	        }
	 
	        if (die_write_report)

	$ ./t5411-proc-receive-hook.sh --run=1-3,5-42 -vixd
	[...]
	+ diff -u expect actual
	--- expect      2022-02-04 11:53:52.006413296 +0000
	+++ actual      2022-02-04 11:53:52.006413296 +0000
	@@ -3,6 +3,7 @@
	 remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        
	 remote: # proc-receive hook        
	 remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        
	+remote: showing a custom message        
	 remote: # post-receive hook        
	 remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next        
	 To <URL/of/upstream.git>
	error: last command exited with $?=1

Is the unstated reason that we consider the tmp_objdir_migrate() more of
a a point of no return?

IOW I'm wondering why it doesn't look more like this (the object
migration could probably be dropped, it should be near-ish instant, but
proc-receive can take a long time):

	diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
	index f8b9a931273..33bbafbc9e2 100644
	--- a/builtin/receive-pack.c
	+++ b/builtin/receive-pack.c
	@@ -1907,6 +1907,26 @@ static void execute_commands_atomic(struct command *commands,
	 	strbuf_release(&err);
	 }
	 
	+static int pusher_went_away(struct command *commands, const char *msg)
	+{
	+	struct command *cmd;
	+	static const char buf[] = "0005\2";
	+
	+	/*
	+	 * Send a keepalive packet on sideband 2 (progress info) to notice
	+	 * a client that has disconnected (e.g. killed with ^C) while
	+	 * pre-receive was running.
	+	 */
	+	if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
	+		for (cmd = commands; cmd; cmd = cmd->next) {
	+			if (!cmd->error_string)
	+				cmd->error_string = msg;
	+		}
	+		return 1;
	+	}
	+	return 0;
	+}
	+
	 static void execute_commands(struct command *commands,
	 			     const char *unpacker_error,
	 			     struct shallow_info *si,
	@@ -1971,21 +1991,9 @@ static void execute_commands(struct command *commands,
	 		return;
	 	}
	 
	-	/*
	-	 * Send a keepalive packet on sideband 2 (progress info) to notice
	-	 * a client that has disconnected (e.g. killed with ^C) while
	-	 * pre-receive was running.
	-	 */
	-	if (use_sideband) {
	-		static const char buf[] = "0005\2";
	-		if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
	-			for (cmd = commands; cmd; cmd = cmd->next) {
	-				if (!cmd->error_string)
	-					cmd->error_string = "pusher went away";
	-			}
	-			return;
	-		}
	-	}
	+	if (use_sideband && pusher_went_away(commands,
	+					     "pusher can't be contacted post-pre-receive"))
	+		return;
	 
	 	/*
	 	 * Now we'll start writing out refs, which means the objects need
	@@ -2000,6 +2008,10 @@ static void execute_commands(struct command *commands,
	 	}
	 	tmp_objdir = NULL;
	 
	+	if (use_sideband && pusher_went_away(commands,
	+					     "pusher can't be contacted post-object migration"))
	+		return;
	+
	 	check_aliased_updates(commands);
	 
	 	free(head_name_to_free);
	@@ -2013,6 +2025,10 @@ static void execute_commands(struct command *commands,
	 			    (cmd->run_proc_receive || use_atomic))
	 				cmd->error_string = "fail to run proc-receive hook";
	 
	+	if (use_sideband && pusher_went_away(commands,
	+					     "pusher can't be contacted post-proc-receive"))
	+		return;
	+
	 	if (use_atomic)
	 		execute_commands_atomic(commands, si);
	 	else

But also, this whole thing is "if the pre-receive hook etc. etc.", but
we do in fact run this when there's no hook at all. See how this
interacts with run_and_feed_hook() and the "!hook_path" check.

So isn't this unnecessary if there's no such hook, and we should unfold
the find_hook() etc. from that codepath (or pass up a "I ran the hook"
state)?

Re: [PATCH v4] receive-pack: check if client is alive before completing the push

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Is the motivation purely a UX change where it's considered that the user
> *must* be shown the output, or are we doing the wrong thing and not
> continuing at all if we run into SIGPIPE here (then presumably only for
> hooks that produce output?).
>
> I admit this is somewhat contrived, but aren't we now doing worse for
> users where the pre-receive hook takes 10s, but they already asked for
> their push to be performed. Then they disconnect from WiFi unexpectedly,
> and find that that it didn't go through?
>
> Anyway, I see you made this opt-in configurable in earlier iterations. I
> wonder if that's still something worth doing, or if we should just take
> this change as-is.

I guess the above is exactly the same reaction I still have against
the series.  In a case where the user did *not* see "git push"
complete after getting a positive response from the other side that
says the changes to refs have succeeded, due to whatever reason
(e.g. "^C" or connection droppage), the user cannot expect whether
the push to have completed or got aborted, both from the UX point of
view and from the correctness point of view, I would think.

Your keyboard interrupt "^C" may have come too late to matter at the
receiving end, or your WiFi may or may not have disconnected before
the receiving end got everything necessary from you to carry out the
operation, for example, and you are not simply in control of these
things.

Re: [PATCH v4] receive-pack: check if client is alive before completing the push

[Robin Jarry]

Hi Ævar,

Ævar Arnfjörð Bjarmason, Feb 04, 2022 at 12:37:
> I've read the upthread, but I still don't quite get why it's a must to
> unconditionally abort the push because the pusher went away.
>
> At this point we've passed the pre-receive hook, are about to migrate
> the objects, still have proc-receive left to run, and finally will
> update the refs.
>
> Is the motivation purely a UX change where it's considered that the user
> *must* be shown the output, or are we doing the wrong thing and not
> continuing at all if we run into SIGPIPE here (then presumably only for
> hooks that produce output?).
>
> I admit this is somewhat contrived, but aren't we now doing worse for
> users where the pre-receive hook takes 10s, but they already asked for
> their push to be performed. Then they disconnect from WiFi unexpectedly,
> and find that that it didn't go through?

This *is* purely motivated by UX.

pre-receive hooks may that perform various verifications. They may
require connecting to a bug tracker to validate that the referenced
tickets are in the proper state and associated with the proper git
repository. They may also run patch validation scripts such as
[checkpatch.pl][1].

[1]: https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html

When pushing large series of commits, these checks can take
a significant amount of time. While the checks are running, some users
may change their mind and hit ctrl-c because they forgot something.

On their point of view, the operation seems to have been aborted.
Whereas if the pre-receive hook completes without errors, the push will
be completed. This may be confusing to some.

> Anyway, I see you made this opt-in configurable in earlier iterations. I
> wonder if that's still something worth doing, or if we should just take
> this change as-is.

The earlier iterations were a lot more complex and actually messed with
SIGPIPE forwarding to the pre-receive hook itself. This last version is
much simpler so I did not think about adding an option. I could make
this behaviour opt-in, I don't mind.

> What I don't get is *if* we're doing this for the UX reason why are we
> singling out the pre-receive hook in particular, and not covering
> proc-receive? I.e. we'll also produce output the user might see there,
> as you can see with this ad-hoc testing change (showhing changed "git
> push" output when I add to the hook output):
>
> 	diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
> 	index cc08506cf0b..933f0599497 100644
> 	--- a/t/helper/test-proc-receive.c
> 	+++ b/t/helper/test-proc-receive.c
> 	@@ -188,6 +188,7 @@ int cmd__proc_receive(int argc, const char **argv)
> 	                if (returns.nr)
> 	                        for_each_string_list_item(item, &returns)
> 	                                fprintf(stderr, "proc-receive> %s\n", item->string);
> 	+               fprintf(stderr, "showing a custom message\n");
> 	        }
> 	 
> 	        if (die_write_report)
>
> 	$ ./t5411-proc-receive-hook.sh --run=1-3,5-42 -vixd
> 	[...]
> 	+ diff -u expect actual
> 	--- expect      2022-02-04 11:53:52.006413296 +0000
> 	+++ actual      2022-02-04 11:53:52.006413296 +0000
> 	@@ -3,6 +3,7 @@
> 	 remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        
> 	 remote: # proc-receive hook        
> 	 remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        
> 	+remote: showing a custom message        
> 	 remote: # post-receive hook        
> 	 remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next        
> 	 To <URL/of/upstream.git>
> 	error: last command exited with $?=1
>
> Is the unstated reason that we consider the tmp_objdir_migrate() more of
> a a point of no return?

I have almost zero experience with proc-receive. I had understood that
tmp_objdir_migrate() meant that the push operation was "complete" in the
sense that commits, tags, branches had been updated (regardless of
proc-receive status). Maybe I am completely wrong.

> IOW I'm wondering why it doesn't look more like this (the object
> migration could probably be dropped, it should be near-ish instant, but
> proc-receive can take a long time):
>
> 	diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> 	index f8b9a931273..33bbafbc9e2 100644
> 	--- a/builtin/receive-pack.c
> 	+++ b/builtin/receive-pack.c
> 	@@ -1907,6 +1907,26 @@ static void execute_commands_atomic(struct command *commands,
> 	 	strbuf_release(&err);
> 	 }
> 	 
> 	+static int pusher_went_away(struct command *commands, const char *msg)
> 	+{
> 	+	struct command *cmd;
> 	+	static const char buf[] = "0005\2";
> 	+
> 	+	/*
> 	+	 * Send a keepalive packet on sideband 2 (progress info) to notice
> 	+	 * a client that has disconnected (e.g. killed with ^C) while
> 	+	 * pre-receive was running.
> 	+	 */
> 	+	if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
> 	+		for (cmd = commands; cmd; cmd = cmd->next) {
> 	+			if (!cmd->error_string)
> 	+				cmd->error_string = msg;
> 	+		}
> 	+		return 1;
> 	+	}
> 	+	return 0;
> 	+}
> 	+
> 	 static void execute_commands(struct command *commands,
> 	 			     const char *unpacker_error,
> 	 			     struct shallow_info *si,
> 	@@ -1971,21 +1991,9 @@ static void execute_commands(struct command *commands,
> 	 		return;
> 	 	}
> 	 
> 	-	/*
> 	-	 * Send a keepalive packet on sideband 2 (progress info) to notice
> 	-	 * a client that has disconnected (e.g. killed with ^C) while
> 	-	 * pre-receive was running.
> 	-	 */
> 	-	if (use_sideband) {
> 	-		static const char buf[] = "0005\2";
> 	-		if (write_in_full(1, buf, sizeof(buf) - 1) < 0) {
> 	-			for (cmd = commands; cmd; cmd = cmd->next) {
> 	-				if (!cmd->error_string)
> 	-					cmd->error_string = "pusher went away";
> 	-			}
> 	-			return;
> 	-		}
> 	-	}
> 	+	if (use_sideband && pusher_went_away(commands,
> 	+					     "pusher can't be contacted post-pre-receive"))
> 	+		return;
> 	 
> 	 	/*
> 	 	 * Now we'll start writing out refs, which means the objects need
> 	@@ -2000,6 +2008,10 @@ static void execute_commands(struct command *commands,
> 	 	}
> 	 	tmp_objdir = NULL;
> 	 
> 	+	if (use_sideband && pusher_went_away(commands,
> 	+					     "pusher can't be contacted post-object migration"))
> 	+		return;
> 	+
> 	 	check_aliased_updates(commands);
> 	 
> 	 	free(head_name_to_free);
> 	@@ -2013,6 +2025,10 @@ static void execute_commands(struct command *commands,
> 	 			    (cmd->run_proc_receive || use_atomic))
> 	 				cmd->error_string = "fail to run proc-receive hook";
> 	 
> 	+	if (use_sideband && pusher_went_away(commands,
> 	+					     "pusher can't be contacted post-proc-receive"))
> 	+		return;
> 	+
> 	 	if (use_atomic)
> 	 		execute_commands_atomic(commands, si);
> 	 	else
>
> But also, this whole thing is "if the pre-receive hook etc. etc.", but
> we do in fact run this when there's no hook at all. See how this
> interacts with run_and_feed_hook() and the "!hook_path" check.
>
> So isn't this unnecessary if there's no such hook, and we should unfold
> the find_hook() etc. from that codepath (or pass up a "I ran the hook"
> state)?

You're right, maybe this keepalive packet should only be sent if there
is a pre-receive hook.

Also, if proc-receive can indeed reject the push operation, there should
be multiple "checkpoints" as you said.

To sum up, I can send a v5 with multiple "checkpoints" and only via an
opt-in config option. Would that be OK?

Cheers

Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

[Junio C Hamano]

"Robin Jarry" <robin.jarry@6wind.com> writes:

> My main goal is to abort a push if a user hits ctrl-c (or is
> disconnected) before the objects have been moved to permanent storage.
>
> But this always leads to errors on the client side when receive-pack
> sends the "keepalive packet":

Yes, you'd need to make all three new combinations work if you touch
the protocol.  An updated "receive-pack" must be inter-operable with
a vanilla "push" as well as an updated "push", and an updated "push"
must be inter-operable with a vanilla "receive-pack".

You'd need to invent a new protocol capability, advertise it on the
updated "receive-pack" side, and the updated "push" must check if
the capability is advertized before asking to activate it.  Then
only after both ends discover that the other side knows how to deal
with "keepalive" packets, use that feature.