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

[NSENGIYUMVA WILBERFORCE <nsengiyumvawilberforce@gmail.com>]

Hi
>
> > From: Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com>
> >
> > 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.
>
> Lacking motivation.  Without explaining why somebody may want to
> have the feature and what it would be used for, "only works for
> commits" would invite a "so what?  does it even have to work?"  as a
> response, so start with a brief descrioption "with the current set
> of atoms, $this_useful_thing cannot easily be achieved" before
> describing its limitation.

Ok, I will edit the commit message. Thanks
>
> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> > index 6da899c6296..9a0be85368b 100644
> > --- a/Documentation/git-for-each-ref.txt
> > +++ b/Documentation/git-for-each-ref.txt
> > @@ -212,6 +212,33 @@ symref::
> >       `:lstrip` and `:rstrip` options in the same way as `refname`
> >       above.
> >
> > +signature::
> > +...
> > +signature:trustlevel::
> > +     The Trust level of the GPG signature of a commit. Possible
> > +     outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.
>
> A good list.  How do these work for signature made with a tool other
> than GPG (in other words, when "gpg.format" is set to something
> other than "openpgp")?

You mean ssh and X509, right? honestly, I did not check the behavior.
I am going to check

> Having said that, wouldn't it be natural to expect that the same
> code can deal with signed tags?  After all we use the same signature
> verification machinery at the lowest level in the callchain.

Very right, it works for signed tags too.

>
> Handing the !arg case first will make the if/else if/... cascade
> easier to follow, no?  Also the body of the function may want to
> become a separate function that returns one of these S_FOO constants.
>
>         static enum signatore_option signature_atom_parser(...)
>         {
>                 enum signature_option opt = parse_signature_option(arg);
>                 if (opt < 0)
>                         return strbuf_addf_ret(err, opt, _("unknown ..."), arg);
>                 return opt;
>         }
>
> where parse_signature_option() would look like
>
>         static enum signature_option parse_signature_option(const char *arg)
>         {
>                 if (!arg)
>                         return S_BARE;
>                 else if (!strcmp(arg, "signer"))
>                         return S_SIGNER;
>                 ...
>                 else
>                         return -1;
>         }
>
> or something like that?

 It makes more sense
>
> > +{
> > +     int i;
> > +     struct commit *commit = (struct commit *) obj;
>
> Style?  No SP between cast and value?

ok, noted
>
> > +
> > +     for (i = 0; i < used_atom_cnt; i++) {
> > +             struct used_atom *atom = &used_atom[i];
> > +             const char *name = atom->name;
> > +             struct atom_value *v = &val[i];
> > +             struct signature_check sigc = { 0 };
> > +
> > +             if (!!deref != (*name == '*'))
> > +                     continue;
> > +             if (deref)
> > +                     name++;
> > +             if (strcmp(name, "signature") &&
> > +                     strcmp(name, "signature:signer") &&
> > +                     strcmp(name, "signature:grade") &&
> > +                     strcmp(name, "signature:key") &&
> > +                     strcmp(name, "signature:fingerprint") &&
> > +                     strcmp(name, "signature:primarykeyfingerprint") &&
> > +                     strcmp(name, "signature:trustlevel"))
> > +                     continue;
>
> And with the helper above, we can avoid the repetition here that can
> go out of sync with the parser function.

I am not sure I have understood this, which helper?

> > +             check_commit_signature(commit, &sigc);
>
> If a format asks for signature:signer and signature:key, we
> shouldn't be running GPG twice.  First check used_atom[] to see if
> we even need to do _any_ signature processing (and leave if there is
> not), populate the sigc just once and then enter the loop, perhaps?

Yeah, I think it was not right calling check_commit_signature() in the
loop. Populating sigc at once looks more good to me


>
>  In adddition, a call to check_commit_signature() should have a
>
> matching call to signature_check_clear(); otherwise all the
>
> resources held by sigc would leak, wouldn't it?

Yeah, it would.


On Mon, Dec 26, 2022 at 9:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "nsengaw4c via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com>
> >
> > 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.
>
> Lacking motivation.  Without explaining why somebody may want to
> have the feature and what it would be used for, "only works for
> commits" would invite a "so what?  does it even have to work?"  as a
> response, so start with a brief descrioption "with the current set
> of atoms, $this_useful_thing cannot easily be achieved" before
> describing its limitation.
>
> Having said that, wouldn't it be natural to expect that the same
> code can deal with signed tags?  After all we use the same signature
> verification machinery at the lowest level in the callchain.
>
> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> > index 6da899c6296..9a0be85368b 100644
> > --- a/Documentation/git-for-each-ref.txt
> > +++ b/Documentation/git-for-each-ref.txt
> > @@ -212,6 +212,33 @@ symref::
> >       `:lstrip` and `:rstrip` options in the same way as `refname`
> >       above.
> >
> > +signature::
> > +...
> > +signature:trustlevel::
> > +     The Trust level of the GPG signature of a commit. Possible
> > +     outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.
>
> A good list.  How do these work for signature made with a tool other
> than GPG (in other words, when "gpg.format" is set to something
> other than "openpgp")?
>
> > @@ -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)
> > +{
> > +     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);
> > +     }
> > +     else
> > +             atom->u.signature.option = S_BARE;
> > +     return 0;
> > +}
>
> Handing the !arg case first will make the if/else if/... cascade
> easier to follow, no?  Also the body of the function may want to
> become a separate function that returns one of these S_FOO constants.
>
>         static enum signatore_option signature_atom_parser(...)
>         {
>                 enum signature_option opt = parse_signature_option(arg);
>                 if (opt < 0)
>                         return strbuf_addf_ret(err, opt, _("unknown ..."), arg);
>                 return opt;
>         }
>
> where parse_signature_option() would look like
>
>         static enum signature_option parse_signature_option(const char *arg)
>         {
>                 if (!arg)
>                         return S_BARE;
>                 else if (!strcmp(arg, "signer"))
>                         return S_SIGNER;
>                 ...
>                 else
>                         return -1;
>         }
>
> or something like that?
>
> > @@ -1344,6 +1374,69 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
> >       }
> >  }
> >
> > +static void grab_signature(struct atom_value *val, int deref, struct object *obj)
>
> To be considerate for future developers, perhaps rename this to
> grab_commit_signature(), so that they can add grab_tag_signature()
> when they lift the limitation of this implementaiton?
>
> > +{
> > +     int i;
> > +     struct commit *commit = (struct commit *) obj;
>
> Style?  No SP between cast and value?
>
> > +
> > +     for (i = 0; i < used_atom_cnt; i++) {
> > +             struct used_atom *atom = &used_atom[i];
> > +             const char *name = atom->name;
> > +             struct atom_value *v = &val[i];
> > +             struct signature_check sigc = { 0 };
> > +
> > +             if (!!deref != (*name == '*'))
> > +                     continue;
> > +             if (deref)
> > +                     name++;
> > +             if (strcmp(name, "signature") &&
> > +                     strcmp(name, "signature:signer") &&
> > +                     strcmp(name, "signature:grade") &&
> > +                     strcmp(name, "signature:key") &&
> > +                     strcmp(name, "signature:fingerprint") &&
> > +                     strcmp(name, "signature:primarykeyfingerprint") &&
> > +                     strcmp(name, "signature:trustlevel"))
> > +                     continue;
>
> And with the helper above, we can avoid the repetition here that can
> go out of sync with the parser function.
>
> > +             check_commit_signature(commit, &sigc);
>
> If a format asks for signature:signer and signature:key, we
> shouldn't be running GPG twice.  First check used_atom[] to see if
> we even need to do _any_ signature processing (and leave if there is
> not), populate the sigc just once and then enter the loop, perhaps?
>
> In adddition, a call to check_commit_signature() should have a
> matching call to signature_check_clear(); otherwise all the
> resources held by sigc would leak, wouldn't it?