On 12/06, Junio C Hamano wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> 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
