Eric Sunshine [off-list ref] writes:
quoted
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.
On 1/7/2021 2:57 PM, Junio C Hamano wrote:
Eric Sunshine [off-list ref] writes:
quoted
quoted
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