* [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2 @ 2016-09-27 19:08 René Scharfe 2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe 0 siblings, 1 reply; 6+ messages in thread From: René Scharfe @ 2016-09-27 19:08 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano Replace uses of strbuf_addf() for adding strings with more lightweight strbuf_addstr() calls. This is shorter and makes the intent clearer. bc57b9c0cc5a123365a922fa1831177e3fd607ed already converted three cases, this patch covers two more. A semantic patch for Coccinelle is included for easier checking for new cases that might be introduced in the future. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- builtin/submodule--helper.c | 2 +- contrib/coccinelle/strbuf.cocci | 6 ++++++ submodule.c | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index e3fdc0a..444ec06 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -753,7 +753,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, if (suc->recursive_prefix) strbuf_addf(&sb, "%s/%s", suc->recursive_prefix, ce->name); else - strbuf_addf(&sb, "%s", ce->name); + strbuf_addstr(&sb, ce->name); strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf); strbuf_addch(out, '\n'); goto cleanup; diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci index 7932d48..4b7553f 100644 --- a/contrib/coccinelle/strbuf.cocci +++ b/contrib/coccinelle/strbuf.cocci @@ -3,3 +3,9 @@ expression E1, E2; @@ - strbuf_addf(E1, E2); + strbuf_addstr(E1, E2); + +@@ +expression E1, E2; +@@ +- strbuf_addf(E1, "%s", E2); ++ strbuf_addstr(E1, E2); diff --git a/submodule.c b/submodule.c index 0ef2ff4..dcc5ce3 100644 --- a/submodule.c +++ b/submodule.c @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, find_unique_abbrev(one->hash, DEFAULT_ABBREV)); if (!fast_backward && !fast_forward) strbuf_addch(&sb, '.'); - strbuf_addf(&sb, "%s", find_unique_abbrev(two->hash, DEFAULT_ABBREV)); + strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); if (message) strbuf_addf(&sb, " %s%s\n", message, reset); else -- 2.10.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2 2016-09-27 19:08 [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2 René Scharfe @ 2016-09-27 19:11 ` René Scharfe 2016-09-27 20:28 ` Junio C Hamano 2016-10-07 0:46 ` Jeff King 0 siblings, 2 replies; 6+ messages in thread From: René Scharfe @ 2016-09-27 19:11 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs instead of taking detours through find_unique_abbrev() and its static buffer. This is shorter and a bit more efficient. 1eb47f167d65d1d305b9c196a1bb40eb96117cb1 already converted six cases, this patch covers three more. A semantic patch for Coccinelle is included for easier checking for new cases that might be introduced in the future. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- contrib/coccinelle/strbuf.cocci | 6 ++++++ diff.c | 2 +- submodule.c | 2 +- wt-status.c | 3 +-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci index 4b7553f..1e24298 100644 --- a/contrib/coccinelle/strbuf.cocci +++ b/contrib/coccinelle/strbuf.cocci @@ -9,3 +9,9 @@ expression E1, E2; @@ - strbuf_addf(E1, "%s", E2); + strbuf_addstr(E1, E2); + +@@ +expression E1, E2, E3; +@@ +- strbuf_addstr(E1, find_unique_abbrev(E2, E3)); ++ strbuf_add_unique_abbrev(E1, E2, E3); diff --git a/diff.c b/diff.c index a178ed3..be11e4e 100644 --- a/diff.c +++ b/diff.c @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg, } strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, find_unique_abbrev(one->oid.hash, abbrev)); - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev)); + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); if (one->mode == two->mode) strbuf_addf(msg, " %06o", one->mode); strbuf_addf(msg, "%s\n", reset); diff --git a/submodule.c b/submodule.c index dcc5ce3..8cf40ea 100644 --- a/submodule.c +++ b/submodule.c @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, find_unique_abbrev(one->hash, DEFAULT_ABBREV)); if (!fast_backward && !fast_forward) strbuf_addch(&sb, '.'); - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); if (message) strbuf_addf(&sb, " %s%s\n", message, reset); else diff --git a/wt-status.c b/wt-status.c index 9628c1d..99d1b0a 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1383,8 +1383,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1, if (!strcmp(cb->buf.buf, "HEAD")) { /* HEAD is relative. Resolve it to the right reflog entry. */ strbuf_reset(&cb->buf); - strbuf_addstr(&cb->buf, - find_unique_abbrev(nsha1, DEFAULT_ABBREV)); + strbuf_add_unique_abbrev(&cb->buf, nsha1, DEFAULT_ABBREV); } return 1; } -- 2.10.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2 2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe @ 2016-09-27 20:28 ` Junio C Hamano 2016-09-27 20:59 ` René Scharfe 2016-10-07 0:46 ` Jeff King 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2016-09-27 20:28 UTC (permalink / raw) To: René Scharfe; +Cc: Git List René Scharfe <l.s.r@web.de> writes: > diff --git a/diff.c b/diff.c > index a178ed3..be11e4e 100644 > --- a/diff.c > +++ b/diff.c > @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg, > } > strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, > find_unique_abbrev(one->oid.hash, abbrev)); > - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev)); > + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); OK. > diff --git a/submodule.c b/submodule.c > index dcc5ce3..8cf40ea 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, > find_unique_abbrev(one->hash, DEFAULT_ABBREV)); > if (!fast_backward && !fast_forward) > strbuf_addch(&sb, '.'); > - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); > + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); I wonder how could this change come out of this definition: @@ expression E1, E2, E3; @@ - strbuf_addstr(E1, find_unique_abbrev(E2, E3)); + strbuf_add_unique_abbrev(E1, E2, E3); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2 2016-09-27 20:28 ` Junio C Hamano @ 2016-09-27 20:59 ` René Scharfe 0 siblings, 0 replies; 6+ messages in thread From: René Scharfe @ 2016-09-27 20:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List Am 27.09.2016 um 22:28 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: >> diff --git a/submodule.c b/submodule.c >> index dcc5ce3..8cf40ea 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, >> find_unique_abbrev(one->hash, DEFAULT_ABBREV)); >> if (!fast_backward && !fast_forward) >> strbuf_addch(&sb, '.'); >> - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); >> + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); > > I wonder how could this change come out of this definition: > > @@ > expression E1, E2, E3; > @@ > - strbuf_addstr(E1, find_unique_abbrev(E2, E3)); > + strbuf_add_unique_abbrev(E1, E2, E3); Impossible. I added "->hash" manually during a rebase (merging a0d12c44, wrongly). Good catch, thanks! Seeing proof of skipping compile-testing I wonder what else I do forget in my daily life. :-| I'll better go to sleep now.. Fixup patch, generated by reverting the diff, re-adding the semantic patch and using coccicheck; compiles and survives make test: --- submodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 8cf40ea..bb06b60 100644 --- a/submodule.c +++ b/submodule.c @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, find_unique_abbrev(one->hash, DEFAULT_ABBREV)); if (!fast_backward && !fast_forward) strbuf_addch(&sb, '.'); - strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); + strbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV); if (message) strbuf_addf(&sb, " %s%s\n", message, reset); else -- 2.10.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2 2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe 2016-09-27 20:28 ` Junio C Hamano @ 2016-10-07 0:46 ` Jeff King 2016-10-07 20:45 ` René Scharfe 1 sibling, 1 reply; 6+ messages in thread From: Jeff King @ 2016-10-07 0:46 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Junio C Hamano On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote: > Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs > instead of taking detours through find_unique_abbrev() and its static > buffer. This is shorter and a bit more efficient. > [...] > diff --git a/diff.c b/diff.c > index a178ed3..be11e4e 100644 > --- a/diff.c > +++ b/diff.c > @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg, > } > strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, > find_unique_abbrev(one->oid.hash, abbrev)); > - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev)); > + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); > if (one->mode == two->mode) > strbuf_addf(msg, " %06o", one->mode); > strbuf_addf(msg, "%s\n", reset); This one is an interesting case, and maybe a good example of why blind coccinelle usage can have some pitfalls. :) We get rid of the strbuf_addstr(), but notice that we leave untouched the find_unique_abbrev() call immediately above. There was a symmetry to the two that has been lost. Probably either: strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set find_unique_abbrev(one->oid.hash, abbrev), find_unique_abbrev(two->oid.hash, abbrev)); or: strbuf_addf(msg, "%s%sindex ", line_prefix, set); strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev); strbuf_addstr(msg, ".."); strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); would be a more appropriate refactoring. The problem is in the original patch (which also lacks symmetry; either this predates the multi-buffer find_unique_abbrev, or the original author didn't know about it), but I think your refactor makes it slightly worse. I noticed because I have another series which touches these lines, and it wants to symmetrically swap out find_unique_abbrev for something else. :) I don't think it's a big enough deal to switch now (and I've already rebased my series which will touch these lines), but I wanted to mention it as a thing to watch out for as we do more of these kinds of automated transformations. > --- a/submodule.c > +++ b/submodule.c > @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, > find_unique_abbrev(one->hash, DEFAULT_ABBREV)); > if (!fast_backward && !fast_forward) > strbuf_addch(&sb, '.'); > - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); > + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); This one is a similar situation, I think. -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2 2016-10-07 0:46 ` Jeff King @ 2016-10-07 20:45 ` René Scharfe 0 siblings, 0 replies; 6+ messages in thread From: René Scharfe @ 2016-10-07 20:45 UTC (permalink / raw) To: Jeff King; +Cc: Git List, Junio C Hamano Am 07.10.2016 um 02:46 schrieb Jeff King: > On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote: > >> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs >> instead of taking detours through find_unique_abbrev() and its static >> buffer. This is shorter and a bit more efficient. >> [...] >> diff --git a/diff.c b/diff.c >> index a178ed3..be11e4e 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg, >> } >> strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, >> find_unique_abbrev(one->oid.hash, abbrev)); >> - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev)); >> + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); >> if (one->mode == two->mode) >> strbuf_addf(msg, " %06o", one->mode); >> strbuf_addf(msg, "%s\n", reset); > > This one is an interesting case, and maybe a good example of why blind > coccinelle usage can have some pitfalls. :) Thank you for paying attention. :) In general I agree that the surrounding code of such changes should be checked; the issue at hand could be part of a bigger problem. > We get rid of the strbuf_addstr(), but notice that we leave untouched > the find_unique_abbrev() call immediately above. There was a symmetry to > the two that has been lost. > > Probably either: > > strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set > find_unique_abbrev(one->oid.hash, abbrev), > find_unique_abbrev(two->oid.hash, abbrev)); > > or: > > strbuf_addf(msg, "%s%sindex ", line_prefix, set); > strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev); > strbuf_addstr(msg, ".."); > strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); > > would be a more appropriate refactoring. The problem is in the original > patch (which also lacks symmetry; either this predates the multi-buffer > find_unique_abbrev, or the original author didn't know about it), but I > think your refactor makes it slightly worse. I still think the automatically generated patch is a net win, but we shouldn't stop there. > I noticed because I have another series which touches these lines, and > it wants to symmetrically swap out find_unique_abbrev for something > else. :) I don't think it's a big enough deal to switch now (and I've > already rebased my series which will touch these lines), but I wanted to > mention it as a thing to watch out for as we do more of these kinds of > automated transformations. OK, then I'll wait for that series to land. >> --- a/submodule.c >> +++ b/submodule.c >> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, >> find_unique_abbrev(one->hash, DEFAULT_ABBREV)); >> if (!fast_backward && !fast_forward) >> strbuf_addch(&sb, '.'); >> - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); >> + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); > > This one is a similar situation, I think. Yes, and there are some more. Will take a look. I don't know how to crack printf-style formats using semantic patches. It's easy for fixed formats (silly example): - strbuf_addf(sb, "%s%s", a, b); + strbuf_addf(sb, "%s", a); + strbuf_addf(sb, "%s", b); But how to do that for arbitrary formats? Probably not worth it.. René ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-07 20:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-27 19:08 [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2 René Scharfe 2016-09-27 19:11 ` [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, " René Scharfe 2016-09-27 20:28 ` Junio C Hamano 2016-09-27 20:59 ` René Scharfe 2016-10-07 0:46 ` Jeff King 2016-10-07 20:45 ` René Scharfe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).