On 1/7/2021 12:09 AM, Eric Sunshine wrote: > On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> 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
