Re: git rev-list fails to verify ssh-signed commits (but git log works)

From: Jeff King <peff@peff.net>
Date: Feb 9, 2023, 12:41 UTC
Message-ID: <Y+TqEM21o+3TGx6D@coredump.intra.peff.net>
In-Reply-To: <xmqqh6vwrpce.fsf@gitster.g>

On Wed, Feb 08, 2023 at 09:56:16AM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> >   1. Somebody would need to dig into the reasons, if any, for not
> >      calling git_gpg_config() everywhere. It might be fine, but there
> >      may be a good reason which we're now violating. Digging in the
> >      history and looking at the code might yield some hints.
> 
> Hmph, I didn't consider calling gpg_config unconditionally.  It may
> do a bit more than what a typical config callback does (i.e. as
> opposed to just store the string values it gets, it tries table
> look-ups and stuff) but it is not too bad.

It all looks pretty typical for config parsing to me. Matching in a constant-sized list of strings happens in lots of places (e.g., push.default). Performance-wise it's probably fine.

I'd be more worried about correctness (command "foo" must not parse option "bar" because it is plumbing and must use the default). But looking over the options, I really don't see anything like that. The one I'd expect (or worry most about) is "we do/do not bother to enable signatures at all based on config" but I don't think that is the case. We default use_format to &gpg_format[0], so there is always a signature format defined, even if the config is skipped.

The original split comes from your 2f47eae2a1 (Split GPG interface into its own helper library, 2011-09-07), where it was just moving bits out of the git-tag config. So I think we just grew into this situation organically.

Show 6 quoted lines

> I wonder if gpg-interface functions can and should be taught to
> initialize themselves lazily without relying on the usual
> git_config(git_gpg_config) sequence.  I.e. the first call to
> sign_buffer(), check_signature(), get_signing_key_id(), etc.
> would internally make a git_config(git_gpg_config) call, with the
> current callers of git_config(git_gpg_config) removed.

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.

-Peff