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 jmmah...@gmail.com 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 jmmah...@gmail.com 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=gitm=140230874124060w=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
[PATCH v2 00/19] add strbuf_set operations
Version 2 of the patch set to add strbuf_set operations. Includes suggestions from Eric Sunshine [1]: - New operations and their documentation placed in one patch. - Less ambiguous documentation: Replace the buffer content with [...] - Use imperative mood in log messages. - Don't use strbuf_set operations in cases where strbuf_add is used to build up a buffer in multiple steps. Multiple adds suggest that re-ordering is possible whereas a strbuf_set at the beginning suggests that it isn't. This is confusing. 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=gitm=140230874124060w=2 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
