Re: [PATCH] ref-filter: add new atom "signature" atom

[NSENGIYUMVA WILBERFORCE <nsengiyumvawilberforce@gmail.com>]

>
> diff --git a/ref-filter.c b/ref-filter.c
> index a4c3f89f64..3b3592acb2 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -405,8 +405,9 @@ static int subject_atom_parser(struct ref_format *format UNUSED,
>         return 0;
>  }
>
> -static int signature_atom_parser(struct ref_format *format, struct used_atom *atom,
> -                              const char *arg, struct strbuf *err)
> +static int signature_atom_parser(struct ref_format *format UNUSED,
> +                                struct used_atom *atom,
> +                                const char *arg, struct strbuf *err)
>  {
>         if (arg) {
>                 if (!strcmp(arg, "signer"))
>
> This will eventually be necessary once we turn on -Wunused-parameter.
> I'm preparing a patch to convert all of the other parsers that need it,
> and I don't want to create a dependency between the two patches (it's OK
> for you to add the UNUSED now, it's just not enforced yet).
>
> I can also circle back after your patch is merged and add it, but it's a
> bit easier to do it up front.

Thanks, worked on it

> +{
> +     if (arg) {
> +             if (!strcmp(arg, "signer"))
> +                     atom->u.signature.option = S_SIGNER;
> +             else if (!strcmp(arg, "grade"))
> +                     atom->u.signature.option = S_GRADE;
> +             else if (!strcmp(arg, "key"))
> +                     atom->u.signature.option = S_KEY;
> +             else if (!strcmp(arg, "fingerprint"))
> +                     atom->u.signature.option = S_FINGERPRINT;
> +             else if (!strcmp(arg, "primarykeyfingerprint"))
> +                     atom->u.signature.option = S_PRI_KEY_FP;
> +             else if (!strcmp(arg, "trustlevel"))
> +                     atom->u.signature.option = S_TRUST_LEVEL;
> +             else
> +                     return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg);
> +     }

The ref-filter code recently got a helper function to report this kind
of argument error consistently, via dda4fc1a84 (ref-filter: factor out
"unrecognized %(foo) arg" errors, 2022-12-14). If you rebase the patch
on the current master, you can just do:

  return err_bad_arg(err, "signature", arg);

which will make the error message match the others (which in turn saves
>
> work for translators).

Thanks for this, I have seen it too


On Tue, Dec 27, 2022 at 1:11 AM Jeff King <peff@peff.net> wrote:
>
> On Tue, Dec 27, 2022 at 12:55:23AM +0000, nsengaw4c via GitGitGadget wrote:
>
> > This only works for commits. Add "signature" atom with `grade`,
> > `signer`, `key`, `fingerprint`, `primarykeyfingerprint`, `trustlevel`
> > as arguments. This code and it's documentation are inspired by
> > how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were
> > implemented.
>
> I don't have a real review for you, but rather two small requests since
> I was working in this area recently.
>
> > @@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom
> >       return 0;
> >  }
> >
> > +static int signature_atom_parser(struct ref_format *format, struct used_atom *atom,
> > +                            const char *arg, struct strbuf *err)
>
> Can you squash in an annotation for the unused parameter, like this:
>
> diff --git a/ref-filter.c b/ref-filter.c
> index a4c3f89f64..3b3592acb2 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -405,8 +405,9 @@ static int subject_atom_parser(struct ref_format *format UNUSED,
>         return 0;
>  }
>
> -static int signature_atom_parser(struct ref_format *format, struct used_atom *atom,
> -                              const char *arg, struct strbuf *err)
> +static int signature_atom_parser(struct ref_format *format UNUSED,
> +                                struct used_atom *atom,
> +                                const char *arg, struct strbuf *err)
>  {
>         if (arg) {
>                 if (!strcmp(arg, "signer"))
>
> This will eventually be necessary once we turn on -Wunused-parameter.
> I'm preparing a patch to convert all of the other parsers that need it,
> and I don't want to create a dependency between the two patches (it's OK
> for you to add the UNUSED now, it's just not enforced yet).
>
> I can also circle back after your patch is merged and add it, but it's a
> bit easier to do it up front.
>
> > +{
> > +     if (arg) {
> > +             if (!strcmp(arg, "signer"))
> > +                     atom->u.signature.option = S_SIGNER;
> > +             else if (!strcmp(arg, "grade"))
> > +                     atom->u.signature.option = S_GRADE;
> > +             else if (!strcmp(arg, "key"))
> > +                     atom->u.signature.option = S_KEY;
> > +             else if (!strcmp(arg, "fingerprint"))
> > +                     atom->u.signature.option = S_FINGERPRINT;
> > +             else if (!strcmp(arg, "primarykeyfingerprint"))
> > +                     atom->u.signature.option = S_PRI_KEY_FP;
> > +             else if (!strcmp(arg, "trustlevel"))
> > +                     atom->u.signature.option = S_TRUST_LEVEL;
> > +             else
> > +                     return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg);
> > +     }
>
> The ref-filter code recently got a helper function to report this kind
> of argument error consistently, via dda4fc1a84 (ref-filter: factor out
> "unrecognized %(foo) arg" errors, 2022-12-14). If you rebase the patch
> on the current master, you can just do:
>
>   return err_bad_arg(err, "signature", arg);
>
> which will make the error message match the others (which in turn saves
> work for translators).
>
> -Peff