[PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

[PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 09 2023, Jeff King wrote:

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

I'd already played around with that a bit as part of reviewing Junio's
change, this goes on top of that.

I found that continuing this conversion was getting harder, but these
3 cases really were trivial cases where we're just reading a variable
globally, and then proceeding to use it in one specific place.

Out of the remaining ones gpg.program et all looked easiest, but I
didn't continue with it.

For anyone interested think it would be best to continue by converting
the remaining bits by having commit, tag etc. set up some "struct
gpg", so that when they could directly instruct it ot do its config
reading before parse_options(). The remaining complexity is mainly
with the file-global & having to juggle in what order we read & set
what.

FWIW when poking at this I found that we have fairly robust testing
support for this area, but it could be better, but it's good enough to
spot that if we stop reading these we'll fail tests.

But e.g. for the "gpg.program" we've got tests that'll fail if the
"gpg" program variable isn't read, but not for the "ssh" variable, but
as they'll both share the same/similar reader code any future
migration should spot any glaring bugs, just possibly not subtle ones.

Branch & passing[1] CI at:
https://github.com/avar/git/tree/avar/gpg-lazy-init-configset

1. Well, passing except for the general current Windows CI dumpster
   fire on topics based off current "master".

Ævar Arnfjörð Bjarmason (2):
  {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper
  gpg-interface.c: lazily get GPG config variables on demand

 builtin/am.c            |  7 +----
 builtin/commit-tree.c   |  7 +----
 builtin/verify-commit.c |  7 +----
 builtin/verify-tag.c    |  7 +----
 gpg-interface.c         | 66 ++++++++++++++++-------------------------
 5 files changed, 29 insertions(+), 65 deletions(-)

-- 
2.39.1.1475.gc2542cdc5ef

[PATCH 1/2] {am,commit-tree,verify-{commit,tag}}: refactor away config wrapper

[Ævar Arnfjörð Bjarmason]

In the preceding commit these config functions became mere wrappers
for git_default_config(), so let's invoke it directly instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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 40126b59c54..fccf40f8ee7 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 f6a099d601c..c0bbe9373d0 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 3c5d0b024c9..7aedf10e856 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 ecffb069bf1..5c00b0b0f77 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.39.1.1475.gc2542cdc5ef

[PATCH 2/2] gpg-interface.c: lazily get GPG config variables on demand

[Ævar Arnfjörð Bjarmason]

In the preceding commit we started calling gpg_interface_lazy_init()
when the interface is used, in order to lazily init config.

Parts of the git_gpg_config() we were left with then are going to be
harder to convert to the configset API. E.g. in the case of
"configured_signing_key" the set_signing_key() and get_signing_key()
will modify our global variable, assigning either the config to it, a
user-supplied key (see "keyid" in builtin/tag.c). To avoid the global
we'd need to pass that "keyid" all the way down to the callbacks in
"struct gpg_format".

But in the cases being changed here we can move the reading of the
config variable to be adjacent to its use.

As with the preceding change this isn't without its downsides, just as
in the preceding commit this stopped being an immediate error, and
instead depends on whether we'll reach something that lazily inits the
GPG config:

	git -c gpg.mintrustlevel=bad show --show-signature

This change likewise defers our initialization of these variables even
further. But this should be OK, it's the common pattern for most other
config we read.

At this point we could remove gpg_interface_lazy_init() from
check_signature(), as it only uses gpg.minTrustLevel, and calls
functions that don't need the lazy config, but let's keep to
future-proof changes to the API that may need the initialization at a
distance.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gpg-interface.c | 66 +++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 41 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 404d4cccf34..ab24ed3c57b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -22,8 +22,6 @@ static void gpg_interface_lazy_init(void)
 }
 
 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;
 
 struct gpg_format {
 	const char *name;
@@ -453,6 +451,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	struct strbuf ssh_keygen_out = STRBUF_INIT;
 	struct strbuf ssh_keygen_err = STRBUF_INIT;
 	struct strbuf verify_time = STRBUF_INIT;
+	char *ssh_allowed_signers;
 	const struct date_mode verify_date_mode = {
 		.type = DATE_STRFTIME,
 		.strftime_fmt = "%Y%m%d%H%M%S",
@@ -460,7 +459,8 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		.local = 1,
 	};
 
-	if (!ssh_allowed_signers) {
+	if (git_config_get_string("gpg.ssh.allowedsignersfile",
+				  &ssh_allowed_signers)) {
 		error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification"));
 		return -1;
 	}
@@ -520,6 +520,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		     *line;
 		     line = next) {
 			const char *end_of_text;
+			char *ssh_revocation_file;
 
 			next = end_of_text = strchrnul(line, '\n');
 
@@ -556,7 +557,8 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 				     verify_time.buf,
 				     NULL);
 
-			if (ssh_revocation_file) {
+			if (!git_config_get_pathname("gpg.ssh.revocationfile",
+						     (const char **)&ssh_revocation_file)) {
 				if (file_exists(ssh_revocation_file)) {
 					strvec_pushl(&ssh_keygen.args, "-r",
 						     ssh_revocation_file, NULL);
@@ -564,6 +566,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 					warning(_("ssh signing revocation file configured but not found: %s"),
 						ssh_revocation_file);
 				}
+				free(ssh_revocation_file);
 			}
 
 			sigchain_push(SIGPIPE, SIG_IGN);
@@ -599,6 +602,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	strbuf_release(&ssh_keygen_out);
 	strbuf_release(&ssh_keygen_err);
 	strbuf_release(&verify_time);
+	free(ssh_allowed_signers);
 
 	return ret;
 }
@@ -643,6 +647,8 @@ int check_signature(struct signature_check *sigc,
 {
 	struct gpg_format *fmt;
 	int status;
+	static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
+	const char *value;
 
 	gpg_interface_lazy_init();
 
@@ -661,6 +667,19 @@ int check_signature(struct signature_check *sigc,
 	if (status && !sigc->output)
 		return !!status;
 
+	if (!git_config_get_string_tmp("gpg.mintrustlevel", &value)) {
+		char *trust;
+		int ret;
+
+		trust = xstrdup_toupper(value);
+		ret = parse_gpg_trust_level(trust, &configured_min_trust_level);
+		free(trust);
+
+		if (ret)
+			return error(_("invalid value for '%s': '%s'"),
+				     "gpg.mintrustlevel", value);
+	}
+
 	status |= sigc->result != 'G';
 	status |= sigc->trust_level < configured_min_trust_level;
 
@@ -719,8 +738,6 @@ static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
 {
 	struct gpg_format *fmt = NULL;
 	char *fmtname = NULL;
-	char *trust;
-	int ret;
 
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
@@ -740,38 +757,6 @@ static int git_gpg_config(const char *var, const char *value, void *cb UNUSED)
 		return 0;
 	}
 
-	if (!strcmp(var, "gpg.mintrustlevel")) {
-		if (!value)
-			return config_error_nonbool(var);
-
-		trust = xstrdup_toupper(value);
-		ret = parse_gpg_trust_level(trust, &configured_min_trust_level);
-		free(trust);
-
-		if (ret)
-			return error(_("invalid value for '%s': '%s'"),
-				     var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "gpg.ssh.defaultkeycommand")) {
-		if (!value)
-			return config_error_nonbool(var);
-		return git_config_string(&ssh_default_key_command, var, value);
-	}
-
-	if (!strcmp(var, "gpg.ssh.allowedsignersfile")) {
-		if (!value)
-			return config_error_nonbool(var);
-		return git_config_pathname(&ssh_allowed_signers, var, value);
-	}
-
-	if (!strcmp(var, "gpg.ssh.revocationfile")) {
-		if (!value)
-			return config_error_nonbool(var);
-		return git_config_pathname(&ssh_revocation_file, var, value);
-	}
-
 	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
 		fmtname = "openpgp";
 
@@ -851,16 +836,15 @@ static const char *get_default_ssh_signing_key(void)
 	int ret = -1;
 	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
 	struct strbuf **keys;
-	char *key_command = NULL;
+	char *key_command;
 	const char **argv;
 	int n;
 	char *default_key = NULL;
 	const char *literal_key = NULL;
 
-	if (!ssh_default_key_command)
+	if (git_config_get_string("gpg.ssh.defaultkeycommand", &key_command))
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
 
-	key_command = xstrdup(ssh_default_key_command);
 	n = split_cmdline(key_command, &argv);
 
 	if (n < 0)
-- 
2.39.1.1475.gc2542cdc5ef

Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> On Thu, Feb 09 2023, Jeff King wrote:
>
>> 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.
>
> I'd already played around with that a bit as part of reviewing Junio's
> change, this goes on top of that.

What's your intention of sending these?  I think we are already in
agreement that the churn may not be worth the risk, so if these are
"and here is the churn would look like, not for application", I
would understand it and appreciate it.  But did you mean that these
patches are for application?  I am not sure...

Thanks.

Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 09 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> On Thu, Feb 09 2023, Jeff King wrote:
>>
>>> 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.
>>
>> I'd already played around with that a bit as part of reviewing Junio's
>> change, this goes on top of that.
>
> What's your intention of sending these?

For them to be picked up on top of your jc/gpg-lazy-init.

> I think we are already in
> agreement that the churn may not be worth the risk, so if these are
> "and here is the churn would look like, not for application", I
> would understand it and appreciate it.  But did you mean that these
> patches are for application?  I am not sure...

I understood your "I specifically did not want anybody to start doing
this line of analysis" in [1] to mean that you didn't want to have the
sort of change that the last paragraph of 2/2 notes that we're
deliberately not doing.

I.e. that we'd like to keep the gpg_interface_lazy_init() boilerplate,
even though we might carefully reason that a specific API entry point
won't need to initialize the file-scoped config variables right now.

I then took your "it is vastly preferred not to do such a change in this
step" in [2] as a note that it was deliberate that the change in 1/2
here wasn't part of your jc/gpg-lazy-init, but not that we shouldn't
follow-up with such a clean-up.

The "on top once the dust settled" in [2] can then be addressed by
graduating your jc/gpg-lazy-init soon, and keeping this in "seen" for a
bit, although I think the changes here (and in particular 1/2) are
trivial enough to graduate soon thereafter.

Given that I had mixed feelings about submitting this now, but Jeff's
[3] convinced me. I.e. the change in 2/2 'reduces the risk of "oops, we
forgot to initialize the config here"' in the future.

But obviously it's up to you whether you pick this up, and you don't
seem especially keen on doing so, so if not I guess we'll just drop
this, but I'd be happy if you did.

I do think that the 2/2 here has the added benefit of making your change
easier to review, and that's why I wrote it initially. I was poking at
your patch to see what behavior changes, logic errors or bugs I could
find in it.

I.e. your end state is that we're reading 7 config variables (I'm
counting the *.program ones as one variable). The 2/2 here brings that
down to just 3. Thus the surface area of potential issues where we don't
call gpg_interface_lazy_init() before accessing the values is reduced.

Which is also I why I opted to send this sooner than later, having that
as a review aid helps others now, and not in a few months.

1. https://lore.kernel.org/git/xmqq5ycbpp8a.fsf@gitster.g/
2. https://lore.kernel.org/git/xmqqpmaimvtd.fsf_-_@gitster.g/
3. https://lore.kernel.org/git/Y+TqEM21o+3TGx6D@coredump.intra.peff.net/

Re: [PATCH 0/2] gpg-interface: cleanup + convert low hanging fruit to configset API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> What's your intention of sending these?
>
> For them to be picked up on top of your jc/gpg-lazy-init.
>
>> I think we are already in
>> agreement that the churn may not be worth the risk, so if these are
>> "and here is the churn would look like, not for application", I
>> would understand it and appreciate it.  But did you mean that these
>> patches are for application?  I am not sure...
>
> I understood your "I specifically did not want anybody to start doing
> this line of analysis" in [1] to mean that you didn't want to have the
> sort of change that the last paragraph of 2/2 notes that we're
> deliberately not doing.

I didn't want to see "oh you are calling lazy_init here but you can
delay it even further" kind of comments that is wrong and wastes our
time.  

> I.e. that we'd like to keep the gpg_interface_lazy_init() boilerplate,
> even though we might carefully reason that a specific API entry point
> won't need to initialize the file-scoped config variables right now.

It is the complete opposite of what I meant.

Changing

	git_am_config(...) {
		return git_default_config(...);
	}
	... 
		git_config(git_am_config);

to

	/* no git_am_config() */
	...
		git_config(git_default_config);

is perfectly fine as a clean-up post series.

If we are moving away from git_config() callback style, and move to
git_config_get_*() style, the upthread already said it does not have
a good risk/benefit ratio, but if we were to do so, then we should
not leave some still using the callback style while others using
git_config_get_*(), which will lead to configuration read in a wrong
order and easily breaking precedence rules.

And if we were to move away completely from the callback style, then
I do not see a point to build such a series on top of the lazy init
patch, which is about staying with the callback style.

So, that is exactly why I asked the question after seeing it was
marked to apply on top of the lazy init thing, which did not make
sense to me.