On 1/4/2021 1:22 AM, Eric Sunshine wrote: > On Sun, Jan 3, 2021 at 8:01 PM Derrick Stolee <stolee@xxxxxxxxx> wrote: >> On 1/2/2021 1:12 AM, Eric Sunshine wrote: >>> This should allow you to get rid of the globals introduced by patch >>> [4/12] (assuming passing the index and repo arguments around >>> everywhere doesn't get overly hairy). >> >> Perhaps now that I've removed the compatibility macros, it would be >> easier to insert the method parameters since most of the lines that >> need to change would be method prototypes and the calls to those methods >> (plus the callback function details). >> >> Is that a valuable effort? I could give it a try, but I want to be sure >> that adjusting all of those helper methods in the builtin would indeed >> have valuable improvements over the static globals used here. > > My impression was that the goal of the earlier work was to pass the > index and repository to each function specifically to avoid tying the > function to a particular index or repository. This helps in cases in > which client code needs to operate on a different index or repository > (for instance, a submodule). Generally speaking, making the index and > repository file-static rather than global does not help reach that > goal since functions are still tied to state which is not local to the > function itself. > > Would the extra effort be valuable in this particular case? I'm not > familiar with this code, but given that `update-index` is a builtin, > such effort may not be too meaningful. If, however, any of the code > from `buildin/update-index.c` ever gets "libified" and moved into the > core library, then that would be a good time to update the functions > to take those values as arguments rather than relying on file-static > or globals. But that's not something that this series necessarily > needs to do; the task can wait until the code needs to be shared by > other modules, I would think. I tried again tonight, and it started getting messy, but then I realized that I could group the callbacks that need the repo and index to use a common struct that holds the other parameters they need. It's still a bigger patch than I'd like, but it is more reasonable. v2 is incoming with my attempt at this. Thanks, -Stolee
