Jeff King <peff@xxxxxxxx> writes: > On Tue, Mar 28, 2017 at 03:33:48PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@xxxxxxxx> writes: >> >> > It's a lot of patches, but hopefully they're all pretty straightforward >> > to read. >> >> Yes, quite a lot of changes. I didn't see anything questionable in >> there. >> >> As to the "patch-id" thing, I find the alternate one slightly easier >> to read. Also, exactly because this is not a performance critical >> codepath, it may be better if patch_id_add_string() filtered out >> whitespaces; that would allow the source to express things in more >> natural way, e.g. >> >> patch_id_addf(&ctx, "new file mode"); >> patch_id_addf(&ctx, "%06o", p->two->mode); >> patch_id_addf(&ctx, "--- /dev/null"); >> patch_id_addf(&ctx, "+++ b/%.*s", len2, p->two->path); >> >> Or I may be going overboard by bringing "addf" into the mix X-<. > > 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.