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

From: Martin Ågren <martin.agren@gmail.com>
Date: Jul 4, 2018, 07:10 UTC
Message-ID: <CAN0heSrXpLCDRjnZC80QXBG27gd6m5reBn1hfNd_KXxnPVkA2g@mail.gmail.com>
In-Reply-To: <db46732d1cd09a71200e00c2d09605d5beaff364.1530616446.git.henning.schild@siemens.com>

Hi Henning,

On 3 July 2018 at 14:38, Henning Schild <henning.schild@siemens.com> wrote:

Show 6 quoted lines

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

Show 20 quoted lines

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

Show 9 quoted lines

> +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?

Show 8 quoted lines

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