Re: [PATCH 3/9] ref-filter: strip signature when parsing tag trailers

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 09, 2024 at 07:14:45PM -0400, Jeff King wrote:
> The implementation here is pretty simple: we just make a NUL-terminated
> copy of the non-signature part of the tag (which we've already parsed)
> and pass it to the trailer API. There are some alternatives I rejected,
> at least for now:
> 
>   - the trailer code already understands skipping past some cruft at the
>     end of a commit, such as patch dividers. see find_end_of_log_message().

s/./,

>     We could teach it to do the same for signatures. But since this is
>     the only context where we'd want that feature, and since we've already
>     parsed the object into subject/body/signature here, it seemed easier
>     to just pass in the truncated message.
> 
>   - it would be nice if we could just pass in a pointer/len pair to the

s/it/It

> diff --git a/ref-filter.c b/ref-filter.c
> index 0f5513ba7e..e39f505a81 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2008,9 +2008,17 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
>  			v->s = strbuf_detach(&s, NULL);
>  		} else if (atom->u.contents.option == C_TRAILERS) {
>  			struct strbuf s = STRBUF_INIT;
> +			const char *msg;
> +			char *to_free = NULL;
> +
> +			if (siglen)
> +				msg = to_free = xmemdupz(subpos, sigpos - subpos);
> +			else
> +				msg = subpos;
>  
>  			/* Format the trailer info according to the trailer_opts given */
> -			format_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);
> +			format_trailers_from_commit(&atom->u.contents.trailer_opts, msg, &s);
> +			free(to_free);
>  
>  			v->s = strbuf_detach(&s, NULL);
>  		} else if (atom->u.contents.option == C_BARE)

I've been surprised that we use `subpos` as the starting point here,
which includes the whole message including its subject. I would have
thought that it was sufficient to only pass the message body as input,
which saves allocating some bytes. At least `trailer_info_get()` does
not seem to care about the subject at all.

In any case this would be a micro-optimization anyway, it just left me
scratching my head for a second or two.

Patrick