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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html