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

From: Jeff King <peff@peff.net>
Date: Feb 9, 2023, 12:49 UTC
Message-ID: <Y+Tr1g+HTn45rsTq@coredump.intra.peff.net>
In-Reply-To: <xmqqlel7rj9z.fsf_-_@gitster.g>

On Wed, Feb 08, 2023 at 12:31:36PM -0800, Junio C Hamano wrote:

Show 9 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.
> 
> So here is such a change.  I only checked that it passed t/ tests
> locally (and I do not run some tests like svn and p4).

I think the tests tell us two things:

  - you didn't miss a spot where config needed to be initialized lazily
    here. The risk here is that many tests will be using defaults, not
    configured values, so coverage is not as good as you might hope.

  - there isn't a case where initializing the config all the time is a
    problem (i.e., the plumbing/porcelain thing discussed earlier). My
    "yikes" patch from upthread likewise passed, so that gives us a
    little confidence (though again, it's not clear that the plumbing
    cases which didn't _expect_ to read config would have test
    coverage).

    That said, having manually reviewed what the function is doing, I
    think it's probably OK (see my other response).

Show 16 quoted lines

>  builtin/am.c            |  6 ------
>  builtin/commit-tree.c   |  3 ---
>  builtin/commit.c        |  4 ----
>  builtin/log.c           |  2 --
>  builtin/merge.c         |  3 ---
>  builtin/pull.c          |  6 ------
>  builtin/push.c          |  5 -----
>  builtin/receive-pack.c  |  4 ----
>  builtin/send-pack.c     |  2 --
>  builtin/tag.c           |  5 -----
>  builtin/verify-commit.c |  3 ---
>  builtin/verify-tag.c    |  3 ---
>  fmt-merge-msg.c         |  5 -----
>  gpg-interface.c         | 24 +++++++++++++++++++++++-
>  gpg-interface.h         |  1 -
>  sequencer.c             |  4 ----

This all looks fairly sensible to me. I think we'd really want to see a "rev-list --format" test, too. One, because that's the immediate goal of this change. But two, because I think we are only guessing that loading the config is sufficient here. We've had bug with other subsystems where they expected to be initialized but plumbing callers didn't (e.g., the lazy init of notes-refs, etc).

I _think_ we're probably good here. Just looking at "git log" (where we know --format, etc, works), it doesn't seem to do anything beyond initializing the config.

-Peff