From: Jonathan Tan Date: Fri, 05 May 2023 21:07:02 GMT Subject: Re: [PATCH 10/14] (RFC-only) config: finish config_fn_t refactor Message-ID: <20230505210702.3359841-1-jonathantanmy@google.com> In-Reply-To: <230501.868re8jna4.gmgdl@evledraar.gmail.com> Ævar Arnfjörð Bjarmason writes: > But as I pointed out in > https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/ > 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.