On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmahler@xxxxxxxxx> wrote: > Version 2 of the patch set to add strbuf_set operations. > > Includes suggestions from Eric Sunshine [1]: > > [...snip...] > > Using strbuf_set before a sequence of adds can be confusing. But using > strbuf_add when nothing important was being added to can also be > confusing. strbuf_set can be used to make these cases more clear. > > struct strbuf mybuf = STRBUF_INIT; > > strbuf_add(&mybuf, ...); /* Was something there before? */ > > strbuf_set(&mybuf, ...); /* Replace everything. */ > > Several single line replacements were made for this reason. > > Additional files have also been converted compared to version 1 [1]. > > [1]: http://marc.info/?l=git&m=140230874124060&w=2 Food for thought: This patch series has now likely entered the territory of unnecessary code churn. Quoting from Documentation/CodingGuidelines: Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style. "Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up." Cf. http://article.gmane.org/gmane.linux.kernel/943020 Rewriting code merely for the sake of replacing strbuf_reset() + strbuf_add() with strbuf_set(), while not exactly a style cleanup, falls into the same camp. (The patch which introduces strbuf_set(), however, is not churn.) Each change you make can potentially conflict with other topics already in-flight (see [1], for example), thus making more work for Junio. Also, these sorts of apparently innocuous "mechanical" changes can have hidden costs [2], so it's useful to weigh carefully how lengthy you want to make this patch series. Having said that, patch 4/19, particularly the changes to builtin/remote.c:mv(), in which multiple strbufs are reused repeatedly, does seem to make the code a bit easier to read and likely reduces cognitive load. [1]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245147 [2]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245144 > Jeremiah Mahler (19): > add strbuf_set operations > sha1_name: simplify via strbuf_set() > fast-import: simplify via strbuf_set() > builtin/remote: simplify via strbuf_set() > branch: simplify via strbuf_set() > builtin/branch: simplify via strbuf_set() > builtin/checkout: simplify via strbuf_set() > builtin/mailinfo: simplify via strbuf_set() > builtin/tag: simplify via strbuf_set() > date: simplify via strbuf_set() > diffcore-order: simplify via strbuf_set() > http-backend: simplify via strbuf_set() > http: simplify via strbuf_set() > ident: simplify via strbuf_set() > remote-curl: simplify via strbuf_set() > submodule: simplify via strbuf_set() > transport: simplify via strbuf_set() > vcs-svn/svndump: simplify via strbuf_set() > wt-status: simplify via strbuf_set() > > Documentation/technical/api-strbuf.txt | 18 +++++++++++ > branch.c | 6 ++-- > builtin/branch.c | 3 +- > builtin/checkout.c | 18 ++++------- > builtin/mailinfo.c | 18 ++++------- > builtin/remote.c | 59 ++++++++++++---------------------- > builtin/tag.c | 3 +- > date.c | 3 +- > diffcore-order.c | 3 +- > fast-import.c | 6 ++-- > http-backend.c | 6 ++-- > http.c | 3 +- > ident.c | 6 ++-- > remote-curl.c | 3 +- > sha1_name.c | 15 +++------ > strbuf.c | 21 ++++++++++++ > strbuf.h | 14 ++++++++ > submodule.c | 5 ++- > transport.c | 3 +- > vcs-svn/svndump.c | 3 +- > wt-status.c | 3 +- > 21 files changed, 110 insertions(+), 109 deletions(-) > > -- > 2.0.0.592.gf55b190 -- 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