Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > But as I pointed out in > https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@xxxxxxxxx/ > I think this part (and the preceding two commits) are really taking is > down the wrong path. > > To demonstrate why, here's a patch-on-top of this topic as a whole where > I renamed the "kvi" struct members. I've excluded config.c itself (as > its internals aren't interesting for the purposes of this discussion): [snip patch showing where kvi is used] Ah, thanks for this patch. One of my objections to your proposal in the aforementioned thread is that we would end up with a lot of callback- taking config functions that have 2 variants, one for the kvi callback and one for the non-kvi callback, but here your patch shows that it won't be the case. Your approach does look like a reasonable one. 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.
