Re: [PATCH] gpg-interface: lazily initialize and read the configuration

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2023, 02:05 UTC
Message-ID: <xmqq5ycbpp8a.fsf@gitster.g>
In-Reply-To: <230209.86fsbfznot.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 7 quoted lines

> One thing left un-noted here is that this will defer any errors in the
> config now until use (or lazy init), so e.g.:
>
> 	git -c gpg.mintrustlevel=bad show --show-signature
>
> Used to exit with code 128 and an error, but will now (at least for me)
> proceed to run show successfully.

That one is probably a good thing. We shouldn't interrupt users for a misspelt configuration value that the user is not using.

Show 10 quoted lines

>> @@ -632,6 +644,8 @@ int check_signature(struct signature_check *sigc,
>>  	struct gpg_format *fmt;
>>  	int status;
>>  
>> +	gpg_interface_lazy_init();
>> +
>>  	sigc->result = 'N';
>>  	sigc->trust_level = -1;
>
> This is needed, because we need "configured_min_trust_level" populated.

I specifically did not want anybody to start doing this line of analysis, because it will add unnecessary bugs, and also introduce maintenance problems. You may be able to grab the current state of the code, but that will get stale and won't be a good guide to keep our code robust.

Show 13 quoted lines

>> @@ -695,11 +709,13 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
>>  
>>  void set_signing_key(const char *key)
>>  {
>> +	gpg_interface_lazy_init();
>> +
>>  	free(configured_signing_key);
>>  	configured_signing_key = xstrdup(key);
>>  }
>
> But this is not, we could say that we're doing it for good measure, but
> it's hard to imagine a scenario where we would end up actually needing
> lazy init here. we'll just set a variable here, which...

And especially this one, we must have init or we'll be incorrect, I think. There is a direct set_signing_key() caller (I think in "git tag") that does not come from the git_config() callback route. Without the lazy initialization, we'd get configured_signing_key from the config because early in the start-up sequence of the command we would do git_gpg_config() via git_config(), and then try to process "-u keyid" by calling this one again.

If you forget to lazily initialize here, configured_signing_key gets the keyid obtained via "-u keyid", and then when control reaches the real signing function, we'd realize that we still haven't initialized ourselves. And we call lazy init, which finds configured keyID, which is very likely different from "-u keyid" (the user would not be passing "-u keyid" from the command line to override, if that is the same as the configured one), and clobber it.

Thanks.