Re: [PATCH 00/12] Remove more index compatibility macros

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jan 3, 2021 at 8:01 PM Derrick Stolee <stolee@gmail.com> 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.