{"thread":{"id":"57305","subject":"[PATCH] receive-pack: interrupt pre-receive when client disconnects","startedAt":"2022-01-25T10:07:31Z","lastAt":"2022-02-07T19:31:48Z","messageCount":20,"participants":["Robin Jarry","Jiang Xin","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446836","messageId":"20220125095445.1796938-1-robin.jarry@6wind.com","threadId":"57305","inReplyTo":null,"subject":"[PATCH] receive-pack: interrupt pre-receive when client disconnects","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-25T09:54:44Z","receivedAt":"2022-01-25T10:07:31Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"When hitting ctrl-c on the client while a remote pre-receive hook is\nrunning, receive-pack is not killed by SIGPIPE because the signal is\nignored. This is a side effect of commit ec7dbd145bd8 (\"receive-pack:\nallow hooks to ignore its standard input stream\").\n\nThe pre-receive hook itself is not interrupted and does not receive any\nerror since its stdout is a pipe which is read in an async thread and\noutput back to the client socket in a side band channel.\n\nAfter the pre-receive has exited the SIGPIPE default handler is restored\nand if the hook did not report any error, objects are migrated from\ntemporary to permanent storage.\n\nThis can be confusing for most people and may even be considered a bug.\nWhen receive-pack cannot forward pre-receive output to the client, do\nnot ignore the error and kill the hook process so that the push does not\ncomplete.\n\nSigned-off-by: Robin Jarry <robin.jarry@6wind.com>\n---\nNote that if a pre-receive hook does not produce any output, any\ndisconnection of the client will not cause the hook to be killed. This\nis not ideal but as far as I can see, there is no way to check if the\nclient is alive without writing in the side band channel.\n\n builtin/receive-pack.c | 55 ++++++++++++++++++++++++++++++++++++------\n sideband.c             | 31 +++++++++++++++++++++---\n sideband.h             |  4 +++\n 3 files changed, 79 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf9..0f41fe8c6a85 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -469,6 +469,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n \tint keepalive_active = 0;\n+\tstruct child_process *proc = arg;\n \n \tif (keepalive_in_sec <= 0)\n \t\tuse_keepalive = KEEPALIVE_NEVER;\n@@ -494,7 +495,11 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t\t} else if (ret == 0) {\n \t\t\t\t/* no data; send a keepalive packet */\n \t\t\t\tstatic const char buf[] = \"0005\\1\";\n-\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n+\t\t\t\tif (proc && proc->pid > 0) {\n+\t\t\t\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t} else\n+\t\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n \t\t\t\tcontinue;\n \t\t\t} /* else there is actual data to read */\n \t\t}\n@@ -512,8 +517,21 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t\t\t * with it.\n \t\t\t\t */\n \t\t\t\tkeepalive_active = 1;\n-\t\t\t\tsend_sideband(1, 2, data, p - data, use_sideband);\n-\t\t\t\tsend_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n+\t\t\t\tif (proc && proc->pid > 0) {\n+\t\t\t\t\tif (send_sideband2(1, 2, data, p - data,\n+\t\t\t\t\t\t\t   use_sideband) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t\tif (send_sideband2(1, 2, p + 1,\n+\t\t\t\t\t\t\t   sz - (p - data + 1),\n+\t\t\t\t\t\t\t   use_sideband) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t} else {\n+\t\t\t\t\tsend_sideband(1, 2, data, p - data,\n+\t\t\t\t\t\t      use_sideband);\n+\t\t\t\t\tsend_sideband(1, 2, p + 1,\n+\t\t\t\t\t\t      sz - (p - data + 1),\n+\t\t\t\t\t\t      use_sideband);\n+\t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n@@ -522,10 +540,24 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t * Either we're not looking for a NUL signal, or we didn't see\n \t\t * it yet; just pass along the data.\n \t\t */\n-\t\tsend_sideband(1, 2, data, sz, use_sideband);\n+\t\tif (proc && proc->pid > 0) {\n+\t\t\tif (send_sideband2(1, 2, data, sz, use_sideband) < 0)\n+\t\t\t\tgoto error;\n+\t\t} else\n+\t\t\tsend_sideband(1, 2, data, sz, use_sideband);\n \t}\n \tclose(in);\n \treturn 0;\n+error:\n+\tclose(in);\n+\tif (proc && proc->pid > 0) {\n+\t\t/*\n+\t\t * SIGPIPE would be more relevant but we want to make sure that\n+\t\t * the hook does not ignore the signal.\n+\t\t */\n+\t\tkill(proc->pid, SIGKILL);\n+\t}\n+\treturn -1;\n }\n \n static void hmac_hash(unsigned char *out,\n@@ -809,7 +841,8 @@ struct receive_hook_feed_state {\n };\n \n typedef int (*feed_fn)(void *, const char **, size_t *);\n-static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n+static int run_and_feed_hook(const char *hook_name,\n+\t\t\t     int isolate_sigpipe, feed_fn feed,\n \t\t\t     struct receive_hook_feed_state *feed_state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n@@ -842,6 +875,10 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n \t\tmuxer.proc = copy_to_sideband;\n+\t\tif (isolate_sigpipe)\n+\t\t\tmuxer.data = NULL;\n+\t\telse\n+\t\t\tmuxer.data = &proc;\n \t\tmuxer.in = -1;\n \t\tcode = start_async(&muxer);\n \t\tif (code)\n@@ -922,6 +959,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n static int run_receive_hook(struct command *commands,\n \t\t\t    const char *hook_name,\n \t\t\t    int skip_broken,\n+\t\t\t    int isolate_sigpipe,\n \t\t\t    const struct string_list *push_options)\n {\n \tstruct receive_hook_feed_state state;\n@@ -935,7 +973,8 @@ static int run_receive_hook(struct command *commands,\n \t\treturn 0;\n \tstate.cmd = commands;\n \tstate.push_options = push_options;\n-\tstatus = run_and_feed_hook(hook_name, feed_receive_hook, &state);\n+\tstatus = run_and_feed_hook(hook_name, isolate_sigpipe,\n+\t\t\t\t   feed_receive_hook, &state);\n \tstrbuf_release(&state.buf);\n \treturn status;\n }\n@@ -1963,7 +2002,7 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n-\tif (run_receive_hook(commands, \"pre-receive\", 0, push_options)) {\n+\tif (run_receive_hook(commands, \"pre-receive\", 0, 0, push_options)) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\t\tif (!cmd->error_string)\n \t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n@@ -2566,7 +2605,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\telse if (report_status)\n \t\t\treport(commands, unpack_status);\n \t\tsigchain_pop(SIGPIPE);\n-\t\trun_receive_hook(commands, \"post-receive\", 1,\n+\t\trun_receive_hook(commands, \"post-receive\", 1, 1,\n \t\t\t\t &push_options);\n \t\trun_update_post_hook(commands);\n \t\tstring_list_clear(&push_options, 0);\ndiff --git a/sideband.c b/sideband.c\nindex 85bddfdcd4f5..27f8d653eb24 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -247,11 +247,25 @@ int demultiplex_sideband(const char *me, int status,\n \treturn 1;\n }\n \n+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,\n+\t\t\t      int packet_max, int ignore_errors);\n+\n /*\n  * fd is connected to the remote side; send the sideband data\n  * over multiplexed packet stream.\n  */\n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)\n+{\n+\t(void)send_sideband_priv(fd, band, data, sz, packet_max, 1);\n+}\n+\n+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max)\n+{\n+\treturn send_sideband_priv(fd, band, data, sz, packet_max, 0);\n+}\n+\n+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,\n+\t\t\t      int packet_max, int ignore_errors)\n {\n \tconst char *p = data;\n \n@@ -265,13 +279,24 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tif (ignore_errors)\n+\t\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\telse if (write_in_full(fd, hdr, 5) < 0)\n+\t\t\t\treturn -1;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tif (ignore_errors)\n+\t\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\telse if (write_in_full(fd, hdr, 4) < 0)\n+\t\t\t\treturn -1;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\t\tif (ignore_errors)\n+\t\t\twrite_or_die(fd, p, n);\n+\t\telse if (write_in_full(fd, p, n) < 0)\n+\t\t\treturn -1;\n \t\tp += n;\n \t\tsz -= n;\n \t}\n+\n+\treturn 0;\n }\ndiff --git a/sideband.h b/sideband.h\nindex 5a25331be55d..cb92777418e1 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -29,5 +29,9 @@ int demultiplex_sideband(const char *me, int status,\n \t\t\t enum sideband_type *sideband_type);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n+/*\n+ * Do not die on write errors, return -1 instead.\n+ */\n+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n #endif\n-- \n2.34.1\n\n"},{"id":"446917","messageId":"CANYiYbGRK0eshjUJoPH0yWT1tVLoerOMC6CY6tAMrwAh7T+y1g@mail.gmail.com","threadId":"57305","inReplyTo":"20220125095445.1796938-1-robin.jarry@6wind.com","subject":"Re: [PATCH] receive-pack: interrupt pre-receive when client disconnects","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-01-26T07:17:42Z","receivedAt":"2022-01-26T07:17:57Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Wed, Jan 26, 2022 at 12:09 AM Robin Jarry <robin.jarry@6wind.com> wrote:\n>\n> When hitting ctrl-c on the client while a remote pre-receive hook is\n> running, receive-pack is not killed by SIGPIPE because the signal is\n> ignored. This is a side effect of commit ec7dbd145bd8 (\"receive-pack:\n> allow hooks to ignore its standard input stream\").\n>\n> The pre-receive hook itself is not interrupted and does not receive any\n> error since its stdout is a pipe which is read in an async thread and\n> output back to the client socket in a side band channel.\n>\n> After the pre-receive has exited the SIGPIPE default handler is restored\n> and if the hook did not report any error, objects are migrated from\n> temporary to permanent storage.\n\nWe used to ignore the SIGPIPE signal when calling \"pre-receive\" hook,\nso we could tolerant a buggy \"pre-receive\" implementation which didn't\nconsume all the input from \"receive-pack\". On the other side, \"ctrl-c\"\nfrom the client side will terminate \"receive-pack\", only if we do not\nignore the SIGPIPE signal when running \"pre-receive\".\n\nWouldn't this be much simpler: add a new configuration variable\n\"receive.loosePreReceiveImplementation\", and only ignore SIGPIPE when\n\"receive-pack\" turns off the config variable?\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf9..0f41fe8c6a85 100644\n> @@ -522,10 +540,24 @@ static int copy_to_sideband(int in, int out, void *arg)\n>                  * Either we're not looking for a NUL signal, or we didn't see\n>                  * it yet; just pass along the data.\n>                  */\n> -               send_sideband(1, 2, data, sz, use_sideband);\n> +               if (proc && proc->pid > 0) {\n> +                       if (send_sideband2(1, 2, data, sz, use_sideband) < 0)\n> +                               goto error;\n> +               } else\n> +                       send_sideband(1, 2, data, sz, use_sideband);\n>         }\n>         close(in);\n>         return 0;\n> +error:\n> +       close(in);\n> +       if (proc && proc->pid > 0) {\n> +               /*\n> +                * SIGPIPE would be more relevant but we want to make sure that\n> +                * the hook does not ignore the signal.\n> +                */\n> +               kill(proc->pid, SIGKILL);\n> +       }\n> +       return -1;\n>  }\n\nKill the \"pre-receive\" process, so the calling of\n\"finish_command(&proc)\" at the end of \"run_and_feed_hook()\" will\nterminate \"receive-pack\".\n\n> diff --git a/sideband.c b/sideband.c\n> index 85bddfdcd4f5..27f8d653eb24 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> +static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,\n> +                             int packet_max, int ignore_errors)\n>  {\n>         const char *p = data;\n>\n> @@ -265,13 +279,24 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n>                 if (0 <= band) {\n>                         xsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n>                         hdr[4] = band;\n> -                       write_or_die(fd, hdr, 5);\n> +                       if (ignore_errors)\n\n\"ignore_errors\" or \"die_on_errors\"?\n\n> +                               write_or_die(fd, hdr, 5);\n> +                       else if (write_in_full(fd, hdr, 5) < 0)\n> +                               return -1;\n\n--\nJiang Xin\n"},{"id":"446941","messageId":"CHFM74053TIA.3G3CIXQYDMDXS@diabtop","threadId":"57305","inReplyTo":"CANYiYbGRK0eshjUJoPH0yWT1tVLoerOMC6CY6tAMrwAh7T+y1g@mail.gmail.com","subject":"Re: [PATCH] receive-pack: interrupt pre-receive when client disconnects","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-26T12:46:00Z","receivedAt":"2022-01-26T12:46:06Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Jiang Xin, Jan 26, 2022 at 08:17:\n> We used to ignore the SIGPIPE signal when calling \"pre-receive\" hook,\n> so we could tolerant a buggy \"pre-receive\" implementation which didn't\n> consume all the input from \"receive-pack\". On the other side, \"ctrl-c\"\n> from the client side will terminate \"receive-pack\", only if we do not\n> ignore the SIGPIPE signal when running \"pre-receive\".\n>\n> Wouldn't this be much simpler: add a new configuration variable\n> \"receive.loosePreReceiveImplementation\", and only ignore SIGPIPE when\n> \"receive-pack\" turns off the config variable?\n\nI had not thought of this. Yes it would be much simpler. I'll prepare\nanother patch with this approach.\n\nThanks!\n"},{"id":"447001","messageId":"20220126214438.3066132-1-robin.jarry@6wind.com","threadId":"57305","inReplyTo":"20220125095445.1796938-1-robin.jarry@6wind.com","subject":"[PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-26T21:44:37Z","receivedAt":"2022-01-26T21:44:56Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"When hitting ctrl-c on the client while a remote pre-receive hook is\nrunning, receive-pack is not killed by SIGPIPE because the signal is\nignored. This is a side effect of commit ec7dbd145bd8 (receive-pack:\nallow hooks to ignore its standard input stream).\n\nThe pre-receive hook itself is not interrupted and does not receive any\nerror since its stdout is a pipe which is read in an async thread and\noutput back to the client socket in a side band channel.\n\nAfter the pre-receive has exited the SIGPIPE default handler is restored\nand if the hook did not report any error, objects are migrated from\ntemporary to permanent storage.\n\nThis can be confusing for most people and may even be considered a bug.\n\nAdd a new receive.strictPreReceiveImpl config option to *not* ignore\nSIGPIPE when running pre-receive. If set to true, and the hook output\ncannot be forwarded to the client, receive-pack will be killed via\nSIGPIPE and the push will be aborted. Add a signal handler to kill and\nreap the hook process before exiting. This option only affects\npre-receive.\n\nThis does not guarantee that all client disconnections will abort\na push. If there is no pre-receive hook or if it does not produce any\noutput, receive-pack will not be killed via SIGPIPE and the push will\ncomplete.\n\nSigned-off-by: Robin Jarry <robin.jarry@6wind.com>\n---\nv1 -> v2:\n  Changed approach following Jiang Xin advice. Adding an option makes\n  more sense and also makes a much simpler patch.\n\n Documentation/config/receive.txt | 13 +++++++++++++\n builtin/receive-pack.c           | 26 +++++++++++++++++++++++++-\n 2 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt\nindex 85d5b5a3d2d8..7174168541dc 100644\n--- a/Documentation/config/receive.txt\n+++ b/Documentation/config/receive.txt\n@@ -143,3 +143,16 @@ receive.updateServerInfo::\n receive.shallowUpdate::\n \tIf set to true, .git/shallow can be updated when new refs\n \trequire new shallow roots. Otherwise those refs are rejected.\n+\n+receive.strictPreReceiveImpl::\n+\tIf a pre-receive hook does not consume its standard input fully, it may\n+\tkill receive-pack via SIGPIPE. This can lead to obscure push failures.\n+\tTo avoid potential death-by-SIGPIPE due to poorly written hooks,\n+\treceive-pack ignores SIGPIPE while running the pre-receive hook.\n++\n+If this option is set to true, SIGPIPE will `not` be ignored by receive-pack\n+while running the \"pre-receive\" hook. This has a side-effect: If the hook\n+outputs something and the client has disconnected, receive-pack will be killed\n+and the push will be aborted.\n++\n+SIGPIPE is always ignored while running \"post-receive\".\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf9..8718a6dd91b4 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -74,6 +74,7 @@ static const char *head_name;\n static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n+static int strict_pre_receive_impl;\n static const char *alt_shallow_file;\n static struct strbuf push_cert = STRBUF_INIT;\n static struct object_id push_cert_oid;\n@@ -219,6 +220,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.strictprereceiveimpl\") == 0) {\n+\t\tstrict_pre_receive_impl = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.certnonceseed\") == 0)\n \t\treturn git_config_string(&cert_nonce_seed, var, value);\n \n@@ -800,6 +806,19 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t}\n }\n \n+static volatile pid_t hook_pid;\n+\n+static void kill_hook(int signum)\n+{\n+\tif (hook_pid != 0) {\n+\t\tkill(hook_pid, signum);\n+\t\twaitpid(hook_pid, NULL, 0);\n+\t\thook_pid = 0;\n+\t}\n+\tsigchain_pop(signum);\n+\traise(signum);\n+}\n+\n struct receive_hook_feed_state {\n \tstruct command *cmd;\n \tstruct ref_push_report *report;\n@@ -858,7 +877,11 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t\treturn code;\n \t}\n \n-\tsigchain_push(SIGPIPE, SIG_IGN);\n+\thook_pid = proc.pid;\n+\tif (strict_pre_receive_impl && strcmp(hook_name, \"pre-receive\") == 0)\n+\t\tsigchain_push(SIGPIPE, kill_hook);\n+\telse\n+\t\tsigchain_push(SIGPIPE, SIG_IGN);\n \n \twhile (1) {\n \t\tconst char *buf;\n@@ -872,6 +895,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \tif (use_sideband)\n \t\tfinish_async(&muxer);\n \n+\thook_pid = 0;\n \tsigchain_pop(SIGPIPE);\n \n \treturn finish_command(&proc);\n-- \n2.35.0.1.g8273a50afc47\n\n"},{"id":"447033","messageId":"CANYiYbGME-=w4raiwW3w1_gHzVpsvdStz7xVpKqAwx2r_Vezzw@mail.gmail.com","threadId":"57305","inReplyTo":"20220126214438.3066132-1-robin.jarry@6wind.com","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-01-27T03:21:23Z","receivedAt":"2022-01-27T03:21:39Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Thu, Jan 27, 2022 at 10:03 AM Robin Jarry <robin.jarry@6wind.com> wrote:\n> @@ -800,6 +806,19 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n>         }\n>  }\n>\n> +static volatile pid_t hook_pid;\n\nCan we use a flag instead of hook_pid to distinguish the source of the\nSIGPIPE signal?\n1. \"pre-receive\" hook exits early without consuming stdin.\n2. \"pre-receive\" hook hangs after receiving commands from stdin, until\nclient quits by receiving a \"ctrl-c\".\n\n> +static void kill_hook(int signum)\n> +{\n> +       if (hook_pid != 0) {\n> +               kill(hook_pid, signum);\n> +               waitpid(hook_pid, NULL, 0);\n> +               hook_pid = 0;\n\nCan we let the signal handler in \"pre-receive\" to do it job? And we\ncan show some user friendly error message here. E.g.:\n\n    die(\"broken pipe: seems like the pre-receive hook exits early\nwithout consuming its stdin\");\n\n--\nJiang Xin\n"},{"id":"447037","messageId":"xmqqv8y54wxc.fsf@gitster.g","threadId":"57305","inReplyTo":"20220126214438.3066132-1-robin.jarry@6wind.com","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T04:36:31Z","receivedAt":"2022-01-27T04:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Jarry <robin.jarry@6wind.com> writes:\n\n> When hitting ctrl-c on the client while a remote pre-receive hook is\n> running, receive-pack is not killed by SIGPIPE because the signal is\n> ignored. This is a side effect of commit ec7dbd145bd8 (receive-pack:\n> allow hooks to ignore its standard input stream).\n\nI somehow feel that it is unrealistic to expect the command to be\nkilled via SIGPIPE because there is no guarantee that the command\nhas that many bytes to send out to to get the signal in the first\nplace.  Such an expectation is simply wrong, isn't it?\n\n> This can be confusing for most people and may even be considered a bug.\n\nSo, there is not much I see is confusing, and I expect \"most people\"\nwould not get confused or consider it a bug.  Killing a local\nprocess may or may not have any immediate effect on what happens on\nthe other side of the connection.\n\nOn the other hand, the SIGPIPE death by a poorly written pre-receive\nhook was a source of real confusion.  The pushing end cannot do\nanything about it to fix if the hook disconnected before reading all\nof the proposed updates.\n\n> Add a new receive.strictPreReceiveImpl config option to *not* ignore\n\nI guess that the receiving end must know if its hook is loosely written\nor not, so having a knob to revert to the older mode of operation\nmay probably be OK.\n\nDo not abbreviate \"Implementation\" in the name of a configuration\nvariable, if that is the word you meant, by the way.  We try to\nspell things out for clarity.\n\nAlso, \"strict implementation\" is way too vague.  What you want to\nsay here is that the hook will not stop reading its input in the\nmiddle, causing the feeder to be killed by SIGPIPE, and from other\naspects its implementation may not be strict at all.\n\nA name that goes well with a statement \"This hook reads all of its\ninput\" would work much better.\n\nShould this cover only one hook, or should we introduce just one\nconfiguration to say \"all hooks that read from their standard input\nstream are clean and will read their input to the end\"?  Or do we\nneed to have N different variables for each of N hooks that may stop\nreading from their standard input in the middle (not necessarily\nlimited to the receive-pack command)?  I think there are a handful\nother hooks that take input from their standard input stream and I\nam not sure if pre-receive should be singled out like this.\n\nIf this Boolean \"This hook reads all of its input to the end\" is to\nbe added per hook, I suspect that the namespace of the configuration\nvariable should be coordinated with the other effort to \"define\" hooks\nin the configuration file(s) in the first place.  Emily, do you have\na suggestion?\n\n> +static volatile pid_t hook_pid;\n> +\n> +static void kill_hook(int signum)\n> +{\n> +\tif (hook_pid != 0) {\n> +\t\tkill(hook_pid, signum);\n> +\t\twaitpid(hook_pid, NULL, 0);\n> +\t\thook_pid = 0;\n\nIs it safe to kill(2) from within a signal handler?\n\nWhy does this patch do anything more than a partial reversion of\nec7dbd14 (receive-pack: allow hooks to ignore its standard input\nstream, 2014-09-12), i.e. \"if the configuration says do not be\nlenient to hooks that do not consume their input, do not ignore\nsigpipe at all\".\n\n> +\t}\n> +\tsigchain_pop(signum);\n> +\traise(signum);\n> +}\n> +\n>  struct receive_hook_feed_state {\n>  \tstruct command *cmd;\n>  \tstruct ref_push_report *report;\n> @@ -858,7 +877,11 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n>  \t\treturn code;\n>  \t}\n>  \n> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\thook_pid = proc.pid;\n> +\tif (strict_pre_receive_impl && strcmp(hook_name, \"pre-receive\") == 0)\n> +\t\tsigchain_push(SIGPIPE, kill_hook);\n> +\telse\n> +\t\tsigchain_push(SIGPIPE, SIG_IGN);\n>  \n>  \twhile (1) {\n>  \t\tconst char *buf;\n> @@ -872,6 +895,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n>  \tif (use_sideband)\n>  \t\tfinish_async(&muxer);\n>  \n> +\thook_pid = 0;\n>  \tsigchain_pop(SIGPIPE);\n>  \n>  \treturn finish_command(&proc);\n"},{"id":"447056","messageId":"CHGBKD7TF1S5.3VUMATFQPY9TE@diabtop","threadId":"57305","inReplyTo":"CANYiYbGME-=w4raiwW3w1_gHzVpsvdStz7xVpKqAwx2r_Vezzw@mail.gmail.com","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-27T08:38:47Z","receivedAt":"2022-01-27T08:38:51Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Jiang Xin, Jan 27, 2022 at 04:21:\n> Can we use a flag instead of hook_pid to distinguish the source of the\n> SIGPIPE signal?\n> 1. \"pre-receive\" hook exits early without consuming stdin.\n> 2. \"pre-receive\" hook hangs after receiving commands from stdin, until\n> client quits by receiving a \"ctrl-c\".\n\nAlso there is:\n\n3. the client has exited and receive-pack got SIGPIPE while forwarding\n   pre-receive output in the socket.\n\nI don't think we can differentiate from these three situations from the\nreceive-pack point of view.\n\nHowever, using a flag in the signal handler to note that SIGPIPE was\nreceived (for whatever reason) may be better than my current\nimplementation.\n\n> Can we let the signal handler in \"pre-receive\" to do it job? And we\n> can show some user friendly error message here. E.g.:\n>\n>     die(\"broken pipe: seems like the pre-receive hook exits early\n> without consuming its stdin\");\n\nIf that flag is set after pre-receive has exited, we can indeed:\n\n    die(\"broken pipe: ...\").\n\nOf course, if 3. the error message will never reach the client.\n"},{"id":"447057","messageId":"CHGCP9P33XDQ.3FEWHU0PBMNU6@diabtop","threadId":"57305","inReplyTo":"xmqqv8y54wxc.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-27T09:32:12Z","receivedAt":"2022-01-27T09:32:16Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Junio C Hamano, Jan 27, 2022 at 05:36:\n> I somehow feel that it is unrealistic to expect the command to be\n> killed via SIGPIPE because there is no guarantee that the command\n> has that many bytes to send out to to get the signal in the first\n> place.  Such an expectation is simply wrong, isn't it?\n\nMaybe I did not word that properly. Indeed, this only applies if\npre-receive has bytes to send out in the first place. This is what\nI referred to with the last paragraph:\n\n> > This does not guarantee that all client disconnections will abort\n> > a push. If there is no pre-receive hook or if it does not produce\n> > any output, receive-pack will not be killed via SIGPIPE and the push\n> > will complete.\n\nIt would be much better not to rely on pre-receive to have bytes to send\nand to expect that receive-pack will receive SIGPIPE when forwarding\nthem after the client has disconnected.\n\nI thought of sending a \"keepalive packet\" in the socket *after* the\npre-receive hook has completed. I do not know the protocol details.\nWould something like this be suitable:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 8718a6dd91b4..2e0ddd1a59fe 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1990,16 +1990,28 @@ static void execute_commands(struct command *commands,\n \tif (run_receive_hook(commands, \"pre-receive\", 0, push_options)) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\t\tif (!cmd->error_string)\n \t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n \t\t}\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Send a keepalive packet to ensure that the client has not\n+\t * disconnected while pre-receive was running.\n+\t */\n+\t{\n+\t\tstatic const char buf[] = \"0001\";\n+\t\tif (use_sideband)\n+\t\t\tsend_sideband(1, 1, buf, sizeof(buf) - 1, use_sideband);\n+\t\telse\n+\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n+\t}\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\n \t */\n \tif (tmp_objdir_migrate(tmp_objdir) < 0) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\t\tif (!cmd->error_string)\n \t\t\t\tcmd->error_string = \"unable to migrate objects to permanent storage\";\n\nIn that situation, if the client has exited, receive-pack should be\nkilled via SIGPIPE before completing the push.\n\n> Is it safe to kill(2) from within a signal handler?\n\nEven if it is, it is probably not a good idea. I did that to avoid\nleaving a zombie after receive-pack has died. Maybe setting a flag in\nthe signal handler and checking the flag after the process has exited\nwould have been better.\n\n> Why does this patch do anything more than a partial reversion of\n> ec7dbd14 (receive-pack: allow hooks to ignore its standard input\n> stream, 2014-09-12), i.e. \"if the configuration says do not be\n> lenient to hooks that do not consume their input, do not ignore\n> sigpipe at all\".\n\nIndeed it is a partial reversion of that commit. Maybe the \"keepalive\nbefore migrating to permanent storage\" solution is better.\n\nWhat do you think?\n"},{"id":"447106","messageId":"xmqqr18t2fxl.fsf@gitster.g","threadId":"57305","inReplyTo":"CHGCP9P33XDQ.3FEWHU0PBMNU6@diabtop","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T18:26:30Z","receivedAt":"2022-01-27T18:26:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" <robin.jarry@6wind.com> writes:\n\n> Indeed it is a partial reversion of that commit. Maybe the \"keepalive\n> before migrating to permanent storage\" solution is better.\n>\n> What do you think?\n\nSorry, but I was (and am) questioning why we want to do more than\n\"let it be killed by SIGPIPE, just like we used to do before\nec7dbd14 (receive-pack: allow hooks to ignore its standard input\nstream, 2014-09-12) introduced the current behaviour\", so the answer\nis still \"why do we even need to complicate the thing with keepalive\nor anything we don't have, and we didn't have before ec7dbd14, in\nthe code paths that are involved?\"\n"},{"id":"447128","messageId":"CHGR6XNP6TV7.15VGVNQUJM9J6@diabtop","threadId":"57305","inReplyTo":"xmqqr18t2fxl.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-27T20:53:32Z","receivedAt":"2022-01-27T20:53:36Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Junio C Hamano, Jan 27, 2022 at 19:26:\n> Sorry, but I was (and am) questioning why we want to do more than\n> \"let it be killed by SIGPIPE, just like we used to do before\n> ec7dbd14 (receive-pack: allow hooks to ignore its standard input\n> stream, 2014-09-12) introduced the current behaviour\", so the answer\n> is still \"why do we even need to complicate the thing with keepalive\n> or anything we don't have, and we didn't have before ec7dbd14, in\n> the code paths that are involved?\"\n\nMy main goal is to abort a push if a user hits ctrl-c (or is\ndisconnected) before the objects have been moved to permanent storage.\n\n(partially) reverting to previous behavior would only allow aborting\npushes *if* the pre-receive hook sends some output and this output\ncannot be forwarded to the client. There is no guarantee that the hook\nwill send any output. Also, it would restore hard to track issue with\npoorly written pre-receive hooks.\n\nI wonder if it would be possible to not rely on the pre-receive hook\nsending output and this output somehow not being forwarded to the\nclient.\n\nInstead, explicitly check if the client is still connected and alive\nafter the pre-receive hook has exited but before completing the push\ntransaction. That was my intent with that (invalid by the way) keepalive\nexample.\n\nI do not know git internals to say if it feasible without any protocol\nbreakage. My attempts work well for aborting pushes:\n\n Writing objects: 100% (3/3), 321 bytes | 321.00 KiB/s, done.\n Total 3 (delta 1), reused 1 (delta 0), pack-reused 0\n remote: pre-receive start^C\n <--- client has disconnected\n      receive-pack fails to send the \"keepalive\" packet\n      the temp objects are *not* migrated to permanent storage\n\nBut this always leads to errors on the client side when receive-pack\nsends the \"keepalive packet\":\n\n Writing objects: 100% (3/3), 321 bytes | 321.00 KiB/s, done.\n Total 3 (delta 1), reused 1 (delta 0), pack-reused 0\n remote: pre-receive start\n remote: pre-receive end OK\n error: unexpected flush packet while reading remote unpack status\n error: invalid ref status from remote: unpack\n remote: post-receive start\n remote: post-receive end OK\n To git@host:repo.git\n  ! [remote failure]    main -> main (remote failed to report status)\n error: failed to push some refs to 'git@host:repo.git'\n\nAm I chasing rainbows or is that possible in the current state of the\ngit protocol? Maybe I need to send the keepalive packet in a sideband?\nI have read the technical docs several times but I cannot understand how\neverything works properly.\n\nThank you for your time.\n"},{"id":"447138","messageId":"20220127215553.1386024-1-robin.jarry@6wind.com","threadId":"57305","inReplyTo":"CHGR6XNP6TV7.15VGVNQUJM9J6@diabtop","subject":"[PATCH v3] receive-pack: check if client is alive before completing the push","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-27T21:55:53Z","receivedAt":"2022-01-27T21:56:00Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Abort the push operation (i.e. do not migrate the objects from temporary\nto permanent storage) if the client has disconnected while the\npre-receive hook was running.\n\nThis reduces the risk of inconsistencies on network errors or if the\nuser hits ctrl-c while the pre-receive hook is running.\n\nSend a keepalive packet (empty) on sideband 2 (the one to report\nprogress). If the client has exited, receive-pack will be killed via\nSIGPIPE and the push will be aborted. This only works when sideband*\ncapabilities are advertised by the client.\n\nSigned-off-by: Robin Jarry <robin.jarry@6wind.com>\n---\nv2 -> v3:\n    I had missed Documentation/technical/pack-protocol.txt. Using\n    sideband 2 to send the keepalive packet works.\n\n builtin/receive-pack.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf9..8b0d56897c9f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Send a keepalive packet on sideband 2 (progress info) to ensure that\n+\t * the client has not disconnected while pre-receive was running.\n+\t */\n+\tif (use_sideband) {\n+\t\tstatic const char buf[] = \"0005\\2\";\n+\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n+\t}\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\n-- \n2.35.0.4.gfdf4c72cdf3d\n\n"},{"id":"447147","messageId":"xmqqa6fgzqp5.fsf@gitster.g","threadId":"57305","inReplyTo":"CHGR6XNP6TV7.15VGVNQUJM9J6@diabtop","subject":"Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T23:47:34Z","receivedAt":"2022-01-27T23:47:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" <robin.jarry@6wind.com> writes:\n\n> My main goal is to abort a push if a user hits ctrl-c (or is\n> disconnected) before the objects have been moved to permanent storage.\n>\n> But this always leads to errors on the client side when receive-pack\n> sends the \"keepalive packet\":\n\nYes, you'd need to make all three new combinations work if you touch\nthe protocol.  An updated \"receive-pack\" must be inter-operable with\na vanilla \"push\" as well as an updated \"push\", and an updated \"push\"\nmust be inter-operable with a vanilla \"receive-pack\".\n\nYou'd need to invent a new protocol capability, advertise it on the\nupdated \"receive-pack\" side, and the updated \"push\" must check if\nthe capability is advertized before asking to activate it.  Then\nonly after both ends discover that the other side knows how to deal\nwith \"keepalive\" packets, use that feature.\n\n"},{"id":"447162","messageId":"xmqqy230y7vc.fsf@gitster.g","threadId":"57305","inReplyTo":"20220127215553.1386024-1-robin.jarry@6wind.com","subject":"Re: [PATCH v3] receive-pack: check if client is alive before completing the push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-28T01:19:35Z","receivedAt":"2022-01-28T01:19:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Jarry <robin.jarry@6wind.com> writes:\n\n> Abort the push operation (i.e. do not migrate the objects from temporary\n> to permanent storage) if the client has disconnected while the\n> pre-receive hook was running.\n>\n> This reduces the risk of inconsistencies on network errors or if the\n> user hits ctrl-c while the pre-receive hook is running.\n>\n> Send a keepalive packet (empty) on sideband 2 (the one to report\n> progress). If the client has exited, receive-pack will be killed via\n> SIGPIPE and the push will be aborted. This only works when sideband*\n> capabilities are advertised by the client.\n>\n> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>\n> ---\n> v2 -> v3:\n>     I had missed Documentation/technical/pack-protocol.txt. Using\n>     sideband 2 to send the keepalive packet works.\n\nYes, as long as sideband capability is supported (which is true\neverywhere these days), this would be good.\n\nSimple and sensible.\n\nThanks.\n\n\n\n>  builtin/receive-pack.c | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf9..8b0d56897c9f 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n>  \t\treturn;\n>  \t}\n>  \n> +\t/*\n> +\t * Send a keepalive packet on sideband 2 (progress info) to ensure that\n> +\t * the client has not disconnected while pre-receive was running.\n> +\t */\n> +\tif (use_sideband) {\n> +\t\tstatic const char buf[] = \"0005\\2\";\n> +\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n> +\t}\n> +\n>  \t/*\n>  \t * Now we'll start writing out refs, which means the objects need\n>  \t * to be in their final positions so that other processes can see them.\n"},{"id":"447177","messageId":"CHH6X4XFLKXQ.3KKGJCADRTZZ7@diabtop","threadId":"57305","inReplyTo":"xmqqy230y7vc.fsf@gitster.g","subject":"Re: [PATCH v3] receive-pack: check if client is alive before completing the push","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-28T09:13:02Z","receivedAt":"2022-01-28T09:13:07Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Junio C Hamano, Jan 28, 2022 at 02:19:\n> Yes, as long as sideband capability is supported (which is true\n> everywhere these days), this would be good.\n>\n> Simple and sensible.\n>\n> Thanks.\n\nThank you for your patience :)\n"},{"id":"447227","messageId":"xmqq4k5nychf.fsf@gitster.g","threadId":"57305","inReplyTo":"20220127215553.1386024-1-robin.jarry@6wind.com","subject":"Re: [PATCH v3] receive-pack: check if client is alive before completing the push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-28T17:52:12Z","receivedAt":"2022-01-28T17:52:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Jarry <robin.jarry@6wind.com> writes:\n\n> Abort the push operation (i.e. do not migrate the objects from temporary\n> to permanent storage) if the client has disconnected while the\n> pre-receive hook was running.\n>\n> This reduces the risk of inconsistencies on network errors or if the\n> user hits ctrl-c while the pre-receive hook is running.\n>\n> Send a keepalive packet (empty) on sideband 2 (the one to report\n> progress). If the client has exited, receive-pack will be killed via\n> SIGPIPE and the push will be aborted. This only works when sideband*\n> capabilities are advertised by the client.\n\nIf they have already exited but the fact hasn't reached us over the\nnetwork, the write() will succeed to deposit the packet in the send\nbuffer.  So I am not sure how much this would actually help, but it\nshould be safe to send an unsolicited keepalive as long as the other\nside is expecting to hear from us.  When either report_status or\nreport_status_v2 capabilities is in effect, we will make a report()\nor report_v2() call later, so we should be safe.\n\n> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>\n> ---\n> v2 -> v3:\n>     I had missed Documentation/technical/pack-protocol.txt. Using\n>     sideband 2 to send the keepalive packet works.\n>\n>  builtin/receive-pack.c | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf9..8b0d56897c9f 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,\n>  \t\treturn;\n>  \t}\n>  \n> +\t/*\n> +\t * Send a keepalive packet on sideband 2 (progress info) to ensure that\n> +\t * the client has not disconnected while pre-receive was running.\n> +\t */\n\nI suspect that any keepalive, unless it expects an active \"yes, I am\nstill alive\" response from the other side, is too weak to \"ensure\".\n\nI guess \"to notice a client that has disconnected (e.g. killed with\n^C)\" is more appropriate.\n\n> +\tif (use_sideband) {\n> +\t\tstatic const char buf[] = \"0005\\2\";\n> +\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n> +\t}\n\nObserving how execute_commands() and helper functions report an\nerror to the callers higher in the call chain, and ask them to abort\nthe remainder of the operation, I am not sure if write_or_die() is\nappropriate.\n\n    Side note: inside copy_to_sideband(), which runs in async, it is\n    a different matter (i.e. the main process on our side is not\n    what gets killed by that _or_die() part of the call), but this\n    one kills the main process.\n\nThe convention around this code path seems to be to fill explanation\nof error in cmd->error_string and return to the caller.  In this\ncase, the error_strings may not reach the pusher via report() or\nreport_v2() as they may have disconnected, but calling the report()\nfunctions is not the only thing the caller will want to do after\ncalling us, so giving it a chance to clean up may be a better\ndesign, e.g.\n\n\tif (write_in_full(...) < 0) {\n\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n\t        \tcmd->error_string = \"pusher went away\";\n\t\treturn;\n\t}\n\nYes, the current code will not actually use the error string in any\nuseful way in this particular case, since report() or report_v2()\nwill have nobody listening to them.  But being consistent will help\nmaintaining the caller, as it can later be extended to use it\nlocally (e.g. log the request and its outcome, check which cmd has\nsucceeded and failed using the NULL-ness of cmd->error_string, etc.)\n"},{"id":"447232","messageId":"CHHK3G8H9D1X.23YTAHXI55311@diabtop","threadId":"57305","inReplyTo":"xmqq4k5nychf.fsf@gitster.g","subject":"Re: [PATCH v3] receive-pack: check if client is alive before completing the push","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-28T19:32:31Z","receivedAt":"2022-01-28T19:32:36Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Junio C Hamano, Jan 28, 2022 at 18:52:\n> If they have already exited but the fact hasn't reached us over the\n> network, the write() will succeed to deposit the packet in the send\n> buffer.  So I am not sure how much this would actually help, but it\n> should be safe to send an unsolicited keepalive as long as the other\n> side is expecting to hear from us.  When either report_status or\n> report_status_v2 capabilities is in effect, we will make a report()\n> or report_v2() call later, so we should be safe.\n\nThis is not perfect but I think this is the best we can do without\nadding a new capability so that the client sends a reply to the\nkeepalive packet.\n\n> I suspect that any keepalive, unless it expects an active \"yes, I am\n> still alive\" response from the other side, is too weak to \"ensure\".\n>\n> I guess \"to notice a client that has disconnected (e.g. killed with\n> ^C)\" is more appropriate.\n\nOK, I will change that.\n\n> > +\tif (use_sideband) {\n> > +\t\tstatic const char buf[] = \"0005\\2\";\n> > +\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n> > +\t}\n>\n> Observing how execute_commands() and helper functions report an\n> error to the callers higher in the call chain, and ask them to abort\n> the remainder of the operation, I am not sure if write_or_die() is\n> appropriate.\n>\n>     Side note: inside copy_to_sideband(), which runs in async, it is\n>     a different matter (i.e. the main process on our side is not\n>     what gets killed by that _or_die() part of the call), but this\n>     one kills the main process.\n>\n> The convention around this code path seems to be to fill explanation\n> of error in cmd->error_string and return to the caller.  In this\n> case, the error_strings may not reach the pusher via report() or\n> report_v2() as they may have disconnected, but calling the report()\n> functions is not the only thing the caller will want to do after\n> calling us, so giving it a chance to clean up may be a better\n> design, e.g.\n>\n> \tif (write_in_full(...) < 0) {\n> \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n> \t        \tcmd->error_string = \"pusher went away\";\n> \t\treturn;\n> \t}\n>\n> Yes, the current code will not actually use the error string in any\n> useful way in this particular case, since report() or report_v2()\n> will have nobody listening to them.  But being consistent will help\n> maintaining the caller, as it can later be extended to use it\n> locally (e.g. log the request and its outcome, check which cmd has\n> succeeded and failed using the NULL-ness of cmd->error_string, etc.)\n\nThe main receive-pack process will be killed by SIGPIPE anyway but I can\nfill the error_string fields and return for code consistency.\n\nI'll send a v4, thanks for the review.\n"},{"id":"447237","messageId":"20220128194811.3396281-1-robin.jarry@6wind.com","threadId":"57305","inReplyTo":"20220127215553.1386024-1-robin.jarry@6wind.com","subject":"[PATCH v4] receive-pack: check if client is alive before completing the push","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-01-28T19:48:11Z","receivedAt":"2022-01-28T19:48:17Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Abort the push operation (i.e. do not migrate the objects from temporary\nto permanent storage) if the client has disconnected while the\npre-receive hook was running.\n\nThis reduces the risk of inconsistencies on network errors or if the\nuser hits ctrl-c while the pre-receive hook is running.\n\nSend a keepalive packet (empty) on sideband 2 (the one to report\nprogress). If the client has exited the write() operation should fail\nand the push will be aborted. This only works when sideband*\ncapabilities are advertised by the client.\n\nNote: if the write() operation fails, receive-pack will likely be killed\nvia SIGPIPE and even so, since the client is likely gone already, the\nerror strings will go nowhere. I only added them for code consistency.\n\nSigned-off-by: Robin Jarry <robin.jarry@6wind.com>\n---\nv3 -> v4:\n  - reworded the comment block s/ensure/notice/\n  - used write_in_full() instead of write_or_die()\n  - set error_string fields for code consistency\n\n builtin/receive-pack.c | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf9..f8b9a9312733 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1971,6 +1971,22 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Send a keepalive packet on sideband 2 (progress info) to notice\n+\t * a client that has disconnected (e.g. killed with ^C) while\n+\t * pre-receive was running.\n+\t */\n+\tif (use_sideband) {\n+\t\tstatic const char buf[] = \"0005\\2\";\n+\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n+\t\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\t\tif (!cmd->error_string)\n+\t\t\t\t\tcmd->error_string = \"pusher went away\";\n+\t\t\t}\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n \t/*\n \t * Now we'll start writing out refs, which means the objects need\n \t * to be in their final positions so that other processes can see them.\n-- \n2.35.0.4.g44a5d4affccf\n\n"},{"id":"447714","messageId":"220204.864k5e4yvf.gmgdl@evledraar.gmail.com","threadId":"57305","inReplyTo":"20220128194811.3396281-1-robin.jarry@6wind.com","subject":"Re: [PATCH v4] receive-pack: check if client is alive before completing the push","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-04T11:37:23Z","receivedAt":"2022-02-04T12:09:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 28 2022, Robin Jarry wrote:\n\n> Abort the push operation (i.e. do not migrate the objects from temporary\n> to permanent storage) if the client has disconnected while the\n> pre-receive hook was running.\n>\n> This reduces the risk of inconsistencies on network errors or if the\n> user hits ctrl-c while the pre-receive hook is running.\n>\n> Send a keepalive packet (empty) on sideband 2 (the one to report\n> progress). If the client has exited the write() operation should fail\n> and the push will be aborted. This only works when sideband*\n> capabilities are advertised by the client.\n>\n> Note: if the write() operation fails, receive-pack will likely be killed\n> via SIGPIPE and even so, since the client is likely gone already, the\n> error strings will go nowhere. I only added them for code consistency.\n>\n> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>\n> ---\n> v3 -> v4:\n>   - reworded the comment block s/ensure/notice/\n>   - used write_in_full() instead of write_or_die()\n>   - set error_string fields for code consistency\n>\n>  builtin/receive-pack.c | 16 ++++++++++++++++\n>  1 file changed, 16 insertions(+)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf9..f8b9a9312733 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1971,6 +1971,22 @@ static void execute_commands(struct command *commands,\n>  \t\treturn;\n>  \t}\n>  \n> +\t/*\n> +\t * Send a keepalive packet on sideband 2 (progress info) to notice\n> +\t * a client that has disconnected (e.g. killed with ^C) while\n> +\t * pre-receive was running.\n> +\t */\n> +\tif (use_sideband) {\n> +\t\tstatic const char buf[] = \"0005\\2\";\n> +\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n> +\t\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> +\t\t\t\tif (!cmd->error_string)\n> +\t\t\t\t\tcmd->error_string = \"pusher went away\";\n> +\t\t\t}\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n>  \t/*\n>  \t * Now we'll start writing out refs, which means the objects need\n>  \t * to be in their final positions so that other processes can see them.\n\nI've read the upthread, but I still don't quite get why it's a must to\nunconditionally abort the push because the pusher went away.\n\nAt this point we've passed the pre-receive hook, are about to migrate\nthe objects, still have proc-receive left to run, and finally will\nupdate the refs.\n\nIs the motivation purely a UX change where it's considered that the user\n*must* be shown the output, or are we doing the wrong thing and not\ncontinuing at all if we run into SIGPIPE here (then presumably only for\nhooks that produce output?).\n\nI admit this is somewhat contrived, but aren't we now doing worse for\nusers where the pre-receive hook takes 10s, but they already asked for\ntheir push to be performed. Then they disconnect from WiFi unexpectedly,\nand find that that it didn't go through?\n\nAnyway, I see you made this opt-in configurable in earlier iterations. I\nwonder if that's still something worth doing, or if we should just take\nthis change as-is.\n\nWhat I don't get is *if* we're doing this for the UX reason why are we\nsingling out the pre-receive hook in particular, and not covering\nproc-receive? I.e. we'll also produce output the user might see there,\nas you can see with this ad-hoc testing change (showhing changed \"git\npush\" output when I add to the hook output):\n\n\tdiff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c\n\tindex cc08506cf0b..933f0599497 100644\n\t--- a/t/helper/test-proc-receive.c\n\t+++ b/t/helper/test-proc-receive.c\n\t@@ -188,6 +188,7 @@ int cmd__proc_receive(int argc, const char **argv)\n\t                if (returns.nr)\n\t                        for_each_string_list_item(item, &returns)\n\t                                fprintf(stderr, \"proc-receive> %s\\n\", item->string);\n\t+               fprintf(stderr, \"showing a custom message\\n\");\n\t        }\n\t \n\t        if (die_write_report)\n\n\t$ ./t5411-proc-receive-hook.sh --run=1-3,5-42 -vixd\n\t[...]\n\t+ diff -u expect actual\n\t--- expect      2022-02-04 11:53:52.006413296 +0000\n\t+++ actual      2022-02-04 11:53:52.006413296 +0000\n\t@@ -3,6 +3,7 @@\n\t remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        \n\t remote: # proc-receive hook        \n\t remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        \n\t+remote: showing a custom message        \n\t remote: # post-receive hook        \n\t remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next        \n\t To <URL/of/upstream.git>\n\terror: last command exited with $?=1\n\nIs the unstated reason that we consider the tmp_objdir_migrate() more of\na a point of no return?\n\nIOW I'm wondering why it doesn't look more like this (the object\nmigration could probably be dropped, it should be near-ish instant, but\nproc-receive can take a long time):\n\n\tdiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n\tindex f8b9a931273..33bbafbc9e2 100644\n\t--- a/builtin/receive-pack.c\n\t+++ b/builtin/receive-pack.c\n\t@@ -1907,6 +1907,26 @@ static void execute_commands_atomic(struct command *commands,\n\t \tstrbuf_release(&err);\n\t }\n\t \n\t+static int pusher_went_away(struct command *commands, const char *msg)\n\t+{\n\t+\tstruct command *cmd;\n\t+\tstatic const char buf[] = \"0005\\2\";\n\t+\n\t+\t/*\n\t+\t * Send a keepalive packet on sideband 2 (progress info) to notice\n\t+\t * a client that has disconnected (e.g. killed with ^C) while\n\t+\t * pre-receive was running.\n\t+\t */\n\t+\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n\t+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n\t+\t\t\tif (!cmd->error_string)\n\t+\t\t\t\tcmd->error_string = msg;\n\t+\t\t}\n\t+\t\treturn 1;\n\t+\t}\n\t+\treturn 0;\n\t+}\n\t+\n\t static void execute_commands(struct command *commands,\n\t \t\t\t     const char *unpacker_error,\n\t \t\t\t     struct shallow_info *si,\n\t@@ -1971,21 +1991,9 @@ static void execute_commands(struct command *commands,\n\t \t\treturn;\n\t \t}\n\t \n\t-\t/*\n\t-\t * Send a keepalive packet on sideband 2 (progress info) to notice\n\t-\t * a client that has disconnected (e.g. killed with ^C) while\n\t-\t * pre-receive was running.\n\t-\t */\n\t-\tif (use_sideband) {\n\t-\t\tstatic const char buf[] = \"0005\\2\";\n\t-\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n\t-\t\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n\t-\t\t\t\tif (!cmd->error_string)\n\t-\t\t\t\t\tcmd->error_string = \"pusher went away\";\n\t-\t\t\t}\n\t-\t\t\treturn;\n\t-\t\t}\n\t-\t}\n\t+\tif (use_sideband && pusher_went_away(commands,\n\t+\t\t\t\t\t     \"pusher can't be contacted post-pre-receive\"))\n\t+\t\treturn;\n\t \n\t \t/*\n\t \t * Now we'll start writing out refs, which means the objects need\n\t@@ -2000,6 +2008,10 @@ static void execute_commands(struct command *commands,\n\t \t}\n\t \ttmp_objdir = NULL;\n\t \n\t+\tif (use_sideband && pusher_went_away(commands,\n\t+\t\t\t\t\t     \"pusher can't be contacted post-object migration\"))\n\t+\t\treturn;\n\t+\n\t \tcheck_aliased_updates(commands);\n\t \n\t \tfree(head_name_to_free);\n\t@@ -2013,6 +2025,10 @@ static void execute_commands(struct command *commands,\n\t \t\t\t    (cmd->run_proc_receive || use_atomic))\n\t \t\t\t\tcmd->error_string = \"fail to run proc-receive hook\";\n\t \n\t+\tif (use_sideband && pusher_went_away(commands,\n\t+\t\t\t\t\t     \"pusher can't be contacted post-proc-receive\"))\n\t+\t\treturn;\n\t+\n\t \tif (use_atomic)\n\t \t\texecute_commands_atomic(commands, si);\n\t \telse\n\nBut also, this whole thing is \"if the pre-receive hook etc. etc.\", but\nwe do in fact run this when there's no hook at all. See how this\ninteracts with run_and_feed_hook() and the \"!hook_path\" check.\n\nSo isn't this unnecessary if there's no such hook, and we should unfold\nthe find_hook() etc. from that codepath (or pass up a \"I ran the hook\"\nstate)?\n"},{"id":"447762","messageId":"xmqqczk2moc2.fsf@gitster.g","threadId":"57305","inReplyTo":"220204.864k5e4yvf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4] receive-pack: check if client is alive before completing the push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-04T19:19:41Z","receivedAt":"2022-02-04T19:19:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Is the motivation purely a UX change where it's considered that the user\n> *must* be shown the output, or are we doing the wrong thing and not\n> continuing at all if we run into SIGPIPE here (then presumably only for\n> hooks that produce output?).\n>\n> I admit this is somewhat contrived, but aren't we now doing worse for\n> users where the pre-receive hook takes 10s, but they already asked for\n> their push to be performed. Then they disconnect from WiFi unexpectedly,\n> and find that that it didn't go through?\n>\n> Anyway, I see you made this opt-in configurable in earlier iterations. I\n> wonder if that's still something worth doing, or if we should just take\n> this change as-is.\n\nI guess the above is exactly the same reaction I still have against\nthe series.  In a case where the user did *not* see \"git push\"\ncomplete after getting a positive response from the other side that\nsays the changes to refs have succeeded, due to whatever reason\n(e.g. \"^C\" or connection droppage), the user cannot expect whether\nthe push to have completed or got aborted, both from the UX point of\nview and from the correctness point of view, I would think.\n\nYour keyboard interrupt \"^C\" may have come too late to matter at the\nreceiving end, or your WiFi may or may not have disconnected before\nthe receiving end got everything necessary from you to carry out the\noperation, for example, and you are not simply in control of these\nthings.\n"},{"id":"447900","messageId":"CHQ28GCKBVQQ.1EGKW3Y5N4IMM@ringo","threadId":"57305","inReplyTo":"220204.864k5e4yvf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4] receive-pack: check if client is alive before completing the push","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2022-02-07T19:26:43Z","receivedAt":"2022-02-07T19:31:48Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Hi Ævar,\n\nÆvar Arnfjörð Bjarmason, Feb 04, 2022 at 12:37:\n> I've read the upthread, but I still don't quite get why it's a must to\n> unconditionally abort the push because the pusher went away.\n>\n> At this point we've passed the pre-receive hook, are about to migrate\n> the objects, still have proc-receive left to run, and finally will\n> update the refs.\n>\n> Is the motivation purely a UX change where it's considered that the user\n> *must* be shown the output, or are we doing the wrong thing and not\n> continuing at all if we run into SIGPIPE here (then presumably only for\n> hooks that produce output?).\n>\n> I admit this is somewhat contrived, but aren't we now doing worse for\n> users where the pre-receive hook takes 10s, but they already asked for\n> their push to be performed. Then they disconnect from WiFi unexpectedly,\n> and find that that it didn't go through?\n\nThis *is* purely motivated by UX.\n\npre-receive hooks may that perform various verifications. They may\nrequire connecting to a bug tracker to validate that the referenced\ntickets are in the proper state and associated with the proper git\nrepository. They may also run patch validation scripts such as\n[checkpatch.pl][1].\n\n[1]: https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html\n\nWhen pushing large series of commits, these checks can take\na significant amount of time. While the checks are running, some users\nmay change their mind and hit ctrl-c because they forgot something.\n\nOn their point of view, the operation seems to have been aborted.\nWhereas if the pre-receive hook completes without errors, the push will\nbe completed. This may be confusing to some.\n\n> Anyway, I see you made this opt-in configurable in earlier iterations. I\n> wonder if that's still something worth doing, or if we should just take\n> this change as-is.\n\nThe earlier iterations were a lot more complex and actually messed with\nSIGPIPE forwarding to the pre-receive hook itself. This last version is\nmuch simpler so I did not think about adding an option. I could make\nthis behaviour opt-in, I don't mind.\n\n> What I don't get is *if* we're doing this for the UX reason why are we\n> singling out the pre-receive hook in particular, and not covering\n> proc-receive? I.e. we'll also produce output the user might see there,\n> as you can see with this ad-hoc testing change (showhing changed \"git\n> push\" output when I add to the hook output):\n>\n> \tdiff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c\n> \tindex cc08506cf0b..933f0599497 100644\n> \t--- a/t/helper/test-proc-receive.c\n> \t+++ b/t/helper/test-proc-receive.c\n> \t@@ -188,6 +188,7 @@ int cmd__proc_receive(int argc, const char **argv)\n> \t                if (returns.nr)\n> \t                        for_each_string_list_item(item, &returns)\n> \t                                fprintf(stderr, \"proc-receive> %s\\n\", item->string);\n> \t+               fprintf(stderr, \"showing a custom message\\n\");\n> \t        }\n> \t \n> \t        if (die_write_report)\n>\n> \t$ ./t5411-proc-receive-hook.sh --run=1-3,5-42 -vixd\n> \t[...]\n> \t+ diff -u expect actual\n> \t--- expect      2022-02-04 11:53:52.006413296 +0000\n> \t+++ actual      2022-02-04 11:53:52.006413296 +0000\n> \t@@ -3,6 +3,7 @@\n> \t remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        \n> \t remote: # proc-receive hook        \n> \t remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic        \n> \t+remote: showing a custom message        \n> \t remote: # post-receive hook        \n> \t remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next        \n> \t To <URL/of/upstream.git>\n> \terror: last command exited with $?=1\n>\n> Is the unstated reason that we consider the tmp_objdir_migrate() more of\n> a a point of no return?\n\nI have almost zero experience with proc-receive. I had understood that\ntmp_objdir_migrate() meant that the push operation was \"complete\" in the\nsense that commits, tags, branches had been updated (regardless of\nproc-receive status). Maybe I am completely wrong.\n\n> IOW I'm wondering why it doesn't look more like this (the object\n> migration could probably be dropped, it should be near-ish instant, but\n> proc-receive can take a long time):\n>\n> \tdiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> \tindex f8b9a931273..33bbafbc9e2 100644\n> \t--- a/builtin/receive-pack.c\n> \t+++ b/builtin/receive-pack.c\n> \t@@ -1907,6 +1907,26 @@ static void execute_commands_atomic(struct command *commands,\n> \t \tstrbuf_release(&err);\n> \t }\n> \t \n> \t+static int pusher_went_away(struct command *commands, const char *msg)\n> \t+{\n> \t+\tstruct command *cmd;\n> \t+\tstatic const char buf[] = \"0005\\2\";\n> \t+\n> \t+\t/*\n> \t+\t * Send a keepalive packet on sideband 2 (progress info) to notice\n> \t+\t * a client that has disconnected (e.g. killed with ^C) while\n> \t+\t * pre-receive was running.\n> \t+\t */\n> \t+\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n> \t+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t+\t\t\tif (!cmd->error_string)\n> \t+\t\t\t\tcmd->error_string = msg;\n> \t+\t\t}\n> \t+\t\treturn 1;\n> \t+\t}\n> \t+\treturn 0;\n> \t+}\n> \t+\n> \t static void execute_commands(struct command *commands,\n> \t \t\t\t     const char *unpacker_error,\n> \t \t\t\t     struct shallow_info *si,\n> \t@@ -1971,21 +1991,9 @@ static void execute_commands(struct command *commands,\n> \t \t\treturn;\n> \t \t}\n> \t \n> \t-\t/*\n> \t-\t * Send a keepalive packet on sideband 2 (progress info) to notice\n> \t-\t * a client that has disconnected (e.g. killed with ^C) while\n> \t-\t * pre-receive was running.\n> \t-\t */\n> \t-\tif (use_sideband) {\n> \t-\t\tstatic const char buf[] = \"0005\\2\";\n> \t-\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0) {\n> \t-\t\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t-\t\t\t\tif (!cmd->error_string)\n> \t-\t\t\t\t\tcmd->error_string = \"pusher went away\";\n> \t-\t\t\t}\n> \t-\t\t\treturn;\n> \t-\t\t}\n> \t-\t}\n> \t+\tif (use_sideband && pusher_went_away(commands,\n> \t+\t\t\t\t\t     \"pusher can't be contacted post-pre-receive\"))\n> \t+\t\treturn;\n> \t \n> \t \t/*\n> \t \t * Now we'll start writing out refs, which means the objects need\n> \t@@ -2000,6 +2008,10 @@ static void execute_commands(struct command *commands,\n> \t \t}\n> \t \ttmp_objdir = NULL;\n> \t \n> \t+\tif (use_sideband && pusher_went_away(commands,\n> \t+\t\t\t\t\t     \"pusher can't be contacted post-object migration\"))\n> \t+\t\treturn;\n> \t+\n> \t \tcheck_aliased_updates(commands);\n> \t \n> \t \tfree(head_name_to_free);\n> \t@@ -2013,6 +2025,10 @@ static void execute_commands(struct command *commands,\n> \t \t\t\t    (cmd->run_proc_receive || use_atomic))\n> \t \t\t\t\tcmd->error_string = \"fail to run proc-receive hook\";\n> \t \n> \t+\tif (use_sideband && pusher_went_away(commands,\n> \t+\t\t\t\t\t     \"pusher can't be contacted post-proc-receive\"))\n> \t+\t\treturn;\n> \t+\n> \t \tif (use_atomic)\n> \t \t\texecute_commands_atomic(commands, si);\n> \t \telse\n>\n> But also, this whole thing is \"if the pre-receive hook etc. etc.\", but\n> we do in fact run this when there's no hook at all. See how this\n> interacts with run_and_feed_hook() and the \"!hook_path\" check.\n>\n> So isn't this unnecessary if there's no such hook, and we should unfold\n> the find_hook() etc. from that codepath (or pass up a \"I ran the hook\"\n> state)?\n\nYou're right, maybe this keepalive packet should only be sent if there\nis a pre-receive hook.\n\nAlso, if proc-receive can indeed reject the push operation, there should\nbe multiple \"checkpoints\" as you said.\n\nTo sum up, I can send a v5 with multiple \"checkpoints\" and only via an\nopt-in config option. Would that be OK?\n\nCheers\n"}]}