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

[Jeff King <peff@peff.net>]

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