{"thread":{"id":"44176","subject":"[PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with \"%s\", part 2","startedAt":"2016-09-27T19:09:11Z","lastAt":"2016-10-07T20:45:49Z","messageCount":6,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"302742","messageId":"b7a61c78-3c46-375c-4cc3-7c363e551bc6@web.de","threadId":"44176","inReplyTo":null,"subject":"[PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with \"%s\", part 2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-27T19:08:21Z","receivedAt":"2016-09-27T19:09:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Replace uses of strbuf_addf() for adding strings with more lightweight\nstrbuf_addstr() calls.  This is shorter and makes the intent clearer.\n\nbc57b9c0cc5a123365a922fa1831177e3fd607ed already converted three cases,\nthis patch covers two more.\n\nA semantic patch for Coccinelle is included for easier checking for\nnew cases that might be introduced in the future.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/submodule--helper.c     | 2 +-\n contrib/coccinelle/strbuf.cocci | 6 ++++++\n submodule.c                     | 2 +-\n 3 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e3fdc0a..444ec06 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -753,7 +753,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\tif (suc->recursive_prefix)\n \t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->recursive_prefix, ce->name);\n \t\telse\n-\t\t\tstrbuf_addf(&sb, \"%s\", ce->name);\n+\t\t\tstrbuf_addstr(&sb, ce->name);\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\ndiff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci\nindex 7932d48..4b7553f 100644\n--- a/contrib/coccinelle/strbuf.cocci\n+++ b/contrib/coccinelle/strbuf.cocci\n@@ -3,3 +3,9 @@ expression E1, E2;\n @@\n - strbuf_addf(E1, E2);\n + strbuf_addstr(E1, E2);\n+\n+@@\n+expression E1, E2;\n+@@\n+- strbuf_addf(E1, \"%s\", E2);\n++ strbuf_addstr(E1, E2);\ndiff --git a/submodule.c b/submodule.c\nindex 0ef2ff4..dcc5ce3 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n-\tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n+\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n \tif (message)\n \t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\n \telse\n-- \n2.10.0\n\n"},{"id":"302743","messageId":"29e75b7b-6dd0-8c52-e444-cad1ba613cd0@web.de","threadId":"44176","inReplyTo":"b7a61c78-3c46-375c-4cc3-7c363e551bc6@web.de","subject":"[PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-27T19:11:58Z","receivedAt":"2016-09-27T19:12:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\ninstead of taking detours through find_unique_abbrev() and its static\nbuffer.  This is shorter and a bit more efficient.\n\n1eb47f167d65d1d305b9c196a1bb40eb96117cb1 already converted six cases,\nthis patch covers three more.\n\nA semantic patch for Coccinelle is included for easier checking for\nnew cases that might be introduced in the future.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n contrib/coccinelle/strbuf.cocci | 6 ++++++\n diff.c                          | 2 +-\n submodule.c                     | 2 +-\n wt-status.c                     | 3 +--\n 4 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci\nindex 4b7553f..1e24298 100644\n--- a/contrib/coccinelle/strbuf.cocci\n+++ b/contrib/coccinelle/strbuf.cocci\n@@ -9,3 +9,9 @@ expression E1, E2;\n @@\n - strbuf_addf(E1, \"%s\", E2);\n + strbuf_addstr(E1, E2);\n+\n+@@\n+expression E1, E2, E3;\n+@@\n+- strbuf_addstr(E1, find_unique_abbrev(E2, E3));\n++ strbuf_add_unique_abbrev(E1, E2, E3);\ndiff --git a/diff.c b/diff.c\nindex a178ed3..be11e4e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,\n \t\t}\n \t\tstrbuf_addf(msg, \"%s%sindex %s..\", line_prefix, set,\n \t\t\t    find_unique_abbrev(one->oid.hash, abbrev));\n-\t\tstrbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));\n+\t\tstrbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n \t\tif (one->mode == two->mode)\n \t\t\tstrbuf_addf(msg, \" %06o\", one->mode);\n \t\tstrbuf_addf(msg, \"%s\\n\", reset);\ndiff --git a/submodule.c b/submodule.c\nindex dcc5ce3..8cf40ea 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n-\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n+\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n \tif (message)\n \t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\n \telse\ndiff --git a/wt-status.c b/wt-status.c\nindex 9628c1d..99d1b0a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1383,8 +1383,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n \tif (!strcmp(cb->buf.buf, \"HEAD\")) {\n \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n \t\tstrbuf_reset(&cb->buf);\n-\t\tstrbuf_addstr(&cb->buf,\n-\t\t\t      find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n+\t\tstrbuf_add_unique_abbrev(&cb->buf, nsha1, DEFAULT_ABBREV);\n \t}\n \treturn 1;\n }\n-- \n2.10.0\n\n\n"},{"id":"302753","messageId":"xmqqshslm6jo.fsf@gitster.mtv.corp.google.com","threadId":"44176","inReplyTo":"29e75b7b-6dd0-8c52-e444-cad1ba613cd0@web.de","subject":"Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T20:28:11Z","receivedAt":"2016-09-27T20:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> diff --git a/diff.c b/diff.c\n> index a178ed3..be11e4e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,\n>  \t\t}\n>  \t\tstrbuf_addf(msg, \"%s%sindex %s..\", line_prefix, set,\n>  \t\t\t    find_unique_abbrev(one->oid.hash, abbrev));\n> -\t\tstrbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));\n> +\t\tstrbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n\nOK.\n\n> diff --git a/submodule.c b/submodule.c\n> index dcc5ce3..8cf40ea 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n>  \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n>  \tif (!fast_backward && !fast_forward)\n>  \t\tstrbuf_addch(&sb, '.');\n> -\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n> +\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n\nI wonder how could this change come out of this definition:\n\n    @@\n    expression E1, E2, E3;\n    @@\n    - strbuf_addstr(E1, find_unique_abbrev(E2, E3));\n    + strbuf_add_unique_abbrev(E1, E2, E3);\n\n"},{"id":"302767","messageId":"3c80f7d7-a145-3a4a-4bf2-2d8fb6dda04b@web.de","threadId":"44176","inReplyTo":"xmqqshslm6jo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-27T20:59:03Z","receivedAt":"2016-09-27T20:59:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.09.2016 um 22:28 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>> diff --git a/submodule.c b/submodule.c\n>> index dcc5ce3..8cf40ea 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n>>  \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n>>  \tif (!fast_backward && !fast_forward)\n>>  \t\tstrbuf_addch(&sb, '.');\n>> -\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n>> +\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n> \n> I wonder how could this change come out of this definition:\n> \n>     @@\n>     expression E1, E2, E3;\n>     @@\n>     - strbuf_addstr(E1, find_unique_abbrev(E2, E3));\n>     + strbuf_add_unique_abbrev(E1, E2, E3);\n\nImpossible.  I added \"->hash\" manually during a rebase (merging\na0d12c44, wrongly).  Good catch, thanks!\n\nSeeing proof of skipping compile-testing I wonder what else I do\nforget in my daily life. :-|  I'll better go to sleep now..\n\nFixup patch, generated by reverting the diff, re-adding the\nsemantic patch and using coccicheck; compiles and survives make\ntest:\n---\n submodule.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 8cf40ea..bb06b60 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n-\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n+\tstrbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV);\n \tif (message)\n \t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\n \telse\n-- \n2.10.0\n\n"},{"id":"303546","messageId":"20161007004635.xbuaxvc6eyfkyyma@sigill.intra.peff.net","threadId":"44176","inReplyTo":"29e75b7b-6dd0-8c52-e444-cad1ba613cd0@web.de","subject":"Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-07T00:46:35Z","receivedAt":"2016-10-07T00:53:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:\n\n> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\n> instead of taking detours through find_unique_abbrev() and its static\n> buffer.  This is shorter and a bit more efficient.\n> [...]\n> diff --git a/diff.c b/diff.c\n> index a178ed3..be11e4e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,\n>  \t\t}\n>  \t\tstrbuf_addf(msg, \"%s%sindex %s..\", line_prefix, set,\n>  \t\t\t    find_unique_abbrev(one->oid.hash, abbrev));\n> -\t\tstrbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));\n> +\t\tstrbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n>  \t\tif (one->mode == two->mode)\n>  \t\t\tstrbuf_addf(msg, \" %06o\", one->mode);\n>  \t\tstrbuf_addf(msg, \"%s\\n\", reset);\n\nThis one is an interesting case, and maybe a good example of why blind\ncoccinelle usage can have some pitfalls. :)\n\nWe get rid of the strbuf_addstr(), but notice that we leave untouched\nthe find_unique_abbrev() call immediately above. There was a symmetry to\nthe two that has been lost.\n\nProbably either:\n\n  strbuf_addf(msg, \"%s%sindex %s..%s\", line_prefix, set\n\tfind_unique_abbrev(one->oid.hash, abbrev),\n\tfind_unique_abbrev(two->oid.hash, abbrev));\n\nor:\n\n  strbuf_addf(msg, \"%s%sindex \", line_prefix, set);\n  strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev);\n  strbuf_addstr(msg, \"..\");\n  strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n\nwould be a more appropriate refactoring. The problem is in the original\npatch (which also lacks symmetry; either this predates the multi-buffer\nfind_unique_abbrev, or the original author didn't know about it), but I\nthink your refactor makes it slightly worse.\n\nI noticed because I have another series which touches these lines, and\nit wants to symmetrically swap out find_unique_abbrev for something\nelse. :) I don't think it's a big enough deal to switch now (and I've\nalready rebased my series which will touch these lines), but I wanted to\nmention it as a thing to watch out for as we do more of these kinds of\nautomated transformations.\n\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n>  \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n>  \tif (!fast_backward && !fast_forward)\n>  \t\tstrbuf_addch(&sb, '.');\n> -\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n> +\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n\nThis one is a similar situation, I think.\n\n-Peff\n"},{"id":"303610","messageId":"baff7c23-1c0b-3df5-bb4e-661bf0383fe4@web.de","threadId":"44176","inReplyTo":"20161007004635.xbuaxvc6eyfkyyma@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-07T20:45:12Z","receivedAt":"2016-10-07T20:45:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.10.2016 um 02:46 schrieb Jeff King:\n> On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:\n>\n>> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs\n>> instead of taking detours through find_unique_abbrev() and its static\n>> buffer.  This is shorter and a bit more efficient.\n>> [...]\n>> diff --git a/diff.c b/diff.c\n>> index a178ed3..be11e4e 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,\n>>  \t\t}\n>>  \t\tstrbuf_addf(msg, \"%s%sindex %s..\", line_prefix, set,\n>>  \t\t\t    find_unique_abbrev(one->oid.hash, abbrev));\n>> -\t\tstrbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));\n>> +\t\tstrbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n>>  \t\tif (one->mode == two->mode)\n>>  \t\t\tstrbuf_addf(msg, \" %06o\", one->mode);\n>>  \t\tstrbuf_addf(msg, \"%s\\n\", reset);\n>\n> This one is an interesting case, and maybe a good example of why blind\n> coccinelle usage can have some pitfalls. :)\n\nThank you for paying attention. :)  In general I agree that the \nsurrounding code of such changes should be checked; the issue at hand \ncould be part of a bigger problem.\n\n> We get rid of the strbuf_addstr(), but notice that we leave untouched\n> the find_unique_abbrev() call immediately above. There was a symmetry to\n> the two that has been lost.\n>\n> Probably either:\n>\n>   strbuf_addf(msg, \"%s%sindex %s..%s\", line_prefix, set\n> \tfind_unique_abbrev(one->oid.hash, abbrev),\n> \tfind_unique_abbrev(two->oid.hash, abbrev));\n>\n> or:\n>\n>   strbuf_addf(msg, \"%s%sindex \", line_prefix, set);\n>   strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev);\n>   strbuf_addstr(msg, \"..\");\n>   strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);\n>\n> would be a more appropriate refactoring. The problem is in the original\n> patch (which also lacks symmetry; either this predates the multi-buffer\n> find_unique_abbrev, or the original author didn't know about it), but I\n> think your refactor makes it slightly worse.\n\nI still think the automatically generated patch is a net win, but we \nshouldn't stop there.\n\n> I noticed because I have another series which touches these lines, and\n> it wants to symmetrically swap out find_unique_abbrev for something\n> else. :) I don't think it's a big enough deal to switch now (and I've\n> already rebased my series which will touch these lines), but I wanted to\n> mention it as a thing to watch out for as we do more of these kinds of\n> automated transformations.\n\nOK, then I'll wait for that series to land.\n\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,\n>>  \t\t\tfind_unique_abbrev(one->hash, DEFAULT_ABBREV));\n>>  \tif (!fast_backward && !fast_forward)\n>>  \t\tstrbuf_addch(&sb, '.');\n>> -\tstrbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));\n>> +\tstrbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);\n>\n> This one is a similar situation, I think.\n\nYes, and there are some more.  Will take a look.\n\nI don't know how to crack printf-style formats using semantic patches. \nIt's easy for fixed formats (silly example):\n\n\t- strbuf_addf(sb, \"%s%s\", a, b);\n\t+ strbuf_addf(sb, \"%s\", a);\n\t+ strbuf_addf(sb, \"%s\", b);\n\nBut how to do that for arbitrary formats?  Probably not worth it..\n\nRené\n"}]}