From: Jonathan Tan <jonathantanmy@google.com>
Date: Wed, 30 Mar 2022 21:55:22 GMT
Subject: Re: [PATCH v3 1/3] fetch-pack: refactor packet writing and fetch options
Message-ID: <20220330215522.2302571-1-jonathantanmy@google.com>
In-Reply-To: <20220328191112.3092139-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:
> 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.
> 
> 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.

I think it would be clearer to rephrase "that doesn't need to check the
local state" to "that will skip FETCH_CHECK_LOCAL". It does literally
mean the same thing, but I had a mental block with "initial state...is
always FETCH_CHECK_LOCAL" in the preceding sentence.

I agree with some of the others who said that this change should go into
its own patch.

Otherwise, this patch makes sense.