On Tue, Sep 10, 2024 at 08:08:55AM +0200, Patrick Steinhardt wrote:
> > 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.
Yeah, I suspect it would be fine to start at "bodypos" instead (and then
you could just use "nonsiglen" directly as the length). But there may be
corner cases for instances where there's no subject/body at all (though
having _just_ trailers feels weird).
At any rate, I was just following the existing code, which passed in the
whole contents starting from subpos, to avoid any unexpected changes.
-Peff
