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
