| Message ID | 20220502170904.2770649-3-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cat-file: add --batch-command remote-object-info command | expand |
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? > > --- Missing sign-off. 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. > fetch-pack.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index e06125c90a..45473b4602 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1610,18 +1610,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > reader.me = "fetch-pack"; > } > > + /* v2 supports these by default */ > + allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; > + use_sideband = 2; > + if (args->depth > 0 || args->deepen_since || args->deepen_not) > + args->deepen = 1; > + > while (state != FETCH_DONE) { > switch (state) { > case FETCH_CHECK_LOCAL: > sort_ref_list(&ref, ref_compare_name); > QSORT(sought, nr_sought, cmp_ref_by_name); > > - /* v2 supports these by default */ > - allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; > - use_sideband = 2; > - if (args->depth > 0 || args->deepen_since || args->deepen_not) > - args->deepen = 1; > - > /* Filter 'ref' by 'sought' and those that aren't local */ > mark_complete_and_common_ref(negotiator, args, &ref); > filter_refs(args, &ref, sought, nr_sought);
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.
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.
diff --git a/fetch-pack.c b/fetch-pack.c index e06125c90a..45473b4602 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1610,18 +1610,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, reader.me = "fetch-pack"; } + /* v2 supports these by default */ + allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; + use_sideband = 2; + if (args->depth > 0 || args->deepen_since || args->deepen_not) + args->deepen = 1; + while (state != FETCH_DONE) { switch (state) { case FETCH_CHECK_LOCAL: sort_ref_list(&ref, ref_compare_name); QSORT(sought, nr_sought, cmp_ref_by_name); - /* v2 supports these by default */ - allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; - use_sideband = 2; - if (args->depth > 0 || args->deepen_since || args->deepen_not) - args->deepen = 1; - /* Filter 'ref' by 'sought' and those that aren't local */ mark_complete_and_common_ref(negotiator, args, &ref); filter_refs(args, &ref, sought, nr_sought);