Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm
From: Junio C Hamano <hidden>
Date: 2018-07-06 20:34:59
Henning Schild [off-list ref] writes:
-enum gpgformats { PGP_FMT };
+enum gpgformats { PGP_FMT, X509_FMT };
struct gpg_format_data gpg_formats[] = {
{ .format = "PGP", .program = "gpg",
.extra_args_verify = { "--keyid-format=long", },
.sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
},
+ { .format = "X509", .program = "gpgsm",
+ .extra_args_verify = { NULL },
+ .sigs = {X509_SIGNATURE, NULL, }
Missing SP between "{X" is a bit irritating.
Also the trailing comma (the issue is shared with the PGP side) when
the initializer is smashed on a single line feels pretty much
pointless. If it were multi-line, then such a trailing comma would
help future developers to add a new entry, i.e.
.sigs = {
PGP_SIGNATURE,
PGP_MESSAGE,
+ PGP_SOMETHING_NEW,
}
without touching the last existing entry. But on a single line?
-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }
is probably prettier without such a trailing comma.
quoted hunk
@@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb) if (!strcmp(var, "gpg.program")) return git_config_string(&gpg_formats[PGP_FMT].program, var, value); + if (!strcmp(var, "gpg.programX509")) + return git_config_string(&gpg_formats[X509_FMT].program, var, + value);
This is a git_config() callback, isn't it? A two-level variable name is given to a callback after downcasing, so nothing will match "gpg.programX509", I suspect. I see Brian already commented on the name and the better organization being - gpg.format defines 'openpgp' or whatever other values; - gpg.<format>.program defines the actual program where <format> is the value gpg.format would take (e.g. "gpg.openpgp.program = gnupg"). And I agree with these suggestions.