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

From: Derrick Stolee <stolee@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Alban Gruin" <alban.gruin@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v2 14/14] update-index: remove static globals from callbacks
Date: Thu, 7 Jan 2021 06:19:47 -0500	[thread overview]
Message-ID: <73f51832-583a-27e8-65fa-86eb0ebac4cf@gmail.com> (raw)
In-Reply-To: <CAPig+cTfHsd7WMvcX1_433WVOG+y4FceUACBx_4UfnjJTgVY6g@mail.gmail.com>

On 1/7/2021 12:09 AM, Eric Sunshine wrote:
> On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> @@ -1098,8 +1103,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> -       istate = repo->index;
>> +       cd.repo = repo;
>> +       cd.istate = istate = repo->index;
> 
> Will there ever be a case in which `cd.istate` will be different from
> `cd.repo->index`? If not, then we could get by with having only
> `cd.repo`; callers requiring access to `istate` can fetch it from
> `cd.repo`. If, on the other hand, `cd.istate` can be different from
> `cd.repo->istate` -- or if that might become a possibility in the
> future -- then having `cd.istate` makes sense. Not a big deal, though.
> Just generally curious about it.

I don't believe that 'istate' and 'repo->index' will ever be
different in this file. This includes the members of the
callback_data struct, but also the method parameters throughout.

Mostly, this could be seen as an artifact of how we got here:

1. References to the_index or other compatibility macros were
   converted to use the static global 'istate'.

2. References to the static global 'istate' were replaced with
   method parameters for everything except these callbacks.

3. These callbacks were updated to use 'cd.istate' instead of
   the (now defunct) static global 'istate'.

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.

Thanks,
-Stolee