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.
