Re: [PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

Eric,

On Mon, Jun 09, 2014 at 06:12:12AM -0400, Eric Sunshine wrote:

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler [off-list ref] wrote:

quoted

Subject: fast-import.c: cleanup using strbuf_set operations

...

quoted

Signed-off-by: Jeremiah Mahler <redacted>
---
 fast-import.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index e8ec34d..c23935c 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
        hashcpy(b->branch_tree.versions[0].sha1,
                b->branch_tree.versions[1].sha1);

-       strbuf_reset(&new_data);
-       strbuf_addf(&new_data, "tree %s\n",
+       strbuf_setf(&new_data, "tree %s\n",
                sha1_to_hex(b->branch_tree.versions[1].sha1));
        if (!is_null_sha1(b->sha1))
                strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));

Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or
returned immediately following the strbuf_set() call, I am not
convinced that this change is an improvement. This code has the
general form:

    strbuf_reset(...);
    strbuf_add(...);
    if (condition)
        strbuf_add(...);
    strbuf_add(...);

in which it is clear that the string is being built piecemeal, and
it's easy for a programmer to insert, remove, or re-order strbuf_add()
calls.

Replacing the first two lines with strbuf_set() somewhat obscures the
fact that the string is going to be built up piecemeal. Plus, the
change makes it more difficult to insert, remove, or re-order the
strbuf_add() invocations.

This isn't a strong objection, but the benefit of the change seems
minimal or non-existent.

Ditto for several remaining cases in this patch.

...

This is a great observation that I certainly did overlook.  Using
strbuf_add or strbuf_set to help make it more obvious what the code is
doing.

By the same token, strbuf_set can be used to replace strbuf_add to make
it clear that nothing important was being added to and that the entire
buffer is being replaced.

  struct strbuf mybuf = STRBUF_INIT;

  strbuf_add(&mybuf, ...);  /* Was something there before? */

  strbuf_set(&mybuf, ...);  /* Replace everything. */

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler