On Wed, Feb 08, 2023 at 12:31:36PM -0800, Junio C Hamano wrote:
> > 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).
> 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
