I've covered most your response to Ævar upthread, so I'll omit that. Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > As for my own opinions, (before Ævar sent this email) I took a look > at these patches myself and had some issues with at least the first > 2: in patch 1, kvi_fn() replaces fn() for some but not all invocations > of fn() (in patch 2, you can see one such invocation that was not > changed), and I was having difficulty thinking of what kind of bugs > I should watch out for since not all invocations were changed; and > in patch 2, the safeguard of not setting kvi and source together was > removed and likewise I was having difficulty thinking of what kind of > bugs could occur from both being set at once inadvertently. I was going > to suggest reordering the patches such that the large-scale refactoring > (and any supporting patches like [PATCH 06/14] config: inline > git_color_default_config) should occur first (or waiting for a reviewer > who is convinced that patches 1 and 2 are OK, I guess), but having now > seen that sidestepping a large part of this makes sense, sidestepping > seems like a good idea to me. In an off-list discussion, we described some plausible ways to organize the refactor that would make it easier for a reviewer to confirm safety. I haven't tried that yet because it sounds like you'd prefer the sidestepping approach. Do you prefer that primarily for safety reasons, or is it largely motivated by other concerns too (e.g. reducing churn or sidestepping produces a better API)? If the primary concern is just safety, I'm somewhat confident that we can find some way to organize this that makes it easier to review and I should just do it.
