Jeff King <p...@peff.net> writes:

>> @@ -16,13 +16,18 @@ struct gpg_format_data {
>>  
>>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>> +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
>>  
>> -enum gpgformats { PGP_FMT };
>> +enum gpgformats { PGP_FMT, X509_FMT };
>>  struct gpg_format_data gpg_formats[] = {
>>      { .format = "openpgp", .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 }
>> +    },
>
> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> outside of this array (as I hope there wouldn't be after this series),
> would it make more sense to just include the literals inline in the
> array definition? That's one less layer of indirection when somebody is
> reading the code.

It is good design-sense to shoot for fewer levels of indirection,
but I suspect that "'const char **' instead of maximally-sized fixed
array of strings" would require a named array and constants like
this:

        static const char *gpg_verify_args[] = {
                "--verify",
                "--status-fd=1",
                "--keyid-format=long",
                NULL
        };
        static const char *gpg_sigs[] = {
                "-----BEGIN PGP SIGNATURE-----",
                "-----BEGIN PGP MESSAGE-----",
                NULL
        };

        struct gpg_format {
                const char *name;
                const char *program;
                const char * const *verify_args;
                const char * const *sigs;
        } gpg_format[] = {
                {
                        .name = "openpgp",
                        .program = "gpg',
                        .verify_args = gpg_verify_args,
                        .sigs = gpg_sigs,
                },
                {
                        ...
                },
        };

so we may end up having the same number of levels of indirection
anyway in the long-term final form.

As readers may be able to read from the above, I also have a
suspicion that it is a mistake to pretend that "--verify" etc.,
which merely happen to be common across the variants the series
covers, will stay forever to be common across _all_ variants and
that is why the field no longer is called "extra" args but is meant
to contain the full args.

Reply via email to