On Fri, May 15, 2020 at 8:55 PM Jeff King <peff@xxxxxxxx> 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.
