Re: [PATCH v4 2/8] fetch-pack: move fetch default settings

[Calvin Wan <calvinwan@google.com>]

Done thanks for the suggestions

On Tue, May 3, 2022 at 4:07 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
> > Calvin Wan <calvinwan@google.com> writes:
> >
> > > 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 if the initial state is not FETCH_CHECK_LOCAL.
> >
> >
> > > This is a safe change since the initial state is currently always
> > > FETCH_CHECK_LOCAL, so we're guaranteed to hit that code whether it's in
> > > the state machine or not.
> >
> > What does "it" (which supposed to be able to be in the state machine
> > and also not to be in the state matchine) in the last sentence refer
> > to?
>
> I think the "it" refers to the initialization code that was moved.
>
> > The patch looks correct and I agree that this is a benign no-op
> > because the initial value of state is FETCH_CHECK_LOCAL (i.e. the
> > block of code moved here will execute pretty much as the first
> > thing, and the relative order between that block and sorting of ref
> > list should not matter).  I just didn't understand the explanation
> > given by the patch why it is safe.
> >
> > Thanks.
>
> Agreed - I think the commit message would be better off if it was worded
> something like:
>
>   There are some initialization steps that need to be done at the start
>   of the execution of the do_fetch_pack_v2() state machine. Currently,
>   these are done in the FETCH_CHECK_LOCAL state, which is the initial
>   state.
>
>   However, a subsequent patch will allow for another initial state,
>   while still requiring these initialization steps. Therefore, move
>   these initialization steps to before the state machine, so that they
>   are run regardless of the initial state.
>
> I think this description suffices for a reviewer to see that it is safe,
> but if you want, you can add:
>
>   Note that there is no change in behavior, because we're moving code
>   from the beginning of the first state to just before the execution of
>   the state machine.