Taylor Blau <me@xxxxxxxxxxxx> writes: > On Mon, Oct 21, 2024 at 11:57:43AM +0200, Karthik Nayak wrote: >> The `packfile.c` file uses the global variable 'the_repository' extensively >> throughout the code. Let's remove all usecases of this, by modifying the >> required functions to accept a 'struct repository' instead. This is to clean up >> usage of global state. >> >> The first 18 patches are mostly passing a `struct repository` to each of the >> functions within `packfile.c` from other files. The last two patches move some >> global config variables and make them local. I'm not too well versed with this >> section of the code, so would be nice to get some eyes here. > > I agree with the goal of this series, but I worry that as written it > will be quite disruptive to other topics on the list. > I agree, that as it currently sits, this is very disruptive. > The standard way to avoid this disruption is to, for e.g. the first > change, do the following: > > - Introduce a new function repo_odb_pack_name() that takes in a > 'struct repository *', and rewrite odb_pack_name() in terms of it > (passing 'the_repository' in as the argument). > > - Write a Coccinelle rule to replace all calls to odb_pack_name() > with calls to repo_odb_pack_name(). > > - Submit those patches without adjusting any non-obvious callers or > ones that are not contained to a single compilation unit that you > are already touching. > > - Wait until a new development cycle has begun, run spatch on the new > rule to replace all other calls. Then optionally rename > repo_odb_pack_name() to odb_pack_name(). > > I think Patrick (CC'd) has done one of these transitions recently, so > I'll defer to him in case I got any of the details wrong. > > In the meantime, I'm going to hold this one out of seen as it may be > disruptive in the current state. > > Thanks, > Taylor While thinking about this over the last few days and also getting some advice from Patrick, I realized that we don't need to be this disruptive by simply adding the 'repository' variable to the already existing 'packed_git' struct. This allows us to leverage this information more easily, since most of the functions already have access to the 'packed_git' struct. This, plus the series by Jeff 'jk/dumb-http-finalize' which also removes some existing functions. We reduce the impact to only 3 functions being modified. I think with such low impact, it might make more sense to not go with the Coccinelle approach, since it is a lot simpler without it. I'll post a new version tomorrow showcasing this approach, but I'll leave the final decision to you whether it is still disruptive, and if the approach you mentioned would be better. Thanks
Attachment:
signature.asc
Description: PGP signature
