Re: [PATCH] for-each-ref: add split message parts to %(contents:*).

[Junio C Hamano <gitster@pobox.com>]

Michał Górny <mgorny@gentoo.org> writes:

> Now %(contents:subject) contains the message subject, %(contents:body)
> main body part and %(contents:signature) GPG signature.
> ---

Please sign-off when submitting the final round of this patch.

> +The complete message in a commit and tag object is `contents`.
> +Its first line is `contents:subject`, the remaining lines
> +are `contents:body` and the optional GPG signature
> +is `contents:signature`.

To match the parsing of commit objects, I would prefer to see "subject" to
mean "the first paragraph" (usually the first line alone but that is
purely from convention), but that probably is a separate topic.

To paraphrase the last part of your sentence, if a tag is merely annotated
and not signed, contents:signature would be empty (I am just making sure
that I am reading the description correctly).

Is it possible to get %(contents) with a combination of these three new
variants, or does the calling script need to see if each part is empty and
decide where to place newlines?  IOW, a naïve attempt:

	--format='%(contents:subject)\n%(contents:body)\n%(contents:signature)'

is not equivalent to

	--format='%(contents)'

right?

> @@ -478,18 +481,20 @@ static void find_subpos(const char *buf, unsigned long sz, const char **sub, con
>  	buf = strchr(buf, '\n');
>  	if (!buf) {
>  		*body = "";
> +		*signature = *body;
>  		return; /* no body */
>  	}
>  	while (*buf == '\n')
>  		buf++; /* skip blank between subject and body */
>  	*body = buf;
> +	*signature = buf + parse_signature(buf, strlen(buf));

If there is no signature, parse_signature() would return (size_t) 0, no?
I suspect it may be easier for the caller if *signature pointed at the
terminating NUL at the end of the whole thing in such a case. Otherwise,
the caller that finds the buf and the signature points at the same address
cannot tell if the object was a signed tag with no message

	$ git tag -s -m "" empty-tag

or if it was an annotated but not signed tag.

Or better yet, you may even want to stuff NULL there if there is no signature
so that it is absolutely clear for the caller which case it is.

>  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 +505,26 @@ 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, "contents:subject") &&
> +		    strcmp(name, "contents:body") &&
> +		    strcmp(name, "contents:signature"))
>  			continue;
>  		if (!subpos)
> -			find_subpos(buf, sz, &subpos, &bodypos);
> +			find_subpos(buf, sz, &subpos, &bodypos, &sigpos);
>  		if (!subpos)
>  			return;
>  
> -		if (!strcmp(name, "subject"))
> +		if (!strcmp(name, "subject") || !strcmp(name, "contents:subject"))
>  			v->s = copy_line(subpos);
>  		else if (!strcmp(name, "body"))
>  			v->s = xstrdup(bodypos);
>  		else if (!strcmp(name, "contents"))
>  			v->s = xstrdup(subpos);
> +		else if (!strcmp(name, "contents:body"))
> +			v->s = xstrndup(bodypos, sigpos - bodypos);
> +		else if (!strcmp(name, "contents:signature"))
> +			v->s = xstrdup(sigpos);

Again, how does this work for an annotated but not signed tag?