Re: [PATCH 12/15] use get_commit_buffer everywhere

[Jeff King <peff@peff.net>]

On Tue, Jun 10, 2014 at 09:06:35AM -0700, Junio C Hamano wrote:

> > So any call to strbuf_detach on the result would be disastrous.
> 
> You are right.  Where did this original crap come from X-<...

I do not know if that face means you actually looked at the history, but
in case you did not...

It was added by Duy's 13f8b72 (Convert commit_tree() to take strbuf as
message, 2011-12-15). However that was v2 of his patch. If you read the
original thread, you can see that v1 passed a separate pointer/length
pair, and was only changed after a reviewer-who-shall-not-be-named asked
him to change it. ;)

Of course there were many people participating in the review, and none
of us noticed it. I think it is simply a subtle bug.

> > I feel like the most elegant solution is for create_notes_commit to take
> > a buf/len pair rather than a strbuf, but it unfortunately is just
> > feeding that to commit_tree. Adjusting that code path would affect quite
> > a few other spots.
> >
> > The other obvious option is actually populating the strbuf, but it feels
> > ugly to have to make a copy just to satisfy the function interface.
> >
> > Maybe a cast and a warning comment are the least evil thing, as below? I
> > dunno, it feels pretty wrong.
> 
> Yeah, it does feel wrong wrong wrong.  Perhaps this big comment
> would serve as a marker for a low-hanging fruit for somebody else to
> fix it, e.g. by using strbuf-add to make a copy, which would be the
> easiest and safest workaround?

I really think commit_tree is the culprit here. It doesn't actually want
a strbuf at all, but takes one to make passing the pointer/len pair
simpler. Fixing it turned out to be not _too_ disruptive, and it showed
that there is another dubious use of strbuf_attach from a different
caller.

I'll post my re-rolled series with those fixes in a moment.

-Peff