Re: [PATCH v2 7/7] stateless-connect: send response end packet

[Denton Liu <liu.denton@gmail.com>]

Hi Peff,

On Mon, May 18, 2020 at 12:43:08PM -0400, Jeff King wrote:
> On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:

[...]

> > @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> >  	if (reader->status != PACKET_READ_FLUSH)
> >  		die(_("expected flush after ref listing"));
> >  
> > +	if (stateless_rpc) {
> > +		if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
> > +			die(_("expected response end packet after ref listing"));
> > +		if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> > +			die(_("expected flush packet after response end"));
> > +	}
> 
> Having two packets here surprised me. We'd have already received the
> actual flush from the server (as you can see in the context above).
> Wouldn't a single RESPONSE_END be enough to signal the reader?

Yes, having a single RESPONSE_END is enough. I sent a flush packet
because it seemed appropriate when I was writing the patch to end all
messages with a flush but I suppose that's just cargo culting. I'll
remove it in the next iteration.

> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index f73a2ce6cb..bcbbb7e2fb 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> >  	struct fetch_negotiator negotiator_alloc;
> >  	struct fetch_negotiator *negotiator;
> >  	int seen_ack = 0;
> > +	int check_http_delimiter;
> 
> This flag was more complicated than I expected, and I'm not sure how we
> can easily be certain that all necessary paths are covered.

Essentially, any time we're in a path where state is set to
FETCH_SEND_REQUEST or FETCH_DONE, we know that we've finished processing
the current request and we can check for response end packets. Of course,
the one exception to this is the first iteration of the loop when we're
transitioning from FETCH_CHECK_LOCAL to FETCH_SEND_REQUEST where we
can't check for response end packets since nothing has been requested
yet.

> E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via
> process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for
> end-of-response, but READY doesn't? I think the answer is that we'd
> continue to read the same response via FETCH_GET_PACK in this instance.
> 
> I just wonder if there is a better place to put this logic that would be
> more certain to catch every place we'd expect to read to the end of a
> response. But I suppose not. We could push it down into process_acks(),
> but it would have the same READY logic that's here. I'll admit part of
> my complaint is that the existing do_fetch_pack_v2 state-machine switch
> is kind of hard to follow, but that's not your fault.

I debated between the current implementation and doing something like

	int first_iteration = 1;

	...

	while (state != FETCH_DONE) {
		switch (...) {
			...
		}

		if (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {
			if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
				die(_("git fetch-pack: expected response end packet"));
			if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
				die(_("git fetch-pack: expected flush packet"));
		}
		first_iteration = 0;
	}

I think that this catches _all_ the cases without fiddling with any of
the state machine logic.

[...]

> Also, it probably should be check_stateless_delimiter() or something.
> There could be other helpers that support stateless-connect besides
> http.

Yeah... I forgot to change the check_http_delimiter name when I was
doing cleanup. Whoops.

> Speaking of which: this is a change to the remote-helper protocol, since
> we're now expecting stateless-connect helpers to produce these delim
> packets (and failing if they don't). I kind of doubt that anybody but
> remote-curl has implemented v2 stateless-connect, but should we be
> marking this with an extra capability to be on the safe side?

I think that we're probably safe from breaking anything external.
According to the gitremote-helpers documentation, 

	'stateless-connect'::
		Experimental; for internal use only.

so I think that gives us a bit of leeway in terms of the changes we can
make to the stateless-connect protocol. They've been warned ;)

Thanks,

Denton