On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
