On 1/5/2021 10:55 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> My strategy for update-index was to create static globals "repo" and >> "istate" that point to the_repository and the_index, respectively. Then, I >> was able to remove macros one-by-one without changing method prototypes >> within the file. > > Knee-jerk reaction: swapping one pair of global with another? Would > that give us enough upside? It may allow some codepaths involved to > work on an in-core index instance that is different from the_index, > but does not make them reentrant. My intention was to reduce the use of globals in libgit.a while keeping with existing patterns of static globals in the builtin code. While this can be thought of "module variables" instead of true globals, they aren't exactly desirable. In v2, these static globals are temporary to the series and are completely removed by the end. The new patch sequence can hopefully be seen as "this preprocessor macro was expanded" and then "static globals are replaced with method parameters" which are pretty straightforward. > Do we now have callers that actually pass an in-core index instance > that is different from the_index, and more importantly, that fail > loudly if the codepaths involved in this conversion forgets to > update some accesses to the_index not to the specified one? > > If not, ... > >> In total, this allows us to remove four of the compatibility macros because >> they are no longer used. > > ... a conversion like this, removing the use of the compatibility > macros for the sake of removing them, invites future headaches by > leaving untested code churn behind with potential bugs that will > only get discovered after somebody actually starts making calls > with the non-default in-core index instances. Perhaps I had misunderstood the state of the conversion project. I thought that the full conversion was just paused because Duy moved on to other things. I thought it might be valuable to pick up the baton while also thinking about the space. If this is _not_ a valuable project to continue, then I can hold off for now. Unfortunately, we'll never know if everything is safe from assuming the_index until the macro itself is gone. It helps that libgit.a doesn't use it at > I've come to know the competence of you well enough to trust your > patches like patches from other proficient, prolific and prominent > contributors (I won't name names, but you know who you are), but we > are all human and are prone to introduce bugs. That means a lot, thanks. And yes, I'm well aware that bugs can be introduced. I've added my share. > That's all my knee-jerk impression before actually reading the > series through, though. I'll certainloy know more after reading > them. I look forward to your thoughts. Thanks, -Stolee
