push: drop confusing configset/callback redundancy

We parse push config by calling git_config() with our git_push_config()
callback. But inside that callback, when we see "push.gpgsign", we
ignore the value passed into the callback and instead make a new call to
git_config_get_value().

This is unnecessary at best, and slightly wrong at worst (if there are
multiple instances, get_value() only returns one; both methods end up
with last-one-wins, but we'd fail to report errors if earlier
incarnations were bogus).

The call was added by 68c757f219 (push: add a config option push.gpgSign
for default signed pushes, 2015-08-19). That commit doesn't give any
reason to deviate from the usual strategy here; it was probably just
somebody unfamiliar with our config API and its conventions.

It also added identical code to builtin/send-pack.c, which also handles
push.gpgsign.

And then the same issue spread to its neighbor in b33a15b081 (push: add
recurseSubmodules config option, 2015-11-17), presumably via
cargo-culting.

This patch fixes all three to just directly use the value provided to
the callback. While I was adjusting the code to do so, I noticed that
push.gpgsign is overly careful about a NULL value. After
git_parse_maybe_bool() has returned anything besides 1, we know that the
value cannot be NULL (if it were, it would be an implicit "true", and
many callers of maybe_bool rely on that). Here that lets us shorten "if
(v && !strcasecmp(v, ...))" to just "if (!strcasecmp(v, ...))".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Jeff King 2023-12-07 02:26:22 -05:00 committed by Junio C Hamano
parent be6bc048d7
commit 37e8a341ea
2 changed files with 25 additions and 33 deletions

View file

@ -526,26 +526,21 @@ static int git_push_config(const char *k, const char *v,
*flags |= TRANSPORT_PUSH_AUTO_UPSTREAM;
return 0;
} else if (!strcmp(k, "push.gpgsign")) {
const char *value;
if (!git_config_get_value("push.gpgsign", &value)) {
switch (git_parse_maybe_bool(value)) {
case 0:
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);
break;
case 1:
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);
break;
default:
if (value && !strcasecmp(value, "if-asked"))
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);
else
return error(_("invalid value for '%s'"), k);
}
switch (git_parse_maybe_bool(v)) {
case 0:
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);
break;
case 1:
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);
break;
default:
if (!strcasecmp(v, "if-asked"))
set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);
else
return error(_("invalid value for '%s'"), k);
}
} else if (!strcmp(k, "push.recursesubmodules")) {
const char *value;
if (!git_config_get_value("push.recursesubmodules", &value))
recurse_submodules = parse_push_recurse_submodules_arg(k, value);
recurse_submodules = parse_push_recurse_submodules_arg(k, v);
} else if (!strcmp(k, "submodule.recurse")) {
int val = git_config_bool(k, v) ?
RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;

View file

@ -135,21 +135,18 @@ static int send_pack_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
if (!strcmp(k, "push.gpgsign")) {
const char *value;
if (!git_config_get_value("push.gpgsign", &value)) {
switch (git_parse_maybe_bool(value)) {
case 0:
args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
break;
case 1:
args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
break;
default:
if (value && !strcasecmp(value, "if-asked"))
args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;
else
return error(_("invalid value for '%s'"), k);
}
switch (git_parse_maybe_bool(v)) {
case 0:
args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
break;
case 1:
args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
break;
default:
if (!strcasecmp(v, "if-asked"))
args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;
else
return error(_("invalid value for '%s'"), k);
}
}
return git_default_config(k, v, ctx, cb);