Re: [PATCH] git-for-each-ref: move GPG sigs off %(body) to %(signature).

[Michael J Gruber <git@drmicha.warpmail.net>]

Michał Górny venit, vidit, dixit 30.08.2011 10:57:

Welcome to yet another MG (and a fresh spelling of Michael) on the list ;)

> When grabbing a %(body) or %(contents) off a tag, one doesn't really
> expect to get the GPG signature as well (as it's basically useless
> without the complete signed text). Thus, strip it off those two tags,
> and make available via %(signature) if anyone needs it.

No, please do not change %(contents). It is the complete content which
(together with the header) enters into the sha1 calculation.

You will probably also face opposition as regards to %(body), changing
existing behaviour.

In fact, I wish we didn't have %(body) but %(contents:body) just like
other modifiers such as :short.

I think I'd go for

%(contents:signature)

and implement

%(contents:subject) the same as %(subject)
%(contents:body) as contents minus subject minus signature

and slowly deprecate %(subject) and %(body) (simply un-document for now).

Jeff will have more to say about this. Please see below for the sig
detection:

> Signed-off-by: Michał Górny <mgorny@gentoo.org>
> ---
>  Documentation/git-for-each-ref.txt |    3 ++-
>  builtin/for-each-ref.c             |   35 +++++++++++++++++++++++++++++------
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 152e695..7145c46 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -103,7 +103,8 @@ and `date` to extract the named component.
>  
>  The first line of the message in a commit and tag object is
>  `subject`, the remaining lines are `body`.  The whole message
> -is `contents`.
> +is `contents`.  If the message is GPG-signed, the signature is
> +`signature`.
>  
>  For sorting purposes, fields with numeric values sort in numeric
>  order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 89e75c6..fa5c292 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -10,6 +10,9 @@
>  #include "parse-options.h"
>  #include "remote.h"
>  
> +/* Used to strip signature off body */
> +#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----\n"
> +
>  /* Quoting styles */
>  #define QUOTE_NONE 0
>  #define QUOTE_SHELL 1
> @@ -69,6 +72,7 @@ static struct {
>  	{ "subject" },
>  	{ "body" },
>  	{ "contents" },
> +	{ "signature" },
>  	{ "upstream" },
>  	{ "symref" },
>  	{ "flag" },
> @@ -458,7 +462,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
>  	}
>  }
>  
> -static void find_subpos(const char *buf, unsigned long sz, const char **sub, const char **body)
> +static void find_subpos(const char *buf, unsigned long sz, const char **sub, const char **body, const char **sig)
>  {
>  	while (*buf) {
>  		const char *eol = strchr(buf, '\n');
> @@ -478,18 +482,34 @@ static void find_subpos(const char *buf, unsigned long sz, const char **sub, con
>  	buf = strchr(buf, '\n');
>  	if (!buf) {
>  		*body = "";
> +		*sig = *body;
>  		return; /* no body */
>  	}
>  	while (*buf == '\n')
>  		buf++; /* skip blank between subject and body */
>  	*body = buf;
> +	*sig = *body;
> +
> +	/* look for GPG signature */

Again I have to say no. Please look at

3d5854e (tag: recognize rfc1991 signatures, 2010-11-10)

which uses the factored out signature detection as introduced in the
previous commits. Thanks!

> +	while (*buf) {
> +		const char *eol = strchr(buf, '\n');
> +		if (!eol) {
> +			*sig = buf + strlen(buf);
> +			return;
> +		}
> +		if (!strncmp(eol + 1, PGP_SIGNATURE, ARRAY_SIZE(PGP_SIGNATURE)-1)) {
> +			*sig = eol + 1;
> +			break; /* found end of body */
> +		}
> +		buf = eol + 1;
> +	}
>  }
>  
>  /* See grab_values */
>  static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
>  {
>  	int i;
> -	const char *subpos = NULL, *bodypos = NULL;
> +	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
>  
>  	for (i = 0; i < used_atom_cnt; i++) {
>  		const char *name = used_atom[i];
> @@ -500,19 +520,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>  			name++;
>  		if (strcmp(name, "subject") &&
>  		    strcmp(name, "body") &&
> -		    strcmp(name, "contents"))
> +		    strcmp(name, "contents") &&
> +		    strcmp(name, "signature"))
>  			continue;
>  		if (!subpos)
> -			find_subpos(buf, sz, &subpos, &bodypos);
> +			find_subpos(buf, sz, &subpos, &bodypos, &sigpos);
>  		if (!subpos)
>  			return;
>  
>  		if (!strcmp(name, "subject"))
>  			v->s = copy_line(subpos);
>  		else if (!strcmp(name, "body"))
> -			v->s = xstrdup(bodypos);
> +			v->s = xstrndup(bodypos, sigpos - bodypos);
>  		else if (!strcmp(name, "contents"))
> -			v->s = xstrdup(subpos);
> +			v->s = xstrndup(subpos, sigpos - subpos);
> +		else if (!strcmp(name, "signature"))
> +			v->s = xstrdup(sigpos);
>  	}
>  }
>