On Mon, Mar 28, 2022 at 07:11:10PM +0000, Calvin Wan wrote:
> A subsequent commit will need to write capabilities for another
> command, so refactor write_fetch_command_and_capabilities() to be able
> to used by both fetch and future command.
Makes obvious sense, and this was something that jumped out to me when I
looked at the first and second versions of this patch. I'm glad that
this is getting factored out.
> Move fetch options set in `FETCH_CHECK_LOCAL` from the fetch state
> machine to above the state machine so it is set by default. The
> initial state of the state machine is always `FETCH_CHECK_LOCAL` so
> this does not affect any current functionality. This change prepares
> for a subsequent commit that doesn't need to check the local state, but
> still requires those options to be set before sending the fetch request.
This took a little more thinking, but I agree that this is behaviorally
equivalent. The key points are (and here I'm basically restating what
you already wrote):
- when the state machine in do_fetch_pack_v2() is in
FETCH_CHECK_LOCAL, we set a few v2-specific defaults
- it will be helpful for a future patch to have these defaults set
_before_ we enter the main loop
- and we can safely make that change, since the initial state is
always FETCH_CHECK_LOCAL anyway, so we're guaranteed to hit that
code whether it's in the loop or not.
So this seems safe to me, too. I'll have to see why this is necessary,
but I suspect we'll find out in subsequent patches, so let's read on...
Thanks,
Taylor
