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

Am Wed, 4 Jul 2018 09:10:17 +0200
schrieb Martin Ågren [off-list ref]:

Hi Henning,

On 3 July 2018 at 14:38, Henning Schild [off-list ref]
wrote:

quoted

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

Welcome to the mailing list! :-)

Thanks!

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

quoted

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

Ok, i did not actually check coding style yet. I could run it through a
tool, given there is a suggestion. Or i could address issues someone
points out in the review.
What i get from your comment is that it might be ok to leave the code
as is, others have introduces similar constructs before me.

quoted

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

Yes that is code that can not (should not) be reached. I agree that an
assert(0) is not very expressive and will fix that in v2.

quoted

        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?

I will pull the tests from patch 5 before touching that code and fix up
issues inbetween. The whole series saw a "make test" inbetween all
commits.

quoted

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

True, the two are almost the same and should be merged. Will do in v2.

Henning

quoted

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

Martin