On Thu, Jan 7, 2021 at 6:19 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: > On 1/7/2021 12:09 AM, Eric Sunshine wrote: > > 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. > > 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. One way to avoid such confusion would be to leave a comment in the code stating that the two values will always be the same. The other way, of course, is to eliminate `istate` from the structure altogether. I don't want to make more work for you, but the more I think about it, the more I feel that removing `istate` is the sensible thing to do. (And it doesn't require an extra patch -- it can just be how this patch is crafted -- without ever introducing `istate` to the structure in the first place.)
