Jeff King <p...@peff.net> writes: > On Wed, Mar 29, 2017 at 09:05:33AM -0700, Junio C Hamano wrote: > >> > I think there are two things going on in your example. >> > >> > One is that obviously patch_id_addf() removes the spaces from the >> > result. But we could do that now by keeping the big strbuf_addf(), and >> > then just walking the result and feeding non-spaces. >> > >> > The second is that your addf means we are back to formatting everything >> > into a buffer again.... >> >> You are right to point out that I was blinded by the ugliness of >> words stuck together without spaces in between, which was inherited >> from the original code, and failed to see the sole point of this >> series, which is to remove truncation without adding unnecessary >> allocation and freeing. >> >> Thanks for straighten my thinking out. I think the seeming >> ugliness, if it ever becomes a real problem, should be handled >> outside this series after the dust settles. > > Yeah, the no-spaces thing should almost certainly wait. > > There is still the minor question of whether skipping the strbuf > entirely is nicer, even if you still have to feed it strings without > spaces (i.e., what I posted in my initial reply). > > I'm OK either with the series I posted, or wrapping up the alternative > in a commit message.
I do find the updated one easier to follow (if anything it is more compact); I do not think it is worth a reroll, but it is easy enough to replace the patch part of the original with the updated patch and tweak "it's easy to fix by moving to a strbuf" in its log message to something like "But it's easy to eliminate the allocation with a few helper functions, and it makes the result easier to follow", so I am tempted to go that route myself...
