On Wed, Feb 08 2023, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> 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.
*nod*, just noting it.
>>> @@ -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.
>
>>> @@ -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.
Yeah, I take your general point that it's good to sprinkle the
gpg_interface_lazy_init().
In this case I think we'll just barely do the right thing, the only
external caller is tag.c, which first does:
set_signing_key(keyid);
And then:
sign_buffer(buffer, buffer, get_signing_key());
So we'll (I think):
1. Get the non-config'd key from before
2. Call sign_buffer()
3. Promptly clobber that key
4. It won't matter because at that point we'll be passing a key
as a parma, which overrides the "config"
But yeah, dropping it would mean we end up with the wrong key in the
variable afterwards, which even if it's not used is nasty.
