Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

["Martin Ågren" <martin.agren@gmail.com>]

Hi Henning,

On 3 July 2018 at 14:38, Henning Schild <henning.schild@siemens.com> wrote:
> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "PGP". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>

Welcome to the mailing list! :-)

I'll just comment on a few thoughts I had while skimming this.

>  static char *configured_signing_key;
> -static const char *gpg_format = "PGP";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +       const char *format;
> +       const char *program;
> +       const char *extra_args_verify[1];
> +       const char *sigs[2];
> +};
>
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>
> +enum gpgformats { PGP_FMT };
> +struct gpg_format_data gpg_formats[] = {
> +       { .format = "PGP", .program = "gpg",
> +         .extra_args_verify = { "--keyid-format=long", },
> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> +       },
> +};

I think those trailing commas are ok now, but I'm not sure...

I had the same thought about designated initializers. Those should be ok
now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
for color slot parsing, 2018-05-26).

> +static const char *gpg_format = "PGP";
> +
> +static struct gpg_format_data *get_format_data(void)
> +{
> +       int i;
> +       for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +               if (!strcmp(gpg_formats[i].format, gpg_format))
> +                       return gpg_formats + i;
> +       assert(0);

This might be better written as `BUG("bad gpg_format '%s'",
gpg_format);` or something like that.

(It's not supposed to be triggered, not even by invalid data from the
user, right?)

>         if (!strcmp(var, "gpg.format")) {
> -               if (!strcmp(value, "PGP"))

This line was added in patch 3. It errors out precisely when gpg.format
is "PGP", no? That this doesn't break the whole series is explained by
1) it being removed in this patch 4, and 2) there being no tests. It
makes me wonder if something like patch 5 (test gpg.format) could be
part of patch 3, both with negative ("= malformed") and positive ("=
PGP") tests?

> +               j = 0;
> +               for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +                       if (!strcmp(value, gpg_formats[i].format)) {
> +                               j++;
> +                               break;
> +                       }
> +               if (!j)
>                         return error("malformed value for %s: %s", var, value);

`if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?

Or check whether `get_format_data()` returns NULL? Hmm, well you can't,
since it takes its "input" from a global variable...

If you want to keep that global nature, the duplication of search-logic
could perhaps be avoided by having a helper function for returning the
index of a gpg_format (or -1).

>                 return git_config_string(&gpg_format, var, value);
>         }

Martin