Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Feb 09 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> On Thu, Feb 09 2023, Jeff King wrote:
>>
>>> If the gpg code used git_config_get_string(), etc, then they could just
>>> access each key on demand (efficiently, from an internal hash table),
>>> which reduces the risk of "oops, we forgot to initialize the config
>>> here". It does probably mean restructuring the code a little, though
>>> (since you'd often have an accessor function to get "foo.bar" rather
>>> than assuming "foo.bar" was parsed into an enum already, etc). That may
>>> not be worth the effort (and risk of regression) to convert.
>>
>> I'd already played around with that a bit as part of reviewing Junio's
>> change, this goes on top of that.
>
> What's your intention of sending these?

For them to be picked up on top of your jc/gpg-lazy-init.

> I think we are already in
> agreement that the churn may not be worth the risk, so if these are
> "and here is the churn would look like, not for application", I
> would understand it and appreciate it.  But did you mean that these
> patches are for application?  I am not sure...

I understood your "I specifically did not want anybody to start doing
this line of analysis" in [1] to mean that you didn't want to have the
sort of change that the last paragraph of 2/2 notes that we're
deliberately not doing.

I.e. that we'd like to keep the gpg_interface_lazy_init() boilerplate,
even though we might carefully reason that a specific API entry point
won't need to initialize the file-scoped config variables right now.

I then took your "it is vastly preferred not to do such a change in this
step" in [2] as a note that it was deliberate that the change in 1/2
here wasn't part of your jc/gpg-lazy-init, but not that we shouldn't
follow-up with such a clean-up.

The "on top once the dust settled" in [2] can then be addressed by
graduating your jc/gpg-lazy-init soon, and keeping this in "seen" for a
bit, although I think the changes here (and in particular 1/2) are
trivial enough to graduate soon thereafter.

Given that I had mixed feelings about submitting this now, but Jeff's
[3] convinced me. I.e. the change in 2/2 'reduces the risk of "oops, we
forgot to initialize the config here"' in the future.

But obviously it's up to you whether you pick this up, and you don't
seem especially keen on doing so, so if not I guess we'll just drop
this, but I'd be happy if you did.

I do think that the 2/2 here has the added benefit of making your change
easier to review, and that's why I wrote it initially. I was poking at
your patch to see what behavior changes, logic errors or bugs I could
find in it.

I.e. your end state is that we're reading 7 config variables (I'm
counting the *.program ones as one variable). The 2/2 here brings that
down to just 3. Thus the surface area of potential issues where we don't
call gpg_interface_lazy_init() before accessing the values is reduced.

Which is also I why I opted to send this sooner than later, having that
as a review aid helps others now, and not in a few months.

1. https://lore.kernel.org/git/xmqq5ycbpp8a.fsf@gitster.g/
2. https://lore.kernel.org/git/xmqqpmaimvtd.fsf_-_@gitster.g/
3. https://lore.kernel.org/git/Y+TqEM21o+3TGx6D@coredump.intra.peff.net/