Message ID | 20220128194811.3396281-1-robin.jarry@6wind.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v4] receive-pack: check if client is alive before completing the push | expand |
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)?
Æ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.
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
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.
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(+)