Re: [PATCH 00/13] upload-pack: use 'struct upload_pack_data' thoroughly, part 1

[Christian Couder <christian.couder@gmail.com>]

On Fri, May 15, 2020 at 8:55 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, May 15, 2020 at 02:47:16PM -0400, Jeff King wrote:
>
> > > While there are still a lot of static variables at the top of
> > > 'upload-pack.c' after this patch series, it does a lot of ground work
> > > and a number of cleanups.
> >
> > Yeah, I think all of use_thin_pack, use_ofs_delta, etc, should be easy
> > conversions on top (and will really give us the payoff).
>
> Hmm, none of those fields in upload_pack_data are used, even for v2!
> I.e., if I apply this patch:

[...]

> it still compiles. So starting to use those would be a behavior change,
> as we accidentally let use_ofs_delta, etc, propagate from one v2 "fetch"
> command to another for ssh/git/file connections (but not for http). I
> think that's fixing a bug (but one nobody is likely to see, because it
> would imply the client sending different capabilities for each request).

I agree.

> I think we'd want something like the patch below.

Thanks for your patch! I have included it as the first patch in the
"part 2" patch series I am planning to send once this "part 1" will be
merged to pu or next:

- the commit with your patch:
https://gitlab.com/gitlab-org/gitlab-git/-/commit/458b79f0f563226bf50924553fc3889cb0942864
- the whole branch with "part 1" and "part 2":
https://gitlab.com/gitlab-org/gitlab-git/-/commits/fix-upload-pack-variable-scope5/

> Some of the other globals, like multi_ack, are really v0 only (since v2
> assumes certain baseline capabilities). We could either leave them
> as-is, or roll them into upload_pack_data and just let v2 code ignore
> them as it does now.

I am in favor of rolling them into upload_pack_data. That's what I
started doing in "part 1" and continued doing in "part 2". I think it
makes the code more coherent and cleaner.

Thanks for your review,
Christian.