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

From: Jeremiah Mahler <jmmahler@gmail.com>
Date: Jun 9, 2014, 22:00 UTC
Message-ID: <20140609220039.GB18783@hudson.localdomain>
In-Reply-To: <CAPig+cRPPN7N+KUHULXxoGHetN8WPtASxnC7L1fe5rFoBFQ4KA@mail.gmail.com>

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 <jmmahler@gmail.com> wrote:
> > Subject: fast-import.c: cleanup using strbuf_set operations
>

...

Show 46 quoted lines

> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  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