Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

Brandon Williams wrote:

On 09/20, Jeff King wrote:

quoted

(For that matter, could we just be checking whether *list is NULL?)

True, that would probably be the better way to do this.

Nice idea, thank you.

That doesn't capture a few other cases of pkts that aren't supposed to
come before the capabilities^{} line:

 * shallow
 * .have
 * capabilities^{}
 * invalid refnames

Perhaps it should check all of those:

	if ((shallow_points && shallow_points->nr) ||
	    (extra_have && extra_have->nr) ||
	    got_dummy_ref_with_capabilities_declaration ||
	    got_invalid_ref ||
	    *list)

What happens when another type of pkt gets introduced?  This feels
pretty error-prone.  The underlying problem is that we are emulating a
state machine that is not a simple for loop using a simple for loop,
by piling up variables that keep track of the current state.  That
suggests one of the following approaches:

 A. Replace saw_response with an enum describing the state.
    Immediately after reading the first packet, update the state to
    EXPECTING_FIRST_REF.  Immediately after reading the first ref,
    update the state to EXPECTING_SHALLOW.

 B. Use instruction flow to encode the state machine.  Have separate
    loops for processing refs and shallow lines.

By the way, there are some other ways the current code is less strict
than described in pack-protocol.txt:

 - allowing an empty list-of-refs.  (This is deliberate ---
   pack-protocol.txt's lack of documentation of this case is a bug.)

 - allowing multiple capability-lists

 - allowing capabilities^{} combined with other refs

 - allowing refs, shallow, and .have to be interleaved

Tightening those would likely be good for the ecosystem (so that
buggy servers get noticed quickly), but that's a separate topic from
this change.

[...]

I wasn't sure either, which is why I added the comment to prod
discussion.  I agree that is is orthogonal to this series so I'll most
likely drop it, as it doesn't help with the protocol transition
discussion.

I'd be happy to write a separate patch adding the NEEDSWORK comment
(or even a patch doing what the NEEDSWORK comment suggests) to avoid
derailing this one. :)

Thanks,
Jonathan