Re: [PATCH v2 14/14] update-index: remove static globals from callbacks

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 8, 2021, 01:52 UTC
Message-ID: <e598f0e4-bdd9-4b59-f267-545204829a63@gmail.com>
In-Reply-To: <xmqq4kjsmses.fsf@gitster.c.googlers.com>

On 1/7/2021 2:57 PM, Junio C Hamano wrote:

Show 27 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>> It could be possible to replace all references to 'istate' with
>>> 'repo->index' but the patches get slightly more messy. I also
>>> think the code looks messier, but you do make a good point that
>>> there is no concrete reason to separate the two.
>>
>> I agree that it would make the code a bit noisier (to read) if
>> `istate` is eliminated from the callback structure, however, even
>> though I didn't originally feel strongly one way or the other about
>> having both `repo` and `istate` in the structure, I'm now leaning more
>> toward seeing `istate` eliminated. My one (big) concern with `istate`
>> is that it confuses readers into wondering whether `istate` and
>> `repo->istate` will ever be different.
> 
> Some applications may want to work on more than one in-core index at
> the same time (perhaps a merge strategy may want to keep a copy of
> the original index and update a second copy with the result of the
> merge), and it may be useful for such applications if 'repo->istate'
> does not have to be the in-core index instance to be worked on.  So
> things that go in libgit.a may want to allow such distinction.
> 
> But what goes in builtin/ is a different story.  As long as this
> application has no need for such a feature and will always work on
> the primary in-core index, prepared for the in-core repository
> structure for convenience, it may not worth it to support such a
> feature that no callers benefit from.

I'll try to restructure my patches to do the following order:

1. replace compatibility macros with static global references, except
   i. use 'istate' in the methods that don't need a repository.
  ii. use 'repo->index' in the methods that need a repository.

2. replace static globals with method parameters.
   i. drop 'istate' static global with method parameters. Methods that
      have a repo will pass 'repo->index' to these methods.
  ii. drop 'repo' static global with method parameters.

3. replace static globals in callback methods using 'repo->index',
   where 'repo' is a member of the callback_data struct.

That should keep the structure as presented in v2 while also avoiding this question of "can istate differ from repo->index?"

Thanks, -Stolee