* [PATCH v3 0/2] add strbuf_set operations @ 2014-06-12 7:29 Jeremiah Mahler 2014-06-12 7:29 ` [PATCH v3 1/2] " Jeremiah Mahler 2014-06-12 7:29 ` [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() Jeremiah Mahler 0 siblings, 2 replies; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 7:29 UTC (permalink / raw) To: Eric Sunshine; +Cc: Michael Haggerty, git, Jeremiah Mahler Addition of strbuf_set operations, version 3. Includes suggestions from Eric Sunshine [1]: - Revise log message to better argue why this patch is worthwhile. - Avoid documentation redundancy: "Setting buffer", "Replace buffer". - Remove unnecessary changes which didn't have a significant benefit to avoid unnecessary "code churn". builtin/remote.c was the one file which showed a significant benefit. Others with negligible benefits have been left as is. The possible performance improvements using a strbuf_grow_to() operation as suggested by Michael Haggerty [2] has been left for a later patch. [1]: http://marc.info/?l=git&m=140247618416057&w=2 [2]: http://marc.info/?l=git&m=140248834420244&w=2 Jeremiah Mahler (2): add strbuf_set operations builtin/remote: improve readability via strbuf_set() Documentation/technical/api-strbuf.txt | 18 ++++++++++ builtin/remote.c | 63 +++++++++++++--------------------- strbuf.c | 21 ++++++++++++ strbuf.h | 13 +++++++ 4 files changed, 75 insertions(+), 40 deletions(-) -- 2.0.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] add strbuf_set operations 2014-06-12 7:29 [PATCH v3 0/2] add strbuf_set operations Jeremiah Mahler @ 2014-06-12 7:29 ` Jeremiah Mahler 2014-06-12 8:11 ` Thomas Braun 2014-06-12 18:50 ` Junio C Hamano 2014-06-12 7:29 ` [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() Jeremiah Mahler 1 sibling, 2 replies; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 7:29 UTC (permalink / raw) To: Eric Sunshine; +Cc: Michael Haggerty, git, 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); 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) { -- 2.0.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 7:29 ` [PATCH v3 1/2] " Jeremiah Mahler @ 2014-06-12 8:11 ` Thomas Braun 2014-06-12 8:22 ` Jeremiah Mahler 2014-06-12 18:50 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Thomas Braun @ 2014-06-12 8:11 UTC (permalink / raw) To: Jeremiah Mahler, Eric Sunshine; +Cc: Michael Haggerty, git 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 8:11 ` Thomas Braun @ 2014-06-12 8:22 ` Jeremiah Mahler 2014-06-12 18:51 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 8:22 UTC (permalink / raw) To: Thomas Braun; +Cc: git 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 http://github.com/jmahler ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 8:22 ` Jeremiah Mahler @ 2014-06-12 18:51 ` Junio C Hamano 2014-06-12 19:36 ` Jeremiah Mahler 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2014-06-12 18:51 UTC (permalink / raw) To: Jeremiah Mahler; +Cc: Thomas Braun, git 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??? >> > 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 ;-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 18:51 ` Junio C Hamano @ 2014-06-12 19:36 ` Jeremiah Mahler 2014-06-12 21:18 ` Eric Sunshine 0 siblings, 1 reply; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 19:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio, 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? -- Jeremiah Mahler jmmahler@gmail.com http://github.com/jmahler ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 19:36 ` Jeremiah Mahler @ 2014-06-12 21:18 ` Eric Sunshine 2014-06-12 23:14 ` Jeremiah Mahler 0 siblings, 1 reply; 16+ messages in thread From: Eric Sunshine @ 2014-06-12 21:18 UTC (permalink / raw) To: Jeremiah Mahler, Junio C Hamano, Git List 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 21:18 ` Eric Sunshine @ 2014-06-12 23:14 ` Jeremiah Mahler 0 siblings, 0 replies; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 23:14 UTC (permalink / raw) To: Eric Sunshine; +Cc: git On Thu, Jun 12, 2014 at 05:18:29PM -0400, Eric Sunshine wrote: > 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. OK, got it. -- Jeremiah Mahler jmmahler@gmail.com http://github.com/jmahler ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 7:29 ` [PATCH v3 1/2] " Jeremiah Mahler 2014-06-12 8:11 ` Thomas Braun @ 2014-06-12 18:50 ` Junio C Hamano 2014-06-12 19:31 ` Jeremiah Mahler 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2014-06-12 18:50 UTC (permalink / raw) To: Jeremiah Mahler; +Cc: Eric Sunshine, Michael Haggerty, git 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. > > +* 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) > { ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 18:50 ` Junio C Hamano @ 2014-06-12 19:31 ` Jeremiah Mahler 2014-06-12 21:48 ` Eric Sunshine 0 siblings, 1 reply; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 19:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio, Comments below... On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote: > 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); > > ;-) _add would work in your example. strbuf_set would also work and it would perform the same number of operations (one set and one add). However your example is different from mine in which cond1 and cond2 are not necessarily mutually exclusive. > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 19:31 ` Jeremiah Mahler @ 2014-06-12 21:48 ` Eric Sunshine 2014-06-12 23:46 ` Jeremiah Mahler 0 siblings, 1 reply; 16+ messages in thread From: Eric Sunshine @ 2014-06-12 21:48 UTC (permalink / raw) To: Jeremiah Mahler, Junio C Hamano, Git List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 21:48 ` Eric Sunshine @ 2014-06-12 23:46 ` Jeremiah Mahler 2014-06-13 7:15 ` Jeff King 0 siblings, 1 reply; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 23:46 UTC (permalink / raw) To: Eric Sunshine; +Cc: git Eric, On Thu, Jun 12, 2014 at 05:48:52PM -0400, Eric Sunshine wrote: > 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 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 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 -- Jeremiah Mahler jmmahler@gmail.com http://github.com/jmahler ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-12 23:46 ` Jeremiah Mahler @ 2014-06-13 7:15 ` Jeff King 2014-06-14 4:49 ` Jeremiah Mahler 0 siblings, 1 reply; 16+ messages in thread From: Jeff King @ 2014-06-13 7:15 UTC (permalink / raw) To: Jeremiah Mahler, Eric Sunshine, git 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] add strbuf_set operations 2014-06-13 7:15 ` Jeff King @ 2014-06-14 4:49 ` Jeremiah Mahler 0 siblings, 0 replies; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-14 4:49 UTC (permalink / raw) To: Jeff King; +Cc: git Peff, On Fri, Jun 13, 2014 at 03:15:50AM -0400, Jeff King wrote: > 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 Improving remote.c sounds like a better direction than adding set operations. I will start looking in to it. -- Jeremiah Mahler jmmahler@gmail.com http://github.com/jmahler ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() 2014-06-12 7:29 [PATCH v3 0/2] add strbuf_set operations Jeremiah Mahler 2014-06-12 7:29 ` [PATCH v3 1/2] " Jeremiah Mahler @ 2014-06-12 7:29 ` Jeremiah Mahler 2014-06-12 8:19 ` Eric Sunshine 1 sibling, 1 reply; 16+ messages in thread From: Jeremiah Mahler @ 2014-06-12 7:29 UTC (permalink / raw) To: Eric Sunshine; +Cc: Michael Haggerty, git, Jeremiah Mahler 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() 2014-06-12 7:29 ` [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() Jeremiah Mahler @ 2014-06-12 8:19 ` Eric Sunshine 0 siblings, 0 replies; 16+ messages in thread From: Eric Sunshine @ 2014-06-12 8:19 UTC (permalink / raw) To: Jeremiah Mahler; +Cc: Michael Haggerty, Git List On Thu, Jun 12, 2014 at 3:29 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote: > 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); This hunk nicely illustrates an important maintenance benefit of strbuf_set() not mentioned as justification in patch 1/2. The original programmer knew that 'buf' and 'buf2' had not yet been used, even though their declarations are quite distant from this bit of code, so he omitted the call to strbuf_reset(). Although the omission is valid, it also is potentially dangerous and a maintenance burden. The person who next inserts code which uses 'buf' or 'buf2' between here and the declarations must remember or know to add strbuf_reset()s here, but that's easily overlooked. strbuf_set() eliminates that burden. This is the strongest argument I've thus far seen in favor of strbuf_set() (though I'm not, in any way, suggesting you reroll the series merely to mention this). > 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-06-14 4:53 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-12 7:29 [PATCH v3 0/2] add strbuf_set operations Jeremiah Mahler 2014-06-12 7:29 ` [PATCH v3 1/2] " Jeremiah Mahler 2014-06-12 8:11 ` Thomas Braun 2014-06-12 8:22 ` Jeremiah Mahler 2014-06-12 18:51 ` Junio C Hamano 2014-06-12 19:36 ` Jeremiah Mahler 2014-06-12 21:18 ` Eric Sunshine 2014-06-12 23:14 ` Jeremiah Mahler 2014-06-12 18:50 ` Junio C Hamano 2014-06-12 19:31 ` Jeremiah Mahler 2014-06-12 21:48 ` Eric Sunshine 2014-06-12 23:46 ` Jeremiah Mahler 2014-06-13 7:15 ` Jeff King 2014-06-14 4:49 ` Jeremiah Mahler 2014-06-12 7:29 ` [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set() Jeremiah Mahler 2014-06-12 8:19 ` Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.