git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "nsengaw4c via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, nsengaw4c <nsengiyumvawilberforce@gmail.com>
Subject: Re: [PATCH] ref-filter: add new atom "signature" atom
Date: Tue, 27 Dec 2022 11:20:28 +0900	[thread overview]
Message-ID: <xmqqo7rpvb83.fsf@gitster.g> (raw)
In-Reply-To: <pull.1452.git.1672102523902.gitgitgadget@gmail.com> (nsengaw4c via GitGitGadget's message of "Tue, 27 Dec 2022 00:55:23 +0000")

"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?

  reply	other threads:[~2022-12-27  2:25 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-27  0:55 [PATCH] ref-filter: add new atom "signature" atom nsengaw4c via GitGitGadget
2022-12-27  2:20 ` Junio C Hamano [this message]
2023-01-02  4:49   ` NSENGIYUMVA WILBERFORCE
2023-01-02  8:37     ` Christian Couder
2023-01-03  0:58       ` Junio C Hamano
     [not found]   ` <CA+PPyiGd0-AiwhPa5e+fDdA9RybS+c5XeOYm5yycCZco3VHAxg@mail.gmail.com>
2023-01-08 15:21     ` NSENGIYUMVA WILBERFORCE
2022-12-27  6:11 ` Jeff King
2023-01-02  6:34   ` NSENGIYUMVA WILBERFORCE
2023-01-10  0:52 ` [PATCH v3 0/1] ref-filter: add new " Nsengiyumva Wilberforce
2023-01-10  0:52   ` [PATCH v3 1/1] " Nsengiyumva Wilberforce
2023-01-16 17:38     ` [PATCH v4 0/1] " Nsengiyumva Wilberforce
2023-01-16 17:38       ` [PATCH v4 1/1] " Nsengiyumva Wilberforce
2023-03-11 21:06         ` [PATCH v5 0/1] " Nsengiyumva Wilberforce
2023-03-11 21:06           ` [PATCH v5 1/1] " Nsengiyumva Wilberforce
2023-03-14 22:51             ` Junio C Hamano
2023-04-28 18:29             ` Kousik Sanagavarapu
2023-04-29 18:37               ` Kousik Sanagavarapu
2023-01-26 21:07       ` [PATCH v4 0/1] " Junio C Hamano
2023-01-10  9:13   ` [PATCH v3 " Christian Couder
  -- strict thread matches above, loose matches on Subject: below --
2023-01-09  9:02 [PATCH] ref-filter: add new atom " nsengaw4c via GitGitGadget
2023-01-09  9:45 ` Christian Couder
2023-01-09 12:59   ` NSENGIYUMVA WILBERFORCE

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo7rpvb83.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=nsengiyumvawilberforce@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).