Hi
>
> > From: Nsengiyumva Wilberforce <nsengiyumvawilberforce@xxxxxxxxx>
> >
> > 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@xxxxxxxxx> wrote:
>
> "nsengaw4c via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Nsengiyumva Wilberforce <nsengiyumvawilberforce@xxxxxxxxx>
> >
> > 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?
