[v2,0/3] Clear flags before each v2 request

Message

Jonathan Tan Oct. 18, 2018, 8:43 p.m. UTC

To explain the differences in this version of the patch set, I'll quote
an email [1] from Junio:

[1] https://public-inbox.org/git/[e-mail]/

> The change to the code itself sort-of makes sense (I say sort-of
> because I didn't carefully look at the callers to see if they mind
> getting all these flags cleared, but the ones that are cleared are
> the ones that are involved mostly in the negotiation and shold be
> OK).

I have included 2 additional patches for these reasons:

 - After reading the section of Junio's email quoted above, I took
   another look at the flags, and found that not only is state stored in
   the flags between invocations of upload_pack_v2(), state is also
   stored in the want_obj and have_obj global variables. The additional
   patches help clean those up.

 - To help reviewers who want to see if the callers mind getting all 8
   flags cleared, I have included a discussion of all 8 flags in the
   commit message of patch 3. The additional patches made the discussion
   easier.

Responses to other points:

> Hmph, what if commit O had a long history behind it?  
> 
> Should fetching of B result in fetching the whole history?

I think so - when we fetch without --depth or any similar arguments, I
think it's reasonable to have all objects referenced by the fetched
tips.

> Would we
> notice that now all of A's parents are available locally and declare
> that the repository is no longer shallow?

We could, but I think this is outside the scope of this patch set.

> Use test_seq instead, or you'll get hit by test-lint?

Thanks for the pointer to test-lint. I've used test_seq, and checked
that test-lint doesn't print any errors.

> Applied on 'master' or 'maint', this new test does not pass even
> with s/seq/test_&/, so there may be something else wrong with it,
> though.

Thanks - there was a copy-and-paste error (should have grepped for
"fetch< version 2", not "git< version 2").

Jonathan Tan (3):
  upload-pack: make have_obj not global
  upload-pack: make want_obj not global
  upload-pack: clear flags before each v2 request

 t/t5702-protocol-v2.sh |  25 +++++++
 upload-pack.c          | 153 ++++++++++++++++++++++++-----------------
 2 files changed, 115 insertions(+), 63 deletions(-)

Comments

Junio C Hamano Oct. 19, 2018, 3:19 a.m. UTC | #1

Jonathan Tan <[e-mail]> writes:

> Jonathan Tan (3):
>   upload-pack: make have_obj not global
>   upload-pack: make want_obj not global
>   upload-pack: clear flags before each v2 request

It took a bit of time why 2/3 did not apply cleanly but it turns out
this is based on a slightly older tip of 'master' (but still ahead
of 'maint'), that lacks 829a3215 ("commit-graph: close_commit_graph
before shallow walk", 2018-08-20).  Applying and merging it to make
it up-to-date with the tip of 'master' went smoothly once I figured
it out.

The first two clean-up patches are probably overdue and worth doing
regardless of the bugfix.  Nicely done.

The first two steps use static objects local to a transport method
to "preserve the existing behaviour", and because this codepath
happens to want a clean slate every time it gets called, the third
step manages to lose it, which is a nice progression.  But it makes
me wonder if it also hints that there may be a need to invent a
state object that is passed around from the transport layer across
requests, if we want to fulfill a request by calling multiple
transport methods in the future.  In any case, there is no immediate
need to address this comment ;-).

Will replace.  Thanks.