Re: [PATCH v3 3/7] gpg-interface: introduce an abstraction for multiple gpg formats
Am Mon, 16 Jul 2018 13:40:32 -0700
schrieb Junio C Hamano :
> Henning Schild writes:
>
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "openpgp". This commit
> > prepares for the introduction of more formats, that might use other
> > programs and match other signatures.
> >
> > Signed-off-by: Henning Schild
> > ---
> > gpg-interface.c | 84
> > ++--- 1 file
> > changed, 63 insertions(+), 21 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 960c40086..699651fd9 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -7,11 +7,46 @@
> > #include "tempfile.h"
> >
> > static char *configured_signing_key;
> > -static const char *gpg_format = "openpgp";
> > -static const char *gpg_program = "gpg";
> > +struct gpg_format {
> > + const char *name;
> > + const char *program;
> > + const char **extra_args_verify;
> > + const char **sigs;
> > +};
> > +
> > +static const char *openpgp_verify_args[] =
> > { "--keyid-format=long", NULL };
>
> Let's write it like this, even if the current line is short enough:
>
> static const char *openpgp_verify_args[] = {
> "--keyid-format=long",
> NULL
> };
>
> Ditto for the next one. Even if you do not expect these two arrays
> to get longer, people tend to copy & paste to imitate existing code
> when making new things, and we definitely would not want them to be
> doing so on a code like the above (or below). When they need to add
> new elements to their arrays, having the terminating NULL on its own
> line means they will have to see less patch noise.
Ok, for consistency a later patch will introduce { NULL }; on three
lines.
> > +static const char *openpgp_sigs[] = {
> > + "-BEGIN PGP SIGNATURE-",
> > + "-BEGIN PGP MESSAGE-", NULL };
> > +
> > +static struct gpg_format gpg_formats[] = {
> > + { .name = "openpgp", .program = "gpg",
> > + .extra_args_verify = openpgp_verify_args,
>
> If the underlying aray is "verify_args" (not "extra"), perhaps the
> field name should also be .verify_args to match.
Renamed extra_args_verify to verify_args.
> Giving an array of things a name "things[]" is a disease, unless the
> array is very often passed around as a whole to represent the
> collection of everything. When the array is mostly accessed one
> element at a time, being able to say thing[3] to mean the third
> thing is much more pleasant.
>
> So, from that point of view, I pretty much agree with
> openpgp_verify_args[] and openpgp_sigs[], but am against
> gpg_formats[]. The last one should be gpg_format[], for which we
> happen to have only one variant right now.
renamed gpg_formats[] to gpg_format[].
> > + .sigs = openpgp_sigs
> > + },
> > +};
> > +static struct gpg_format *current_format = _formats[0];
>
> Have a blank line before the above one.
>
> What does "current_format" mean? Is it different from "format to be
> used"? As we will overwrite the variable upon reading the config,
> we cannot afford to call it default_format, but somehow "current"
> feels misleading, at least to me. I expected to find "old format"
> elsewhere and there may be some format conversion or something from
> the variable name.
>
> static struct gpg_format *use_format = _format[0];
>
> perhaps?
renamed current_format to use_format.
> > +
> > +static struct gpg_format *get_format_by_name(const char *str)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > + if (!strcasecmp(gpg_formats[i].name, str))
>
> As [1/7], this would become strcmp(), I presume?
>
> > + return gpg_formats + i;
> > + return NULL;
> > +}
> >
> > -#define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> > -#define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> > +static struct gpg_format *get_format_by_sig(const char *sig)
> > +{
> > + int i, j;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > + for (j = 0; gpg_formats[i].sigs[j]; j++)
> > + if (starts_with(sig,
> > gpg_formats[i].sigs[j]))
> > + return gpg_formats + i;
> > + return NULL;
> > +}
>
> OK.
>
> > @@ -140,18 +172,23 @@ int git_gpg_config(const char *var, const
> > char *value, void *cb) }
> >
> > if (!strcmp(var, "gpg.format")) {
> > - if (value && strcasecmp(value, "openpgp"))
> > - return error("malformed value for %s: %s",
> > var, value);
> > - return git_config_string(_format, var, value);
> > - }
> > -
> > - if (!strcmp(var, "gpg.program")) {
> > if (!value)
> > return config_error_nonbool(var);
> > - gpg_program = xstrdup(value);
> > + fmt = get_format_by_name(value);
> > + if (!fmt)
> > + return error("malformed value for %s: %s",
> > var, value);
>
> If I
Re: [PATCH v3 3/7] gpg-interface: introduce an abstraction for multiple gpg formats
Henning Schild writes:
> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "openpgp". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild
> ---
> gpg-interface.c | 84
> ++---
> 1 file changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 960c40086..699651fd9 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -7,11 +7,46 @@
> #include "tempfile.h"
>
> static char *configured_signing_key;
> -static const char *gpg_format = "openpgp";
> -static const char *gpg_program = "gpg";
> +struct gpg_format {
> + const char *name;
> + const char *program;
> + const char **extra_args_verify;
> + const char **sigs;
> +};
> +
> +static const char *openpgp_verify_args[] = { "--keyid-format=long", NULL };
Let's write it like this, even if the current line is short enough:
static const char *openpgp_verify_args[] = {
"--keyid-format=long",
NULL
};
Ditto for the next one. Even if you do not expect these two arrays
to get longer, people tend to copy & paste to imitate existing code
when making new things, and we definitely would not want them to be
doing so on a code like the above (or below). When they need to add
new elements to their arrays, having the terminating NULL on its own
line means they will have to see less patch noise.
> +static const char *openpgp_sigs[] = {
> + "-BEGIN PGP SIGNATURE-",
> + "-BEGIN PGP MESSAGE-", NULL };
> +
> +static struct gpg_format gpg_formats[] = {
> + { .name = "openpgp", .program = "gpg",
> + .extra_args_verify = openpgp_verify_args,
If the underlying aray is "verify_args" (not "extra"), perhaps the
field name should also be .verify_args to match.
Giving an array of things a name "things[]" is a disease, unless the
array is very often passed around as a whole to represent the
collection of everything. When the array is mostly accessed one
element at a time, being able to say thing[3] to mean the third
thing is much more pleasant.
So, from that point of view, I pretty much agree with
openpgp_verify_args[] and openpgp_sigs[], but am against
gpg_formats[]. The last one should be gpg_format[], for which we
happen to have only one variant right now.
> + .sigs = openpgp_sigs
> + },
> +};
> +static struct gpg_format *current_format = _formats[0];
Have a blank line before the above one.
What does "current_format" mean? Is it different from "format to be
used"? As we will overwrite the variable upon reading the config,
we cannot afford to call it default_format, but somehow "current"
feels misleading, at least to me. I expected to find "old format"
elsewhere and there may be some format conversion or something from
the variable name.
static struct gpg_format *use_format = _format[0];
perhaps?
> +
> +static struct gpg_format *get_format_by_name(const char *str)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> + if (!strcasecmp(gpg_formats[i].name, str))
As [1/7], this would become strcmp(), I presume?
> + return gpg_formats + i;
> + return NULL;
> +}
>
> -#define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> -#define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> +static struct gpg_format *get_format_by_sig(const char *sig)
> +{
> + int i, j;
> +
> + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> + for (j = 0; gpg_formats[i].sigs[j]; j++)
> + if (starts_with(sig, gpg_formats[i].sigs[j]))
> + return gpg_formats + i;
> + return NULL;
> +}
OK.
> @@ -140,18 +172,23 @@ int git_gpg_config(const char *var, const char *value,
> void *cb)
> }
>
> if (!strcmp(var, "gpg.format")) {
> - if (value && strcasecmp(value, "openpgp"))
> - return error("malformed value for %s: %s", var, value);
> - return git_config_string(_format, var, value);
> - }
> -
> - if (!strcmp(var, "gpg.program")) {
> if (!value)
> return config_error_nonbool(var);
> - gpg_program = xstrdup(value);
> + fmt = get_format_by_name(value);
> + if (!fmt)
> + return error("malformed value for %s: %s", var, value);
If I say "opongpg", that probably is not "malformed" (it is all
lowercase ascii alphabet that is very plausible-looking string), but
simply "bad value".
But other than the minor nit, yes, this structure is what I expected
to see from the very first step 1/7.
> + current_format = fmt;
> return 0;
> }
>
> + if (!strcmp(var, "gpg.program"))
> + fmtname = "openpgp";
OK, this is a backward compatibility measure to
[PATCH v3 3/7] gpg-interface: introduce an abstraction for multiple gpg formats
Create a struct that holds the format details for the supported formats.
At the moment that is still just "openpgp". This commit prepares for the
introduction of more formats, that might use other programs and match
other signatures.
Signed-off-by: Henning Schild
---
gpg-interface.c | 84 ++---
1 file changed, 63 insertions(+), 21 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 960c40086..699651fd9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,11 +7,46 @@
#include "tempfile.h"
static char *configured_signing_key;
-static const char *gpg_format = "openpgp";
-static const char *gpg_program = "gpg";
+struct gpg_format {
+ const char *name;
+ const char *program;
+ const char **extra_args_verify;
+ const char **sigs;
+};
+
+static const char *openpgp_verify_args[] = { "--keyid-format=long", NULL };
+static const char *openpgp_sigs[] = {
+ "-BEGIN PGP SIGNATURE-",
+ "-BEGIN PGP MESSAGE-", NULL };
+
+static struct gpg_format gpg_formats[] = {
+ { .name = "openpgp", .program = "gpg",
+ .extra_args_verify = openpgp_verify_args,
+ .sigs = openpgp_sigs
+ },
+};
+static struct gpg_format *current_format = _formats[0];
+
+static struct gpg_format *get_format_by_name(const char *str)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+ if (!strcasecmp(gpg_formats[i].name, str))
+ return gpg_formats + i;
+ return NULL;
+}
-#define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
-#define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
+static struct gpg_format *get_format_by_sig(const char *sig)
+{
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+ for (j = 0; gpg_formats[i].sigs[j]; j++)
+ if (starts_with(sig, gpg_formats[i].sigs[j]))
+ return gpg_formats + i;
+ return NULL;
+}
void signature_check_clear(struct signature_check *sigc)
{
@@ -102,12 +137,6 @@ void print_signature_buffer(const struct signature_check
*sigc, unsigned flags)
fputs(output, stderr);
}
-static int is_gpg_start(const char *line)
-{
- return starts_with(line, PGP_SIGNATURE) ||
- starts_with(line, PGP_MESSAGE);
-}
-
size_t parse_signature(const char *buf, size_t size)
{
size_t len = 0;
@@ -115,7 +144,7 @@ size_t parse_signature(const char *buf, size_t size)
while (len < size) {
const char *eol;
- if (is_gpg_start(buf + len))
+ if (get_format_by_sig(buf + len))
match = len;
eol = memchr(buf + len, '\n', size - len);
@@ -132,6 +161,9 @@ void set_signing_key(const char *key)
int git_gpg_config(const char *var, const char *value, void *cb)
{
+ struct gpg_format *fmt = NULL;
+ char *fmtname = NULL;
+
if (!strcmp(var, "user.signingkey")) {
if (!value)
return config_error_nonbool(var);
@@ -140,18 +172,23 @@ int git_gpg_config(const char *var, const char *value,
void *cb)
}
if (!strcmp(var, "gpg.format")) {
- if (value && strcasecmp(value, "openpgp"))
- return error("malformed value for %s: %s", var, value);
- return git_config_string(_format, var, value);
- }
-
- if (!strcmp(var, "gpg.program")) {
if (!value)
return config_error_nonbool(var);
- gpg_program = xstrdup(value);
+ fmt = get_format_by_name(value);
+ if (!fmt)
+ return error("malformed value for %s: %s", var, value);
+ current_format = fmt;
return 0;
}
+ if (!strcmp(var, "gpg.program"))
+ fmtname = "openpgp";
+
+ if (fmtname) {
+ fmt = get_format_by_name(fmtname);
+ return git_config_string(>program, var, value);
+ }
+
return 0;
}
@@ -170,7 +207,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf
*signature, const char *sig
struct strbuf gpg_status = STRBUF_INIT;
argv_array_pushl(,
-gpg_program,
+current_format->program,
"--status-fd=2",
"-bsau", signing_key,
NULL);
@@ -208,6 +245,7 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
struct strbuf *gpg_output, struct strbuf *gpg_status)
{
struct child_process gpg = CHILD_PROCESS_INIT;
+ struct gpg_format *fmt;
struct tempfile *temp;
int ret;
struct strbuf buf = STRBUF_INIT;
@@ -223,10 +261,14 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
return -1;
}
