Re: [PATCH v2 00/19] add strbuf_set operations
Eric, On Wed, Jun 11, 2014 at 04:09:17AM -0400, Eric Sunshine wrote: > On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler wrote: > > Version 2 of the patch set to add strbuf_set operations. > > > > Includes suggestions from Eric Sunshine [1]: > > > > [...snip...] > > ... > > 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 > I did not realize this "code churn" aspect at the time but it makes sense now. These "mechanical" changes can be more trouble than they are worth. I will try to re-focus my changes to only those that have a substantial benefit. -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/19] add strbuf_set operations
On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
