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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2021, 19:57 UTC
Message-ID: <xmqq4kjsmses.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cSLsLibDik3YYTPdwbHpywStMxR_4rRBZfYrX7S-cFnNA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 12 quoted lines

>> 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.

Thanks.