Re: [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader

[Brandon Williams <bmwill@google.com>]

On 12/06, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> 
> EXPECTING_DONE sounded like we are expecting to see 'done' packet
> sent from the other side, but I was mistaken.  It is the state
> where we are "done" expecting anything ;-).
> 
> Having an (unconditional) assignment to 'state' in the above switch
> makes me feel somewhat uneasy, as the next "switch (state)" is what
> is meant as the state machine that would allow us to say things like
> "from this state, transition to that state is impossible".  When we
> get a flush while we are expecting the first ref, for example, we'd
> just go into the "done" state.  There is no provision for a future
> update to say "no, getting a flush in this state is an error".

I believe this is accepted behavior, receiving a flush in that state.
And I don't think there is ever an instance during the ref advertisement
where a flush would be an error.  It just indicates that the
advertisement is finished.

> 
> That is no different from the current code; when read_remote_ref()
> notices that it got a flush, it just leaves the loop without even
> touching 'state' variable.  But at least, I find that the current
> code is more honest---it does not even touch 'state' and allows the
> code after the loop to inspect it, if needed.  From that point of
> vhew, the way the new code uses 'state' to leave the loop upon
> seeing a flush is a regression---it makes it harder to notice and
> report when we got a flush in a wrong state.
> 
> Perhaps getting rid of "EXPECTING_DONE" from the enum and then:
> 
> 	int got_flush = 0;
> 	while (1) {
> 		switch (reader_read()) {
> 		case PACKET_READ_FLUSH:
> 			got_flush = 1;
> 			break;
> 		... other cases ...
> 		}
> 
> 		if (got_flush)
> 			break;
> 
> 		switch (state) {
> 		... current code ...
> 		}
> 	}
> 
> would be an improvement; we can later extend "if (got_flush)" part
> to check what state we are in if we wanted to notice and report an
> error there before breaking out of the loop.
> 

I don't really see how this is any clearer from what this patch does
though.  I thought it made it easier to read as you no longer have an
infinite loop, but rather know when it will end (when you move to the
done state).

-- 
Brandon Williams