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@xxxxxxxxx> wrote: > > Subject: fast-import.c: cleanup using strbuf_set operations > ... > > > Signed-off-by: Jeremiah Mahler <jmmahler@xxxxxxxxx> > > --- > > 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@xxxxxxxxx http://github.com/jmahler -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html