{"thread":{"id":"45116","subject":"[PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again","startedAt":"2017-02-11T19:51:20Z","lastAt":"2017-02-13T21:03:18Z","messageCount":2,"participants":["René Scharfe","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311349","messageId":"f2549b5b-b3c8-3935-cdb9-9908914195b6@web.de","threadId":"45116","inReplyTo":null,"subject":"[PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-11T19:51:08Z","receivedAt":"2017-02-11T19:51:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Don't throw the memory allocated for remove_dir_recursively() away after\na single call, use it for the other entries as well instead.\n\nThis change was done before in deb8e15a (rm: reuse strbuf for all\nremove_dir_recursively() calls), but was reverted as a side-effect of\n55856a35 (rm: absorb a submodules git dir before deletion). Reinstate\nthe optimization.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nWas deb8e15a a rebase victim?\n\n builtin/rm.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 452170a3ab..fb79dcab18 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t */\n \tif (!index_only) {\n \t\tint removed = 0, gitmodules_modified = 0;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tfor (i = 0; i < list.nr; i++) {\n \t\t\tconst char *path = list.entry[i].name;\n \t\t\tif (list.entry[i].is_submodule) {\n-\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\n+\t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addstr(&buf, path);\n \t\t\t\tif (remove_dir_recursively(&buf, 0))\n \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n-\t\t\t\tstrbuf_release(&buf);\n \n \t\t\t\tremoved = 1;\n \t\t\t\tif (!remove_path_from_gitmodules(path))\n@@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tif (!removed)\n \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n \t\t}\n+\t\tstrbuf_release(&buf);\n \t\tif (gitmodules_modified)\n \t\t\tstage_updated_gitmodules();\n \t}\n-- \n2.11.1\n\n"},{"id":"311442","messageId":"CAGZ79kapfeEpX5Y5Y5ebBMjePvrXhOM80KQWU4DuLs9=ApRHTw@mail.gmail.com","threadId":"45116","inReplyTo":"f2549b5b-b3c8-3935-cdb9-9908914195b6@web.de","subject":"Re: [PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-13T21:03:12Z","receivedAt":"2017-02-13T21:03:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Feb 11, 2017 at 11:51 AM, René Scharfe <l.s.r@web.de> wrote:\n> Don't throw the memory allocated for remove_dir_recursively() away after\n> a single call, use it for the other entries as well instead.\n>\n> This change was done before in deb8e15a (rm: reuse strbuf for all\n> remove_dir_recursively() calls), but was reverted as a side-effect of\n> 55856a35 (rm: absorb a submodules git dir before deletion). Reinstate\n> the optimization.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> Was deb8e15a a rebase victim?\n\n(I do not recall it off the top of my head)\n\nThat commit was merged at 03f25e85,\nMerge branch 'rs/rm-strbuf-optim', but it looks\nlike it was reverted as part of 55856a35b2\n(rm: absorb a submodules git dir before deletion)\n\nLooking through the discussion at\nhttps://public-inbox.org/git/xmqqmvfich2e.fsf@gitster.mtv.corp.google.com/\nthere was no apparent signs of confusion, but a reroll was promised, that\nI cannot find on the list.\n\nAnyway, the patch below looks good to me.\n\nThanks,\nStefan\n\n>\n>  builtin/rm.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 452170a3ab..fb79dcab18 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>          */\n>         if (!index_only) {\n>                 int removed = 0, gitmodules_modified = 0;\n> +               struct strbuf buf = STRBUF_INIT;\n>                 for (i = 0; i < list.nr; i++) {\n>                         const char *path = list.entry[i].name;\n>                         if (list.entry[i].is_submodule) {\n> -                               struct strbuf buf = STRBUF_INIT;\n> -\n> +                               strbuf_reset(&buf);\n>                                 strbuf_addstr(&buf, path);\n>                                 if (remove_dir_recursively(&buf, 0))\n>                                         die(_(\"could not remove '%s'\"), path);\n> -                               strbuf_release(&buf);\n>\n>                                 removed = 1;\n>                                 if (!remove_path_from_gitmodules(path))\n> @@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>                         if (!removed)\n>                                 die_errno(\"git rm: '%s'\", path);\n>                 }\n> +               strbuf_release(&buf);\n>                 if (gitmodules_modified)\n>                         stage_updated_gitmodules();\n>         }\n> --\n> 2.11.1\n>\n"}]}