Junio C Hamano, Jan 28, 2022 at 18:52:
> If they have already exited but the fact hasn't reached us over the
> network, the write() will succeed to deposit the packet in the send
> buffer. So I am not sure how much this would actually help, but it
> should be safe to send an unsolicited keepalive as long as the other
> side is expecting to hear from us. When either report_status or
> report_status_v2 capabilities is in effect, we will make a report()
> or report_v2() call later, so we should be safe.
This is not perfect but I think this is the best we can do without
adding a new capability so that the client sends a reply to the
keepalive packet.
> I suspect that any keepalive, unless it expects an active "yes, I am
> still alive" response from the other side, is too weak to "ensure".
>
> I guess "to notice a client that has disconnected (e.g. killed with
> ^C)" is more appropriate.
OK, I will change that.
> > + if (use_sideband) {
> > + static const char buf[] = "0005\2";
> > + write_or_die(1, buf, sizeof(buf) - 1);
> > + }
>
> Observing how execute_commands() and helper functions report an
> error to the callers higher in the call chain, and ask them to abort
> the remainder of the operation, I am not sure if write_or_die() is
> appropriate.
>
> Side note: inside copy_to_sideband(), which runs in async, it is
> a different matter (i.e. the main process on our side is not
> what gets killed by that _or_die() part of the call), but this
> one kills the main process.
>
> The convention around this code path seems to be to fill explanation
> of error in cmd->error_string and return to the caller. In this
> case, the error_strings may not reach the pusher via report() or
> report_v2() as they may have disconnected, but calling the report()
> functions is not the only thing the caller will want to do after
> calling us, so giving it a chance to clean up may be a better
> design, e.g.
>
> if (write_in_full(...) < 0) {
> for (cmd = commands; cmd; cmd = cmd->next)
> cmd->error_string = "pusher went away";
> return;
> }
>
> Yes, the current code will not actually use the error string in any
> useful way in this particular case, since report() or report_v2()
> will have nobody listening to them. But being consistent will help
> maintaining the caller, as it can later be extended to use it
> locally (e.g. log the request and its outcome, check which cmd has
> succeeded and failed using the NULL-ness of cmd->error_string, etc.)
The main receive-pack process will be killed by SIGPIPE anyway but I can
fill the error_string fields and return for code consistency.
I'll send a v4, thanks for the review.
