v3, 2 partsadd strbuf_set operations

> A common use case with strubfs is to set the buffer to a new value.
> This must be done in two steps: a reset followed by an add.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, new_buf, len);
> 
> In cases where the buffer is being built up in steps, these operations
> make sense and correctly convey what is being performed.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, data1, len1);
>   strbuf_add(buf, data2, len2);
>   strbuf_add(buf, data3, len3);
> 
> However, in other cases, it can be confusing and is not very concise.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, default, len1);
> 
>   if (cond1) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data2, len2);
>   }
> 
>   if (cond2) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data3, len3);
>   }
> 
> Add strbuf_set operations so that it can be re-written in a clear and
> concise way.
> 
>   strbuf_set(buf, default len1);

> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> > A common use case with strubfs is to set the buffer to a new value.
> > This must be done in two steps: a reset followed by an add.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, new_buf, len);
> > 
> > In cases where the buffer is being built up in steps, these operations
> > make sense and correctly convey what is being performed.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, data1, len1);
> >   strbuf_add(buf, data2, len2);
> >   strbuf_add(buf, data3, len3);
> > 
> > However, in other cases, it can be confusing and is not very concise.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, default, len1);
> > 
> >   if (cond1) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data2, len2);
> >   }
> > 
> >   if (cond2) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data3, len3);
> >   }
> > 
> > Add strbuf_set operations so that it can be re-written in a clear and
> > concise way.
> > 
> >   strbuf_set(buf, default len1);
> very minor nit: missing comma between default and len1.

> Thomas,
>
> On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
>> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
>> > A common use case with strubfs is to set the buffer to a new value.

>> > This must be done in two steps: a reset followed by an add.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, new_buf, len);
>> > 
>> > In cases where the buffer is being built up in steps, these operations
>> > make sense and correctly convey what is being performed.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, data1, len1);
>> >   strbuf_add(buf, data2, len2);
>> >   strbuf_add(buf, data3, len3);
>> > 
>> > However, in other cases, it can be confusing and is not very concise.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, default, len1);
>> > 
>> >   if (cond1) {
>> >     strbuf_reset(buf);
>> >     strbuf_add(buf, data2, len2);
>> >   }
>> > 
>> >   if (cond2) {
>> >     strbuf_reset(buf);
>> >     strbuf_add(buf, data3, len3);
>> >   }
>> > 
>> > Add strbuf_set operations so that it can be re-written in a clear and
>> > concise way.
>> > 
>> >   strbuf_set(buf, default len1);
>> very minor nit: missing comma between default and len1.
>
> I can't believe I missed that.  Good catch ;-)

> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > Thomas,
> >
> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> >> > A common use case with strubfs is to set the buffer to a new value.
> 
> strubfs???
> 

> On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:
>> Jeremiah Mahler <jmmahler@gmail.com> writes:
>>
>> > Thomas,
>> >
>> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
>> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
>> >> > A common use case with strubfs is to set the buffer to a new value.
>>
>> strubfs???
>>
> I was trying to make it plural.  Perhaps strbuf's?

> On Thu, Jun 12, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:
> >> Jeremiah Mahler <jmmahler@gmail.com> writes:
> >>
> >> > Thomas,
> >> >
> >> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
> >> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> >> >> > A common use case with strubfs is to set the buffer to a new value.
> >>
> >> strubfs???
> >>
> > I was trying to make it plural.  Perhaps strbuf's?
> 
> Junio was pointing out your misspelling, not your intention to pluralize.

> A common use case with strubfs is to set the buffer to a new value.
> This must be done in two steps: a reset followed by an add.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, new_buf, len);
>
> In cases where the buffer is being built up in steps, these operations
> make sense and correctly convey what is being performed.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, data1, len1);
>   strbuf_add(buf, data2, len2);
>   strbuf_add(buf, data3, len3);
>
> However, in other cases, it can be confusing and is not very concise.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, default, len1);
>
>   if (cond1) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data2, len2);
>   }
>
>   if (cond2) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data3, len3);
>   }
>
> Add strbuf_set operations so that it can be re-written in a clear and
> concise way.
>
>   strbuf_set(buf, default len1);
>
>   if (cond1) {
>     strbuf_set(buf, data2, len2);
>   }
>
>   if (cond2) {
>     strbuf_set(buf, data3, len3);
>   }

> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>  strbuf.c                               | 21 +++++++++++++++++++++
>  strbuf.h                               | 13 +++++++++++++
>  3 files changed, 52 insertions(+)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index f9c06a7..ae9c9cc 100644
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -149,6 +149,24 @@ Functions
>  	than zero if the first buffer is found, respectively, to be less than,
>  	to match, or be greater than the second buffer.
>  
> +* Setting the buffer
> +
> +`strbuf_set`::
> +
> +	Replace content with data of a given length.
> +
> +`strbuf_setstr`::
> +
> +	Replace content with data from a NUL-terminated string.
> +
> +`strbuf_setf`::
> +
> +	Replace content with a formatted string.
> +
> +`strbuf_setbuf`::
> +
> +	Replace content with data from another buffer.
> +
>  * Adding data to the buffer
>  
>  NOTE: All of the functions in this section will grow the buffer as necessary.
> diff --git a/strbuf.c b/strbuf.c
> index ac62982..9d64b00 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
>  	strbuf_setlen(sb, sb->len + dlen - len);
>  }
>  
> +void strbuf_set(struct strbuf *sb, const void *data, size_t len)
> +{
> +	strbuf_reset(sb);
> +	strbuf_add(sb, data, len);
> +}
> +
> +void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
> +{
> +	va_list ap;
> +	strbuf_reset(sb);
> +	va_start(ap, fmt);
> +	strbuf_vaddf(sb, fmt, ap);
> +	va_end(ap);
> +}
> +
> +void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
> +{
> +	strbuf_reset(sb);
> +	strbuf_add(sb, sb2->buf, sb2->len);
> +}
> +
>  void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
>  {
>  	strbuf_splice(sb, pos, 0, data, len);
> diff --git a/strbuf.h b/strbuf.h
> index e9ad03e..5041c35 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -101,6 +101,19 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
>   */
>  extern void strbuf_list_free(struct strbuf **);
>  
> +/*----- set buffer to data -----*/
> +extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
> +
> +static inline void strbuf_setstr(struct strbuf *sb, const char *s)
> +{
> +	strbuf_set(sb, s, strlen(s));
> +}
> +
> +__attribute__((format (printf,2,3)))
> +extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
> +
> +extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
> +
>  /*----- add data in your buffer -----*/
>  static inline void strbuf_addch(struct strbuf *sb, int c)
>  {

> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > A common use case with strubfs is to set the buffer to a new value.
> > This must be done in two steps: a reset followed by an add.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, new_buf, len);
> >
> > In cases where the buffer is being built up in steps, these operations
> > make sense and correctly convey what is being performed.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, data1, len1);
> >   strbuf_add(buf, data2, len2);
> >   strbuf_add(buf, data3, len3);
> >
> > However, in other cases, it can be confusing and is not very concise.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, default, len1);
> >
> >   if (cond1) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data3, len3);
> >   }
> >
> > Add strbuf_set operations so that it can be re-written in a clear and
> > concise way.
> >
> >   strbuf_set(buf, default len1);
> >
> >   if (cond1) {
> >     strbuf_set(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_set(buf, data3, len3);
> >   }
> 
> Or even more concisely without making unnecessary internal calls to
> strbuf_reset():
> 
> 	strbuf_reset(buf);
>         if (cond2)
>         	strbuf_add(buf, data3, len3);
> 	else if (cond1)
>         	strbuf_add(buf, data2, len2);
> 	else
>         	strbuf_add(buf, default, len2);
> 
> ;-)

> 
> I am on the fence.
> 
> I have this suspicion that the addition of strbuf_set() would *only*
> help when the original written with reset-and-then-add sequence was
> suboptimal to begin with, and it helps *only* how the code reads,
> without correcting the fact that it is still doing unnecessary
> "first set to a value to be discarded and then reset to set the
> right value", sweeping the issue under the rug.
> 

> Repeated reset-and-then-add on the same strbuf used to be something
> that may indicate that the code is doing unnecessary work.  Now,
> repeated uses of strbuf_set on the same strbuf replaced that pattern
> to be watched for to spot wasteful code paths.
> 

> I dunno...
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >  strbuf.c                               | 21 +++++++++++++++++++++
> >  strbuf.h                               | 13 +++++++++++++
> >  3 files changed, 52 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> > index f9c06a7..ae9c9cc 100644
> > --- a/Documentation/technical/api-strbuf.txt
> > +++ b/Documentation/technical/api-strbuf.txt
> > @@ -149,6 +149,24 @@ Functions
> >  	than zero if the first buffer is found, respectively, to be less than,
> >  	to match, or be greater than the second buffer.
> >  /*----- add data in your buffer -----*/

> On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
>> I am on the fence.
>>
>> I have this suspicion that the addition of strbuf_set() would *only*
>> help when the original written with reset-and-then-add sequence was
>> suboptimal to begin with, and it helps *only* how the code reads,
>> without correcting the fact that it is still doing unnecessary
>> "first set to a value to be discarded and then reset to set the
>> right value", sweeping the issue under the rug.
>>
> It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> benefit from this operation because it is so badly designed.  But this
> seems unlikely given the review process around here ;-)
>
> The one case where it would be doing extra work is when a strbuf_set
> replaces a strbuf_add that didn't previously have a strbuf_reset.
> strbuf_set is not appropriate for all cases, as I mentioned in the
> patch, but in some cases I think it makes it more readable.  And in this
> case it would be doing a reset on an empty strbuf.  Is avoiding this
> expense worth the reduction in readability?
>
> Also, as Eric Sunshine pointed out, being able to easily re-order
> operations can make the code easier to maintain.
>
>> Repeated reset-and-then-add on the same strbuf used to be something
>> that may indicate that the code is doing unnecessary work.  Now,
>> repeated uses of strbuf_set on the same strbuf replaced that pattern
>> to be watched for to spot wasteful code paths.
>>
> If a reset followed by and add was a rare occurrence I would tend to
> agree more.

>> I dunno...
>>
>> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
>> > ---
>> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>> >  strbuf.c                               | 21 +++++++++++++++++++++
>> >  strbuf.h                               | 13 +++++++++++++
>> >  3 files changed, 52 insertions(+)
>> >
>> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
>> > index f9c06a7..ae9c9cc 100644
>> > --- a/Documentation/technical/api-strbuf.txt
>> > +++ b/Documentation/technical/api-strbuf.txt
>> > @@ -149,6 +149,24 @@ Functions
>> >     than zero if the first buffer is found, respectively, to be less than,
>> >     to match, or be greater than the second buffer.
>> >  /*----- add data in your buffer -----*/
> ...
>> >  static inline void strbuf_addch(struct strbuf *sb, int c)
>> >  {
>
> --
> Jeremiah Mahler
> jmmahler@gmail.com
> http://github.com/jmahler

> On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
> >> I am on the fence.
> >>
> >> I have this suspicion that the addition of strbuf_set() would *only*
> >> help when the original written with reset-and-then-add sequence was
> >> suboptimal to begin with, and it helps *only* how the code reads,
> >> without correcting the fact that it is still doing unnecessary
> >> "first set to a value to be discarded and then reset to set the
> >> right value", sweeping the issue under the rug.
> >>
> > It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> > benefit from this operation because it is so badly designed.  But this
> > seems unlikely given the review process around here ;-)
> >
> > The one case where it would be doing extra work is when a strbuf_set
> > replaces a strbuf_add that didn't previously have a strbuf_reset.
> > strbuf_set is not appropriate for all cases, as I mentioned in the
> > patch, but in some cases I think it makes it more readable.  And in this
> > case it would be doing a reset on an empty strbuf.  Is avoiding this
> > expense worth the reduction in readability?
> >
> > Also, as Eric Sunshine pointed out, being able to easily re-order
> > operations can make the code easier to maintain.
> >
> >> Repeated reset-and-then-add on the same strbuf used to be something
> >> that may indicate that the code is doing unnecessary work.  Now,
> >> repeated uses of strbuf_set on the same strbuf replaced that pattern
> >> to be watched for to spot wasteful code paths.
> >>
> > If a reset followed by and add was a rare occurrence I would tend to
> > agree more.
> 
> When composing my review of the builtin/remote.c changes, I wrote
> something like this:
> 
>     Although strbuf_set() does make the code a bit easier to read
>     when strbufs are repeatedly re-used, re-using a variable for
>     different purposes is generally considered poor programming
>     practice. It's likely that heavy re-use of strbufs has been
>     tolerated to avoid multiple heap allocations, but that may be a
>     case of premature (allocation) optimization, rather than good
>     programming. A different ("better") way to make the code more
>     readable and maintainable may be to ban re-use of strbufs for
>     different purposes.
> 
> But I deleted it before sending because it's a somewhat tangential
> issue not introduced by your changes. However, I do see strbuf_set()
> as a Band-Aid for the problem described above, rather than as a useful
> feature on its own. If the practice of re-using strbufs (as a
> premature optimization) ever becomes taboo, then strbuf_set() loses
> its value.
> 

> >> I dunno...
> >>
> >> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> >> > ---
> >> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >> >  strbuf.c                               | 21 +++++++++++++++++++++
> >> >  strbuf.h                               | 13 +++++++++++++
> >> >  3 files changed, 52 insertions(+)
> >> >
> >> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> >> > index f9c06a7..ae9c9cc 100644
> >> > --- a/Documentation/technical/api-strbuf.txt
> >> > +++ b/Documentation/technical/api-strbuf.txt
> >> > @@ -149,6 +149,24 @@ Functions
> >> >     than zero if the first buffer is found, respectively, to be less than,
> >> >     to match, or be greater than the second buffer.
> >> >  /*----- add data in your buffer -----*/
> > ...
> >> >  static inline void strbuf_addch(struct strbuf *sb, int c)
> >> >  {
> >
> > --
> > Jeremiah Mahler
> > jmmahler@gmail.com
> > http://github.com/jmahler

> >     Although strbuf_set() does make the code a bit easier to read
> >     when strbufs are repeatedly re-used, re-using a variable for
> >     different purposes is generally considered poor programming
> >     practice. It's likely that heavy re-use of strbufs has been
> >     tolerated to avoid multiple heap allocations, but that may be a
> >     case of premature (allocation) optimization, rather than good
> >     programming. A different ("better") way to make the code more
> >     readable and maintainable may be to ban re-use of strbufs for
> >     different purposes.
> > 
> > But I deleted it before sending because it's a somewhat tangential
> > issue not introduced by your changes. However, I do see strbuf_set()
> > as a Band-Aid for the problem described above, rather than as a useful
> > feature on its own. If the practice of re-using strbufs (as a
> > premature optimization) ever becomes taboo, then strbuf_set() loses
> > its value.
> > 
> 
> I am getting the feeling that I have mis-understood the purpose of
> strbufs.  It is not just a library to use in place of char*.
> 
> If strbufs should only be added to and never reset a good test would be
> to re-write builtin/remote.c without the use of strbuf_reset.
> 
> builtin/remote.c does re-use the buffers.  But it seems if a buffer is
> used N times then to avoid a reset you would need N buffers.
> 
> But on the other hand I agree with your comment that re-using a variable
> for different purposes is poor practice.
> 
> Now I am not even sure if I want my own patch :-)

> On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:
> 
> > >     Although strbuf_set() does make the code a bit easier to read
> > >     when strbufs are repeatedly re-used, re-using a variable for
> > >     different purposes is generally considered poor programming
> > >     practice. It's likely that heavy re-use of strbufs has been
> > >     tolerated to avoid multiple heap allocations, but that may be a
> > >     case of premature (allocation) optimization, rather than good
> > >     programming. A different ("better") way to make the code more
> > >     readable and maintainable may be to ban re-use of strbufs for
> > >     different purposes.
> > > 
> > > But I deleted it before sending because it's a somewhat tangential
> > > issue not introduced by your changes. However, I do see strbuf_set()
> > > as a Band-Aid for the problem described above, rather than as a useful
> > > feature on its own. If the practice of re-using strbufs (as a
> > > premature optimization) ever becomes taboo, then strbuf_set() loses
> > > its value.
> > > 
> > 
> > I am getting the feeling that I have mis-understood the purpose of
> > strbufs.  It is not just a library to use in place of char*.
> > 
> > If strbufs should only be added to and never reset a good test would be
> > to re-write builtin/remote.c without the use of strbuf_reset.
> > 
> > builtin/remote.c does re-use the buffers.  But it seems if a buffer is
> > used N times then to avoid a reset you would need N buffers.
> > 
> > But on the other hand I agree with your comment that re-using a variable
> > for different purposes is poor practice.
> > 
> > Now I am not even sure if I want my own patch :-)
> 
> I think reusing buffers like remote.c does makes things uglier and more
> confusing than necessary, and probably doesn't have any appreciable
> performance gain. We are saving a handful of allocations, and have such
> wonderful variable names as "buf2" (when is it OK to reuse that one,
> versus regular "buf"?).
> 
> A better reason I think is to reuse allocations across a loop, like:
> 
>   struct strbuf buf = STRBUF_INIT;
> 
>   for (i = 0; i < nr; i++) {
> 	strbuf_reset(&buf);
> 	strbuf_add(&buf, foo[i]);
> 	... do something with buf ...
>   }
>   strbuf_release(&buf);
> 
> You can write that as:
> 
>   for (i = 0; i < nr; i++) {
> 	struct strbuf buf = STRBUF_INIT;
> 	strbuf_add(&buf, foo[i]);
> 	... do something ...
> 	strbuf_release(&buf);
>   }
> 
> and it is definitely still a case of premature optimization. But:
> 
>   1. "nr" here may be very large, so the amortized benefits are greater
> 
>   2. It's only one call to strbuf_reset to cover many items. Not one
>      sprinkled every few lines.
> 
> You'll note that strbuf_getline uses a "set" convention (making it
> different from the rest of strbuf) to handle this looping case.
> 
> I don't have a problem with strbuf_set, but just peeking at remote.c, I
> think I'd rather see it cleaned up. It looks like one of the major uses
> is setting config variables. I wonder how hard it would be to make a
> git_config_set variant that took printf-style formats, and handled the
> strbuf itself.
> 
> -Peff

> builtin/remote.c has many cases where a strbuf was being set to a new
> value.  Improve its readability by using strbuf_set() operations instead
> of reset + add.
>
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  builtin/remote.c | 63 +++++++++++++++++++++-----------------------------------
>  1 file changed, 23 insertions(+), 40 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index c9b67ff..94f03e2 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -184,17 +184,16 @@ static int add(int argc, const char **argv)
>                         remote->fetch_refspec_nr))
>                 die(_("remote %s already exists."), name);
>
> -       strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
> +       strbuf_setf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
>         if (!valid_fetch_refspec(buf2.buf))
>                 die(_("'%s' is not a valid remote name"), name);
>
> -       strbuf_addf(&buf, "remote.%s.url", name);
> +       strbuf_setf(&buf, "remote.%s.url", name);
>         if (git_config_set(buf.buf, url))
>                 return 1;
>
>         if (!mirror || mirror & MIRROR_FETCH) {
> -               strbuf_reset(&buf);
> -               strbuf_addf(&buf, "remote.%s.fetch", name);
> +               strbuf_setf(&buf, "remote.%s.fetch", name);

>                 if (track.nr == 0)
>                         string_list_append(&track, "*");
>                 for (i = 0; i < track.nr; i++) {
> @@ -205,15 +204,13 @@ static int add(int argc, const char **argv)
>         }
>
>         if (mirror & MIRROR_PUSH) {
> -               strbuf_reset(&buf);
> -               strbuf_addf(&buf, "remote.%s.mirror", name);
> +               strbuf_setf(&buf, "remote.%s.mirror", name);
>                 if (git_config_set(buf.buf, "true"))
>                         return 1;
>         }
>
>         if (fetch_tags != TAGS_DEFAULT) {
> -               strbuf_reset(&buf);
> -               strbuf_addf(&buf, "remote.%s.tagopt", name);
> +               strbuf_setf(&buf, "remote.%s.tagopt", name);
>                 if (git_config_set(buf.buf,
>                         fetch_tags == TAGS_SET ? "--tags" : "--no-tags"))
>                         return 1;
> @@ -223,11 +220,9 @@ static int add(int argc, const char **argv)
>                 return 1;
>
>         if (master) {
> -               strbuf_reset(&buf);
> -               strbuf_addf(&buf, "refs/remotes/%s/HEAD", name);
> +               strbuf_setf(&buf, "refs/remotes/%s/HEAD", name);
>
> -               strbuf_reset(&buf2);
> -               strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);
> +               strbuf_setf(&buf2, "refs/remotes/%s/%s", name, master);
>
>                 if (create_symref(buf.buf, buf2.buf, "remote add"))
>                         return error(_("Could not setup master '%s'"), master);
> @@ -584,19 +579,17 @@ static int migrate_file(struct remote *remote)
>         int i;
>         const char *path = NULL;
>
> -       strbuf_addf(&buf, "remote.%s.url", remote->name);
> +       strbuf_setf(&buf, "remote.%s.url", remote->name);
>         for (i = 0; i < remote->url_nr; i++)
>                 if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
>                         return error(_("Could not append '%s' to '%s'"),
>                                         remote->url[i], buf.buf);
> -       strbuf_reset(&buf);
> -       strbuf_addf(&buf, "remote.%s.push", remote->name);
> +       strbuf_setf(&buf, "remote.%s.push", remote->name);
>         for (i = 0; i < remote->push_refspec_nr; i++)
>                 if (git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0))
>                         return error(_("Could not append '%s' to '%s'"),
>                                         remote->push_refspec[i], buf.buf);
> -       strbuf_reset(&buf);
> -       strbuf_addf(&buf, "remote.%s.fetch", remote->name);
> +       strbuf_setf(&buf, "remote.%s.fetch", remote->name);
>         for (i = 0; i < remote->fetch_refspec_nr; i++)
>                 if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0))
>                         return error(_("Could not append '%s' to '%s'"),
> @@ -640,27 +633,24 @@ static int mv(int argc, const char **argv)
>         if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
>                 die(_("remote %s already exists."), rename.new);
>
> -       strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
> +       strbuf_setf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
>         if (!valid_fetch_refspec(buf.buf))
>                 die(_("'%s' is not a valid remote name"), rename.new);
>
> -       strbuf_reset(&buf);
> -       strbuf_addf(&buf, "remote.%s", rename.old);
> -       strbuf_addf(&buf2, "remote.%s", rename.new);
> +       strbuf_setf(&buf, "remote.%s", rename.old);
> +       strbuf_setf(&buf2, "remote.%s", rename.new);
>         if (git_config_rename_section(buf.buf, buf2.buf) < 1)
>                 return error(_("Could not rename config section '%s' to '%s'"),
>                                 buf.buf, buf2.buf);
>
> -       strbuf_reset(&buf);
> -       strbuf_addf(&buf, "remote.%s.fetch", rename.new);
> +       strbuf_setf(&buf, "remote.%s.fetch", rename.new);
>         if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
>                 return error(_("Could not remove config section '%s'"), buf.buf);
> -       strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
> +       strbuf_setf(&old_remote_context, ":refs/remotes/%s/", rename.old);
>         for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
>                 char *ptr;
>
> -               strbuf_reset(&buf2);
> -               strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
> +               strbuf_setstr(&buf2, oldremote->fetch_refspec[i]);
>                 ptr = strstr(buf2.buf, old_remote_context.buf);
>                 if (ptr) {
>                         refspec_updated = 1;
> @@ -683,8 +673,7 @@ static int mv(int argc, const char **argv)
>                 struct string_list_item *item = branch_list.items + i;
>                 struct branch_info *info = item->util;
>                 if (info->remote_name && !strcmp(info->remote_name, rename.old)) {
> -                       strbuf_reset(&buf);
> -                       strbuf_addf(&buf, "branch.%s.remote", item->string);
> +                       strbuf_setf(&buf, "branch.%s.remote", item->string);
>                         if (git_config_set(buf.buf, rename.new)) {
>                                 return error(_("Could not set '%s'"), buf.buf);
>                         }
> @@ -715,12 +704,10 @@ static int mv(int argc, const char **argv)
>
>                 if (item->util)
>                         continue;
> -               strbuf_reset(&buf);
> -               strbuf_addstr(&buf, item->string);
> +               strbuf_setstr(&buf, item->string);
>                 strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
>                                 rename.new, strlen(rename.new));
> -               strbuf_reset(&buf2);
> -               strbuf_addf(&buf2, "remote: renamed %s to %s",
> +               strbuf_setf(&buf2, "remote: renamed %s to %s",
>                                 item->string, buf.buf);
>                 if (rename_ref(item->string, buf.buf, buf2.buf))
>                         die(_("renaming '%s' failed"), item->string);
> @@ -730,16 +717,13 @@ static int mv(int argc, const char **argv)
>
>                 if (!item->util)
>                         continue;
> -               strbuf_reset(&buf);
> -               strbuf_addstr(&buf, item->string);
> +               strbuf_setstr(&buf, item->string);
>                 strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
>                                 rename.new, strlen(rename.new));
> -               strbuf_reset(&buf2);
> -               strbuf_addstr(&buf2, item->util);
> +               strbuf_setstr(&buf2, item->util);
>                 strbuf_splice(&buf2, strlen("refs/remotes/"), strlen(rename.old),
>                                 rename.new, strlen(rename.new));
> -               strbuf_reset(&buf3);
> -               strbuf_addf(&buf3, "remote: renamed %s to %s",
> +               strbuf_setf(&buf3, "remote: renamed %s to %s",
>                                 item->string, buf.buf);
>                 if (create_symref(buf.buf, buf2.buf, buf3.buf))
>                         die(_("creating '%s' failed"), buf.buf);
> @@ -804,8 +788,7 @@ static int rm(int argc, const char **argv)
>                 if (info->remote_name && !strcmp(info->remote_name, remote->name)) {
>                         const char *keys[] = { "remote", "merge", NULL }, **k;
>                         for (k = keys; *k; k++) {
> -                               strbuf_reset(&buf);
> -                               strbuf_addf(&buf, "branch.%s.%s",
> +                               strbuf_setf(&buf, "branch.%s.%s",
>                                                 item->string, *k);
>                                 if (git_config_set(buf.buf, NULL)) {
>                                         strbuf_release(&buf);
> --
> 2.0.0
>