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

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

[Max Gautier]

Hi.

I was trying to implement a pre-push hook to verify my commits are
properly signed before pushing them, and stumbled upon the following
output (which looks like a bug to me):

$ git rev-list @{u}..HEAD --format='%G? %H'
commit 9497d347b048dbea7f527624f815f7926594c4bc
error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification
N 9497d347b048dbea7f527624f815f7926594c4bc
commit 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c
error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification
N 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c
commit ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1
error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification
N ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1
commit 16d17277c608d995ad4d0b495d029c753509930c
error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification
N 16d17277c608d995ad4d0b495d029c753509930c

While git log works and is able to retrieve the signatures

$ git log @{u}..HEAD --format='%G? %H'
G 9497d347b048dbea7f527624f815f7926594c4bc
G 2466c5b3c0f2053b3cdadf4af299aab35e74aa0c
G ded83bc7f31df14b2e9a8d7bdfa1e95eee2bf5c1
G 16d17277c608d995ad4d0b495d029c753509930c


I get the error even though I have the following config :
$ git config --list | grep 'allowed'
gpg.ssh.allowedsignersfile=~/.config/git/MY_SIGNER_KEYS
# by the way the actual config entry in ~/.config/git/config is
# 
#[gpg "ssh"]
#	allowedSignersFile = ~/.config/git/MY_SIGNER_KEYS

$ cat ~/.config/git/MY_SIGNER_KEYS
mg@max.gautier.name,max.gautier@redhat.com sk-ssh-ed25519@openssh.com AAAAGnNrLXNzaC1lZDI1NTE5QG9wZW5zc2guY29tAAAAIL3W2Y4eAF92ySEW6ZE7d8Q+GXvP2G5quvN0zM+f1jGUAAAAB3NzaDphbGw=
mg@max.gautier.name,max.gautier@redhat.com sk-ssh-ed25519@openssh.com AAAAGnNrLXNzaC1lZDI1NTE5QG9wZW5zc2guY29tAAAAIGBP0XfpNXRoFBIW9uEgfnCrrjgvzxr0taOYy0A03DtKAAAABHNzaDo=


Am I missing something obvious ? Or is it git rev-list running in such a
context than it can't find the allowedSignersFile ?

Thanks

-- 
Max Gautier
Software Engineer, Open Services Group, Emerging Technologies
Red Hat
max.gautier@redhat.com

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

[Jeff King]

On Wed, Feb 08, 2023 at 04:56:53PM +0100, Max Gautier wrote:

> I was trying to implement a pre-push hook to verify my commits are
> properly signed before pushing them, and stumbled upon the following
> output (which looks like a bug to me):
> 
> $ git rev-list @{u}..HEAD --format='%G? %H'
> commit 9497d347b048dbea7f527624f815f7926594c4bc
> error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification
>
> [...]
>
> While git log works and is able to retrieve the signatures

Yeah, I think this is a bug. The issue is that not every command loads
the config callback for every config option. This is how we
traditionally implemented the split between porcelain and plumbing
(e.g., user-facing "git diff" will parse and respect "color.diff", but
the scriptable "git diff-files" would not).

In this case, the gpg config has been pushed to its own handler, and a
few specific commands (like git-log) call it. I don't know if there is a
good reason to avoid loading the config in plumbing, or if it was simply
cargo-culted.

I didn't test, but I suspect the patch below would fix your problem:

diff --git a/config.c b/config.c
index 00090a32fc..7ac9f1f5bc 100644
--- a/config.c
+++ b/config.c
@@ -1881,6 +1881,14 @@ int git_default_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "core."))
 		return git_default_core_config(var, value, cb);
 
+	/*
+	 * yikes, this needs to come early in the function because it
+	 * also handles user.signingkey, which would otherwise get
+	 * shunted to git_ident_config() below
+	 */
+	if (git_gpg_config(var, value, cb) < 0)
+		return -1;
+
 	if (starts_with(var, "user.") ||
 	    starts_with(var, "author.") ||
 	    starts_with(var, "committer."))

but it would need a bit more work:

  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.

  2. The individual calls to git_gpg_config() in other programs should
     go away.

  3. It's possible some refactoring may let us avoid the "yikes" comment
     above (e.g., should user.signingkey just go into the normal ident
     config handler?).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	/*
> +	 * yikes, this needs to come early in the function because it
> +	 * also handles user.signingkey, which would otherwise get
> +	 * shunted to git_ident_config() below
> +	 */
> +	if (git_gpg_config(var, value, cb) < 0)
> +		return -1;

Indeed.

>  	if (starts_with(var, "user.") ||
>  	    starts_with(var, "author.") ||
>  	    starts_with(var, "committer."))
>
> but it would need a bit more work:
>
>   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.

>   2. The individual calls to git_gpg_config() in other programs should
>      go away.

Naturally.

>   3. It's possible some refactoring may let us avoid the "yikes" comment
>      above (e.g., should user.signingkey just go into the normal ident
>      config handler?).

Hindsight is golden---if this were called gpg.signingkey we wouldn't
be having this discussion X-<.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> +	/*
>> +	 * yikes, this needs to come early in the function because it
>> +	 * also handles user.signingkey, which would otherwise get
>> +	 * shunted to git_ident_config() below
>> +	 */
>> +	if (git_gpg_config(var, value, cb) < 0)
>> +		return -1;
>> ...
>>  	if (starts_with(var, "user.") ||
>> ...
>>   3. It's possible some refactoring may let us avoid the "yikes" comment
>>      above (e.g., should user.signingkey just go into the normal ident
>>      config handler?).
>
> Hindsight is golden---if this were called gpg.signingkey we wouldn't
> be having this discussion X-<.

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.

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

[Junio C Hamano]

Instead of forcing the porcelain commands to always read the
configuration variables related to the signing and verifying
signatures, lazily initialize the necessary subsystem on demand upon
the first use.

This hopefully would make it more future-proof as we do not have to
think and decide whether we should call git_gpg_config() in the
git_config() callback for each command.

Quite a many git_config() callback functions that used to be custom
callbacks are now just a thin wrapper around git_default_config().
We could further remove, git_FOO_config and replace calls to
git_config(git_FOO_config) with git_config(git_default_config), but
to make it clear which ones are affected and the effect is only the
removal of git_gpg_config(), it is vastly preferred not to do such a
change in this step (they can be done on top once the dust settled).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

> 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).

 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 ----
 16 files changed, 23 insertions(+), 57 deletions(-)

diff --git c/builtin/am.c w/builtin/am.c
index 82a41cbfc4..40126b59c5 100644
--- c/builtin/am.c
+++ w/builtin/am.c
@@ -2314,12 +2314,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 
 static int git_am_config(const char *k, const char *v, void *cb UNUSED)
 {
-	int status;
-
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
-
 	return git_default_config(k, v, NULL);
 }
 
diff --git c/builtin/commit-tree.c w/builtin/commit-tree.c
index cc8d584be2..f6a099d601 100644
--- c/builtin/commit-tree.c
+++ w/builtin/commit-tree.c
@@ -39,9 +39,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
 
 static int commit_tree_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, NULL);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git c/builtin/commit.c w/builtin/commit.c
index 44b763d7cd..794500dc9e 100644
--- c/builtin/commit.c
+++ w/builtin/commit.c
@@ -1600,7 +1600,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
 	struct wt_status *s = cb;
-	int status;
 
 	if (!strcmp(k, "commit.template"))
 		return git_config_pathname(&template_file, k, v);
@@ -1620,9 +1619,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
 	return git_status_config(k, v, s);
 }
 
diff --git c/builtin/log.c w/builtin/log.c
index 04412dd9c9..56741c6d37 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -601,8 +601,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_gpg_config(var, value, cb) < 0)
-		return -1;
 	return git_diff_ui_config(var, value, cb);
 }
 
diff --git c/builtin/merge.c w/builtin/merge.c
index 74de2ebd2b..289c13656c 100644
--- c/builtin/merge.c
+++ w/builtin/merge.c
@@ -659,9 +659,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	}
 
 	status = fmt_merge_msg_config(k, v, cb);
-	if (status)
-		return status;
-	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
 	return git_diff_ui_config(k, v, cb);
diff --git c/builtin/pull.c w/builtin/pull.c
index 1ab4de0005..4367727db6 100644
--- c/builtin/pull.c
+++ w/builtin/pull.c
@@ -359,8 +359,6 @@ static enum rebase_type config_get_rebase(int *rebase_unspecified)
  */
 static int git_pull_config(const char *var, const char *value, void *cb)
 {
-	int status;
-
 	if (!strcmp(var, "rebase.autostash")) {
 		config_autostash = git_config_bool(var, value);
 		return 0;
@@ -372,10 +370,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 		check_trust_level = 0;
 	}
 
-	status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
-
 	return git_default_config(var, value, cb);
 }
 
diff --git c/builtin/push.c w/builtin/push.c
index 60ac8017e5..8f8904dd08 100644
--- c/builtin/push.c
+++ w/builtin/push.c
@@ -502,11 +502,6 @@ static int git_push_config(const char *k, const char *v, void *cb)
 {
 	const char *slot_name;
 	int *flags = cb;
-	int status;
-
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
 
 	if (!strcmp(k, "push.followtags")) {
 		if (git_config_bool(k, v))
diff --git c/builtin/receive-pack.c w/builtin/receive-pack.c
index a90af30363..9894dbdc79 100644
--- c/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -133,10 +133,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 {
 	int status = parse_hide_refs_config(var, value, "receive", &hidden_refs);
 
-	if (status)
-		return status;
-
-	status = git_gpg_config(var, value, NULL);
 	if (status)
 		return status;
 
diff --git c/builtin/send-pack.c w/builtin/send-pack.c
index 4c5d125fa0..c31e27346b 100644
--- c/builtin/send-pack.c
+++ w/builtin/send-pack.c
@@ -130,8 +130,6 @@ static void print_helper_status(struct ref *ref)
 
 static int send_pack_config(const char *k, const char *v, void *cb)
 {
-	git_gpg_config(k, v, NULL);
-
 	if (!strcmp(k, "push.gpgsign")) {
 		const char *value;
 		if (!git_config_get_value("push.gpgsign", &value)) {
diff --git c/builtin/tag.c w/builtin/tag.c
index d428c45dc8..725cfcd62b 100644
--- c/builtin/tag.c
+++ w/builtin/tag.c
@@ -180,8 +180,6 @@ static const char tag_template_nocleanup[] =
 
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-	int status;
-
 	if (!strcmp(var, "tag.gpgsign")) {
 		config_sign_tag = git_config_bool(var, value);
 		return 0;
@@ -194,9 +192,6 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	if (!strcmp(var, "tag.forcesignannotated")) {
 		force_sign_annotate = git_config_bool(var, value);
 		return 0;
diff --git c/builtin/verify-commit.c w/builtin/verify-commit.c
index 3ebad32b0f..3c5d0b024c 100644
--- c/builtin/verify-commit.c
+++ w/builtin/verify-commit.c
@@ -54,9 +54,6 @@ static int verify_commit(const char *name, unsigned flags)
 
 static int git_verify_commit_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git c/builtin/verify-tag.c w/builtin/verify-tag.c
index 217566952d..ecffb069bf 100644
--- c/builtin/verify-tag.c
+++ w/builtin/verify-tag.c
@@ -21,9 +21,6 @@ static const char * const verify_tag_usage[] = {
 
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git c/fmt-merge-msg.c w/fmt-merge-msg.c
index f48f44f9cd..9b83c95a08 100644
--- c/fmt-merge-msg.c
+++ w/fmt-merge-msg.c
@@ -17,8 +17,6 @@ static struct string_list suppress_dest_patterns = STRING_LIST_INIT_DUP;
 
 int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 {
-	int status = 0;
-
 	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
 		int is_bool;
 		merge_log_config = git_config_bool_or_int(key, value, &is_bool);
@@ -37,9 +35,6 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 			string_list_append(&suppress_dest_patterns, value);
 		suppress_dest_pattern_seen = 1;
 	} else {
-		status = git_gpg_config(key, value, NULL);
-		if (status)
-			return status;
 		return git_default_config(key, value, cb);
 	}
 	return 0;
diff --git c/gpg-interface.c w/gpg-interface.c
index 687236430b..3b9ea3ac4c 100644
--- c/gpg-interface.c
+++ w/gpg-interface.c
@@ -9,6 +9,18 @@
 #include "tempfile.h"
 #include "alias.h"
 
+static int git_gpg_config(const char *, const char *, void *);
+
+static void gpg_interface_lazy_init(void)
+{
+	static int done;
+
+	if (done)
+		return;
+	done = 1;
+	git_config(git_gpg_config, 0);
+}
+
 static char *configured_signing_key;
 static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file;
 static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
@@ -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;
 
@@ -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);
 }
 
-int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
+static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
 {
 	struct gpg_format *fmt = NULL;
 	char *fmtname = NULL;
@@ -888,6 +904,8 @@ static const char *get_ssh_key_id(void) {
 /* Returns a textual but unique representation of the signing key */
 const char *get_signing_key_id(void)
 {
+	gpg_interface_lazy_init();
+
 	if (use_format->get_key_id) {
 		return use_format->get_key_id();
 	}
@@ -898,6 +916,8 @@ const char *get_signing_key_id(void)
 
 const char *get_signing_key(void)
 {
+	gpg_interface_lazy_init();
+
 	if (configured_signing_key)
 		return configured_signing_key;
 	if (use_format->get_default_key) {
@@ -923,6 +943,8 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)
 
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
+	gpg_interface_lazy_init();
+
 	return use_format->sign_buffer(buffer, signature, signing_key);
 }
 
diff --git c/gpg-interface.h w/gpg-interface.h
index 8a9ef41779..143cdc1c02 100644
--- c/gpg-interface.h
+++ w/gpg-interface.h
@@ -79,7 +79,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
  */
 const char *gpg_trust_level_to_str(enum signature_trust_level level);
 
-int git_gpg_config(const char *, const char *, void *);
 void set_signing_key(const char *);
 const char *get_signing_key(void);
 
diff --git c/sequencer.c w/sequencer.c
index 3e4a197289..a234b1ff88 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -263,10 +263,6 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 	if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference"))
 		opts->commit_use_reference = git_config_bool(k, v);
 
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
-
 	return git_diff_basic_config(k, v, NULL);
 }
 

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

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 08 2023, Junio C Hamano wrote:

> Instead of forcing the porcelain commands to always read the
> configuration variables related to the signing and verifying
> signatures, lazily initialize the necessary subsystem on demand upon
> the first use.
>
> This hopefully would make it more future-proof as we do not have to
> think and decide whether we should call git_gpg_config() in the
> git_config() callback for each command.

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.

I think that's OK overall, and most of our config reading these days
works like that, but it's probably worth noting.

> Quite a many git_config() callback functions that used to be custom
> callbacks are now just a thin wrapper around git_default_config().
> We could further remove, git_FOO_config and replace calls to
> git_config(git_FOO_config) with git_config(git_default_config), but
> to make it clear which ones are affected and the effect is only the
> removal of git_gpg_config(), it is vastly preferred not to do such a
> change in this step (they can be done on top once the dust settled).

Yeah, we could do that later, but I think it's trivially easy to see
which ones would be affected, i.e. these...

> diff --git c/builtin/am.c w/builtin/am.c
> index 82a41cbfc4..40126b59c5 100644
> --- c/builtin/am.c
> +++ w/builtin/am.c
> @@ -2314,12 +2314,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>  
>  static int git_am_config(const char *k, const char *v, void *cb UNUSED)
>  {
> -	int status;
> -
> -	status = git_gpg_config(k, v, NULL);
> -	if (status)
> -		return status;
> -
>  	return git_default_config(k, v, NULL);
>  }
>  
> diff --git c/builtin/commit-tree.c w/builtin/commit-tree.c
> index cc8d584be2..f6a099d601 100644
> --- c/builtin/commit-tree.c
> +++ w/builtin/commit-tree.c
> @@ -39,9 +39,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
>  
>  static int commit_tree_config(const char *var, const char *value, void *cb)
>  {
> -	int status = git_gpg_config(var, value, NULL);
> -	if (status)
> -		return status;
>  	return git_default_config(var, value, cb);
>  }

...but not a bunch of elided ones here, and then these...

> diff --git c/builtin/verify-commit.c w/builtin/verify-commit.c
> index 3ebad32b0f..3c5d0b024c 100644
> --- c/builtin/verify-commit.c
> +++ w/builtin/verify-commit.c
> @@ -54,9 +54,6 @@ static int verify_commit(const char *name, unsigned flags)
>  
>  static int git_verify_commit_config(const char *var, const char *value, void *cb)
>  {
> -	int status = git_gpg_config(var, value, cb);
> -	if (status)
> -		return status;
>  	return git_default_config(var, value, cb);
>  }
>  
> diff --git c/builtin/verify-tag.c w/builtin/verify-tag.c
> index 217566952d..ecffb069bf 100644
> --- c/builtin/verify-tag.c
> +++ w/builtin/verify-tag.c
> @@ -21,9 +21,6 @@ static const char * const verify_tag_usage[] = {
>  
>  static int git_verify_tag_config(const char *var, const char *value, void *cb)
>  {
> -	int status = git_gpg_config(var, value, cb);
> -	if (status)
> -		return status;
>  	return git_default_config(var, value, cb);
>  }

...we can see are now just git_default_config() by another name. I'd
prefer to just see them gone in this same commit.

> @@ -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.

> @@ -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...

> -int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
> +static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
>  {
>  	struct gpg_format *fmt = NULL;
>  	char *fmtname = NULL;
> @@ -888,6 +904,8 @@ static const char *get_ssh_key_id(void) {
>  /* Returns a textual but unique representation of the signing key */
>  const char *get_signing_key_id(void)
>  {
> +	gpg_interface_lazy_init();
> +

...we'll read back here, and here the lazy init is needed, because...

>  	if (use_format->get_key_id) {

...this is one of the lazy init'd variables.

>  		return use_format->get_key_id();
>  	}
> @@ -898,6 +916,8 @@ const char *get_signing_key_id(void)
>  
>  const char *get_signing_key(void)
>  {
> +	gpg_interface_lazy_init();
> +

ditto, this is needed.

>  	if (configured_signing_key)
>  		return configured_signing_key;
>  	if (use_format->get_default_key) {
> @@ -923,6 +943,8 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)
>  
>  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
>  {
> +	gpg_interface_lazy_init();
> +

and this.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

>> @@ -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.

Thanks.

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

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 08 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Heh. I primarily wanted to see how much damage to the code does it
take to implement such a lazy-loading scheme ;-)  Which turned out
to be "not much".

Maybe when I have time next time, but no promises.

> 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).

Yup.

> 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.

That was my recollection from back when gpg-interface was split out
of "git tag".

Thanks for sanity checking.

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

[Junio C Hamano]

Instead of forcing the porcelain commands to always read the
configuration variables related to the signing and verifying
signatures, lazily initialize the necessary subsystem on demand upon
the first use.

This hopefully would make it more future-proof as we do not have to
think and decide whether we should call git_gpg_config() in the
git_config() callback for each command.

A few git_config() callback functions that used to be custom
callbacks are now just a thin wrapper around git_default_config().
We could further remove, git_FOO_config and replace calls to
git_config(git_FOO_config) with git_config(git_default_config), but
to make it clear which ones are affected and the effect is only the
removal of git_gpg_config(), it is vastly preferred not to do such a
change in this step (they can be done on top once the dust settled).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * OK, this time it comes with a small test addition ;-)

 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 ----
 t/t7031-verify-tag-signed-ssh.sh | 10 ++++++++++
 17 files changed, 33 insertions(+), 57 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 82a41cbfc4..40126b59c5 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2314,12 +2314,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 
 static int git_am_config(const char *k, const char *v, void *cb UNUSED)
 {
-	int status;
-
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
-
 	return git_default_config(k, v, NULL);
 }
 
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index cc8d584be2..f6a099d601 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -39,9 +39,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
 
 static int commit_tree_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, NULL);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 44b763d7cd..794500dc9e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1600,7 +1600,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
 	struct wt_status *s = cb;
-	int status;
 
 	if (!strcmp(k, "commit.template"))
 		return git_config_pathname(&template_file, k, v);
@@ -1620,9 +1619,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
 	return git_status_config(k, v, s);
 }
 
diff --git a/builtin/log.c b/builtin/log.c
index 04412dd9c9..56741c6d37 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -601,8 +601,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_gpg_config(var, value, cb) < 0)
-		return -1;
 	return git_diff_ui_config(var, value, cb);
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 74de2ebd2b..289c13656c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -659,9 +659,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	}
 
 	status = fmt_merge_msg_config(k, v, cb);
-	if (status)
-		return status;
-	status = git_gpg_config(k, v, NULL);
 	if (status)
 		return status;
 	return git_diff_ui_config(k, v, cb);
diff --git a/builtin/pull.c b/builtin/pull.c
index 1ab4de0005..4367727db6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -359,8 +359,6 @@ static enum rebase_type config_get_rebase(int *rebase_unspecified)
  */
 static int git_pull_config(const char *var, const char *value, void *cb)
 {
-	int status;
-
 	if (!strcmp(var, "rebase.autostash")) {
 		config_autostash = git_config_bool(var, value);
 		return 0;
@@ -372,10 +370,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 		check_trust_level = 0;
 	}
 
-	status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
-
 	return git_default_config(var, value, cb);
 }
 
diff --git a/builtin/push.c b/builtin/push.c
index 60ac8017e5..8f8904dd08 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -502,11 +502,6 @@ static int git_push_config(const char *k, const char *v, void *cb)
 {
 	const char *slot_name;
 	int *flags = cb;
-	int status;
-
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
 
 	if (!strcmp(k, "push.followtags")) {
 		if (git_config_bool(k, v))
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a90af30363..9894dbdc79 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -133,10 +133,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 {
 	int status = parse_hide_refs_config(var, value, "receive", &hidden_refs);
 
-	if (status)
-		return status;
-
-	status = git_gpg_config(var, value, NULL);
 	if (status)
 		return status;
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 4c5d125fa0..c31e27346b 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -130,8 +130,6 @@ static void print_helper_status(struct ref *ref)
 
 static int send_pack_config(const char *k, const char *v, void *cb)
 {
-	git_gpg_config(k, v, NULL);
-
 	if (!strcmp(k, "push.gpgsign")) {
 		const char *value;
 		if (!git_config_get_value("push.gpgsign", &value)) {
diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8..725cfcd62b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -180,8 +180,6 @@ static const char tag_template_nocleanup[] =
 
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-	int status;
-
 	if (!strcmp(var, "tag.gpgsign")) {
 		config_sign_tag = git_config_bool(var, value);
 		return 0;
@@ -194,9 +192,6 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	if (!strcmp(var, "tag.forcesignannotated")) {
 		force_sign_annotate = git_config_bool(var, value);
 		return 0;
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 3ebad32b0f..3c5d0b024c 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -54,9 +54,6 @@ static int verify_commit(const char *name, unsigned flags)
 
 static int git_verify_commit_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 217566952d..ecffb069bf 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -21,9 +21,6 @@ static const char * const verify_tag_usage[] = {
 
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
-	if (status)
-		return status;
 	return git_default_config(var, value, cb);
 }
 
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index f48f44f9cd..9b83c95a08 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -17,8 +17,6 @@ static struct string_list suppress_dest_patterns = STRING_LIST_INIT_DUP;
 
 int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 {
-	int status = 0;
-
 	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
 		int is_bool;
 		merge_log_config = git_config_bool_or_int(key, value, &is_bool);
@@ -37,9 +35,6 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 			string_list_append(&suppress_dest_patterns, value);
 		suppress_dest_pattern_seen = 1;
 	} else {
-		status = git_gpg_config(key, value, NULL);
-		if (status)
-			return status;
 		return git_default_config(key, value, cb);
 	}
 	return 0;
diff --git a/gpg-interface.c b/gpg-interface.c
index 687236430b..404d4cccf3 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,6 +9,18 @@
 #include "tempfile.h"
 #include "alias.h"
 
+static int git_gpg_config(const char *, const char *, void *);
+
+static void gpg_interface_lazy_init(void)
+{
+	static int done;
+
+	if (done)
+		return;
+	done = 1;
+	git_config(git_gpg_config, NULL);
+}
+
 static char *configured_signing_key;
 static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file;
 static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
@@ -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;
 
@@ -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);
 }
 
-int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
+static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
 {
 	struct gpg_format *fmt = NULL;
 	char *fmtname = NULL;
@@ -888,6 +904,8 @@ static const char *get_ssh_key_id(void) {
 /* Returns a textual but unique representation of the signing key */
 const char *get_signing_key_id(void)
 {
+	gpg_interface_lazy_init();
+
 	if (use_format->get_key_id) {
 		return use_format->get_key_id();
 	}
@@ -898,6 +916,8 @@ const char *get_signing_key_id(void)
 
 const char *get_signing_key(void)
 {
+	gpg_interface_lazy_init();
+
 	if (configured_signing_key)
 		return configured_signing_key;
 	if (use_format->get_default_key) {
@@ -923,6 +943,8 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)
 
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
+	gpg_interface_lazy_init();
+
 	return use_format->sign_buffer(buffer, signature, signing_key);
 }
 
diff --git a/gpg-interface.h b/gpg-interface.h
index 8a9ef41779..143cdc1c02 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -79,7 +79,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
  */
 const char *gpg_trust_level_to_str(enum signature_trust_level level);
 
-int git_gpg_config(const char *, const char *, void *);
 void set_signing_key(const char *);
 const char *get_signing_key(void);
 
diff --git a/sequencer.c b/sequencer.c
index 3e4a197289..a234b1ff88 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -263,10 +263,6 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 	if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference"))
 		opts->commit_use_reference = git_config_bool(k, v);
 
-	status = git_gpg_config(k, v, NULL);
-	if (status)
-		return status;
-
 	return git_diff_basic_config(k, v, NULL);
 }
 
diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh
index 36eb86a4b1..20913b3713 100755
--- a/t/t7031-verify-tag-signed-ssh.sh
+++ b/t/t7031-verify-tag-signed-ssh.sh
@@ -200,4 +200,14 @@ test_expect_success GPGSSH 'verifying a forged tag with --format should fail sil
 	test_must_be_empty actual-forged
 '
 
+test_expect_success GPGSSH 'rev-list --format=%G' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git rev-list -1 --format="%G? %H" sixth-signed >actual &&
+	cat >expect <<-EOF &&
+	commit $(git rev-parse sixth-signed^0)
+	G $(git rev-parse sixth-signed^0)
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.39.1-418-g7876265d61

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

[Jeff King]

On Thu, Feb 09, 2023 at 12:24:14PM -0800, Junio C Hamano wrote:

> Instead of forcing the porcelain commands to always read the
> configuration variables related to the signing and verifying
> signatures, lazily initialize the necessary subsystem on demand upon
> the first use.
> 
> This hopefully would make it more future-proof as we do not have to
> think and decide whether we should call git_gpg_config() in the
> git_config() callback for each command.

Sorry, I seem to have missed this when you originally posted it. And I
saw it marked as "will merge to next?" in the latest what's cooking. It
looks good to me, and I think we can proceed with it (though of course
it is not urgent and can probably wait until post-2.40).

> A few git_config() callback functions that used to be custom
> callbacks are now just a thin wrapper around git_default_config().
> We could further remove, git_FOO_config and replace calls to
> git_config(git_FOO_config) with git_config(git_default_config), but
> to make it clear which ones are affected and the effect is only the
> removal of git_gpg_config(), it is vastly preferred not to do such a
> change in this step (they can be done on top once the dust settled).

Yes, I think it is good not to do so in this patch. If we want to do it
now on top, here's a patch. Though I could also see the argument for
just leaving them.

-- >8 --
Subject: [PATCH] drop pure pass-through config callbacks

Commit fd2d4c135e (gpg-interface: lazily initialize and read the
configuration, 2023-02-09) shrunk a few custom config callbacks so that
they are just one-liners of:

  return git_default_config(...);

We can drop them entirely and replace them direct calls of
git_default_config() intead. This makes the code a little shorter and
easier to understand (with the downside being that if they do grow
custom options again later, we'll have to recreate the functions).

Signed-off-by: Jeff King <peff@peff.net>
---
I looked over the output of:

  git grep --function-context 'return git_default_config'

to see if there were other cases, not caused by fd2d4c135e. But I didn't
see any.

 builtin/am.c            | 7 +------
 builtin/commit-tree.c   | 7 +------
 builtin/verify-commit.c | 7 +------
 builtin/verify-tag.c    | 7 +------
 4 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 40126b59c5..fccf40f8ee 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2312,11 +2312,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 	return 0;
 }
 
-static int git_am_config(const char *k, const char *v, void *cb UNUSED)
-{
-	return git_default_config(k, v, NULL);
-}
-
 int cmd_am(int argc, const char **argv, const char *prefix)
 {
 	struct am_state state;
@@ -2440,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(usage, options);
 
-	git_config(git_am_config, NULL);
+	git_config(git_default_config, NULL);
 
 	am_state_init(&state);
 
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index f6a099d601..c0bbe9373d 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -37,11 +37,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
 	commit_list_insert(parent, parents_p);
 }
 
-static int commit_tree_config(const char *var, const char *value, void *cb)
-{
-	return git_default_config(var, value, cb);
-}
-
 static int parse_parent_arg_callback(const struct option *opt,
 		const char *arg, int unset)
 {
@@ -118,7 +113,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(commit_tree_config, NULL);
+	git_config(git_default_config, NULL);
 
 	if (argc < 2 || !strcmp(argv[1], "-h"))
 		usage_with_options(commit_tree_usage, options);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 3c5d0b024c..7aedf10e85 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -52,11 +52,6 @@ static int verify_commit(const char *name, unsigned flags)
 	return run_gpg_verify((struct commit *)obj, flags);
 }
 
-static int git_verify_commit_config(const char *var, const char *value, void *cb)
-{
-	return git_default_config(var, value, cb);
-}
-
 int cmd_verify_commit(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
@@ -67,7 +62,7 @@ int cmd_verify_commit(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_verify_commit_config, NULL);
+	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, verify_commit_options,
 			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ecffb069bf..5c00b0b0f7 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -19,11 +19,6 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-static int git_verify_tag_config(const char *var, const char *value, void *cb)
-{
-	return git_default_config(var, value, cb);
-}
-
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
@@ -36,7 +31,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_verify_tag_config, NULL);
+	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, verify_tag_options,
 			     verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
-- 
2.40.0.rc0.479.g8b3a13b6b0

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Sorry, I seem to have missed this when you originally posted it. And I
> saw it marked as "will merge to next?" in the latest what's cooking. It
> looks good to me, and I think we can proceed with it (though of course
> it is not urgent and can probably wait until post-2.40).
> ...
> Yes, I think it is good not to do so in this patch. If we want to do it
> now on top, here's a patch. Though I could also see the argument for
> just leaving them.

Ah, of course I completely forgot about the patch and mentioning a
possible follow-on work.

> I looked over the output of:
>
>   git grep --function-context 'return git_default_config'
>
> to see if there were other cases, not caused by fd2d4c135e. But I didn't
> see any.
>
>  builtin/am.c            | 7 +------
>  builtin/commit-tree.c   | 7 +------
>  builtin/verify-commit.c | 7 +------
>  builtin/verify-tag.c    | 7 +------
>  4 files changed, 4 insertions(+), 24 deletions(-)

Nice to see these reductions.  Thanks.

> diff --git a/builtin/am.c b/builtin/am.c
> index 40126b59c5..fccf40f8ee 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2312,11 +2312,6 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>  	return 0;
>  }
>  
> -static int git_am_config(const char *k, const char *v, void *cb UNUSED)
> -{
> -	return git_default_config(k, v, NULL);
> -}
> -
>  int cmd_am(int argc, const char **argv, const char *prefix)
>  {
>  	struct am_state state;
> @@ -2440,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage_with_options(usage, options);
>  
> -	git_config(git_am_config, NULL);
> +	git_config(git_default_config, NULL);
>  
>  	am_state_init(&state);
>  
> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
> index f6a099d601..c0bbe9373d 100644
> --- a/builtin/commit-tree.c
> +++ b/builtin/commit-tree.c
> @@ -37,11 +37,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
>  	commit_list_insert(parent, parents_p);
>  }
>  
> -static int commit_tree_config(const char *var, const char *value, void *cb)
> -{
> -	return git_default_config(var, value, cb);
> -}
> -
>  static int parse_parent_arg_callback(const struct option *opt,
>  		const char *arg, int unset)
>  {
> @@ -118,7 +113,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	git_config(commit_tree_config, NULL);
> +	git_config(git_default_config, NULL);
>  
>  	if (argc < 2 || !strcmp(argv[1], "-h"))
>  		usage_with_options(commit_tree_usage, options);
> diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
> index 3c5d0b024c..7aedf10e85 100644
> --- a/builtin/verify-commit.c
> +++ b/builtin/verify-commit.c
> @@ -52,11 +52,6 @@ static int verify_commit(const char *name, unsigned flags)
>  	return run_gpg_verify((struct commit *)obj, flags);
>  }
>  
> -static int git_verify_commit_config(const char *var, const char *value, void *cb)
> -{
> -	return git_default_config(var, value, cb);
> -}
> -
>  int cmd_verify_commit(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
> @@ -67,7 +62,7 @@ int cmd_verify_commit(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	git_config(git_verify_commit_config, NULL);
> +	git_config(git_default_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix, verify_commit_options,
>  			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index ecffb069bf..5c00b0b0f7 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -19,11 +19,6 @@ static const char * const verify_tag_usage[] = {
>  		NULL
>  };
>  
> -static int git_verify_tag_config(const char *var, const char *value, void *cb)
> -{
> -	return git_default_config(var, value, cb);
> -}
> -
>  int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
> @@ -36,7 +31,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	git_config(git_verify_tag_config, NULL);
> +	git_config(git_default_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix, verify_tag_options,
>  			     verify_tag_usage, PARSE_OPT_KEEP_ARGV0);

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

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 26 2023, Jeff King wrote:

> On Thu, Feb 09, 2023 at 12:24:14PM -0800, Junio C Hamano wrote:
>
>> Instead of forcing the porcelain commands to always read the
>> configuration variables related to the signing and verifying
>> signatures, lazily initialize the necessary subsystem on demand upon
>> the first use.
>> 
>> This hopefully would make it more future-proof as we do not have to
>> think and decide whether we should call git_gpg_config() in the
>> git_config() callback for each command.
>
> Sorry, I seem to have missed this when you originally posted it. And I
> saw it marked as "will merge to next?" in the latest what's cooking. It
> looks good to me, and I think we can proceed with it (though of course
> it is not urgent and can probably wait until post-2.40).
>
>> A few git_config() callback functions that used to be custom
>> callbacks are now just a thin wrapper around git_default_config().
>> We could further remove, git_FOO_config and replace calls to
>> git_config(git_FOO_config) with git_config(git_default_config), but
>> to make it clear which ones are affected and the effect is only the
>> removal of git_gpg_config(), it is vastly preferred not to do such a
>> change in this step (they can be done on top once the dust settled).
>
> Yes, I think it is good not to do so in this patch. If we want to do it
> now on top, here's a patch. Though I could also see the argument for
> just leaving them.
>
> -- >8 --
> Subject: [PATCH] drop pure pass-through config callbacks
>
> Commit fd2d4c135e (gpg-interface: lazily initialize and read the
> configuration, 2023-02-09) shrunk a few custom config callbacks so that
> they are just one-liners of:
>
>   return git_default_config(...);
>
> We can drop them entirely and replace them direct calls of
> git_default_config() intead. This makes the code a little shorter and
> easier to understand (with the downside being that if they do grow
> custom options again later, we'll have to recreate the functions).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I looked over the output of:
>
>   git grep --function-context 'return git_default_config'
>
> to see if there were other cases, not caused by fd2d4c135e. But I didn't
> see any.

As added review: This is the same patch diff as I sent on February 9th:
https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/;
my local range-diff to my previously submitted topic & next being:
	
	229:  cc5d1d32fd4 !   2:  d93c160dcbc drop pure pass-through config callbacks
	    @@
	      ## Metadata ##
	    -Author: Jeff King <peff@peff.net>
	    +Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	
	      ## Commit message ##
	    -    drop pure pass-through config callbacks
	    +    {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper
	
	    -    Commit fd2d4c135e (gpg-interface: lazily initialize and read the
	    -    configuration, 2023-02-09) shrunk a few custom config callbacks so that
	    -    they are just one-liners of:
	    +    In the preceding commit these config functions became mere wrappers
	    +    for git_default_config(), so let's invoke it directly instead.
	
	    -      return git_default_config(...);
	    -
	    -    We can drop them entirely and replace them direct calls of
	    -    git_default_config() intead. This makes the code a little shorter and
	    -    easier to understand (with the downside being that if they do grow
	    -    custom options again later, we'll have to recreate the functions).
	    -
	    -    Signed-off-by: Jeff King <peff@peff.net>
	    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
	    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
	
	      ## builtin/am.c ##  
	     @@ builtin/am.c: static int parse_opt_show_current_patch(const struct option *opt, const char *ar
	  -:  ----------- >   3:  c099d48b4bf gpg-interface.c: lazily get GPG config variables on demand

So this LGTM.

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

[Jeff King]

On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote:

> As added review: This is the same patch diff as I sent on February 9th:
> https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/;
> my local range-diff to my previously submitted topic & next being:
> [...]
> So this LGTM.

Thanks, and sorry for stealing your patch. I forgot that yours existed
in that thread (and obviously I'm happy if either is applied).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> As added review: This is the same patch diff as I sent on February 9th:
>> https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/;
>> my local range-diff to my previously submitted topic & next being:
>> [...]
>> So this LGTM.
>
> Thanks, and sorry for stealing your patch. I forgot that yours existed
> in that thread (and obviously I'm happy if either is applied).

I am not Ævar but the last time this happened what he said was that
he did so not because he wanted to complain that somebody else stole
his thunder but because he wanted to show his agreement to the patch
by pointing at an indenendent invention of the same thing.

I personally do not appreciate that tactics, exactly because it can
easily be misinterpreted as a complaint.  Saying "I read the patch
and I think it is exactly how I would solve the problem, too.
Looking good" would have been much safer in that regard and conveyed
the same agreement.

Thanks.

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

[Jeff King]

On Thu, Mar 09, 2023 at 09:03:35AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Mar 08, 2023 at 09:34:15AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >
> >> As added review: This is the same patch diff as I sent on February 9th:
> >> https://lore.kernel.org/git/patch-1.2-d93c160dcbc-20230209T142225Z-avarab@gmail.com/;
> >> my local range-diff to my previously submitted topic & next being:
> >> [...]
> >> So this LGTM.
> >
> > Thanks, and sorry for stealing your patch. I forgot that yours existed
> > in that thread (and obviously I'm happy if either is applied).
> 
> I am not Ævar but the last time this happened what he said was that
> he did so not because he wanted to complain that somebody else stole
> his thunder but because he wanted to show his agreement to the patch
> by pointing at an indenendent invention of the same thing.
> 
> I personally do not appreciate that tactics, exactly because it can
> easily be misinterpreted as a complaint.  Saying "I read the patch
> and I think it is exactly how I would solve the problem, too.
> Looking good" would have been much safer in that regard and conveyed
> the same agreement.

I think you are being too hard on Ævar here. While I agree it can be
interpreted as passive aggressive sniping, I'd be a little frustrated,
too, if I had written a patch and then somebody submitted the exact same
thing later. (Hence my response that it was "oops" and not an
intentional slight).

If two people are independently doing the same work, we're wasting
effort, and it's worth thinking about how we can avoid that. In this
particular case, my opinion is that Ævar's original patch was OK by
itself, but it was coupled with an unwelcome change. Submitting the
cleanup to the now-empty callbacks on their own would have had a greater
chance of success. (As a maintainer, you can also split up patches after
the fact, but there is cognitive load in doing so. Minimizing that load
is something submitters can do to help the project scale).

-Peff

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

[Jeff King]

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

> >   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.

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

I arrived at the same conclusion after seeing what these
configuration affected.  If some plumbing codepaths should avoid
certain gpg-interface features, I didn't see a good reason for them
to be calling into gpg-interface.c functions (e.g. that would have
meant that "rev-list '--format=%G'" should have errored out with a
"%G is only available in log but not in rev-list which is a
plumbing" error message) and at that point reading configuration
is much lessor problem.

> 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.

Yup.  Care must be taken not to break "-u <keyid>" that is set via
calling set_signing_key() directly, which assumes that the default
key is already set from the config earlier, but the resulting code
may make it clearer that our rule is "command line first, and then
peek into configuration as a fallback value", if we did such a
conversion.

Thanks.

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

[Junio C Hamano]

Max Gautier <max.gautier@redhat.com> writes:

> $ git rev-list @{u}..HEAD --format='%G? %H'
> commit 9497d347b048dbea7f527624f815f7926594c4bc
> error: gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification

"git rev-list" as a plumbing does not want to read in or be affected
by unnecessary configuration variables, so it uses
git_default_config() to read the barebones, unlike "git log" that
uses git_log_config() that calls, among other things,
git_gpg_config().

Untill "git rev-list" learns to inspect the --format string and
checks necessary configuration on-demand (e.g. presence of "%G"
signals the need to call git_gpg_config()), this is sort of
expected, unfortunately.