{"thread":{"id":"52409","subject":"[Outreachy] [PATCH] bisect--helper: refer branch.buf before strbuf_release(...)","startedAt":"2019-12-08T17:29:53Z","lastAt":"2019-12-09T08:16:05Z","messageCount":3,"participants":["Miriam Rubio","Johannes Schindelin","Miriam R."],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387718","messageId":"20191208172813.16518-1-mirucam@gmail.com","threadId":"52409","inReplyTo":null,"subject":"[Outreachy] [PATCH] bisect--helper: refer branch.buf before strbuf_release(...)","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2019-12-08T17:28:13Z","receivedAt":"2019-12-08T17:29:53Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane \n\nMove `error(\"...%s...\", branch.buf);` before `strbuf_release(&branch);`\nto release string buffer `branch` and the memory it used.\n\nMentored-by: Johannes Schindelin \nMentored-by: Christian Couder \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1fbe156e67..3055b2bb50 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)\n \n \t\targv_array_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n \t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n+\t\t\terror(_(\"could not check out original\"\n+\t\t\t\t\" HEAD '%s'. Try 'git bisect\"\n+\t\t\t\t\" reset '.\"), branch.buf);\n \t\t\tstrbuf_release(&branch);\n \t\t\targv_array_clear(&argv);\n-\t\t\treturn error(_(\"could not check out original\"\n-\t\t\t\t \" HEAD '%s'. Try 'git bisect\"\n-\t\t\t\t \" reset '.\"), branch.buf);\n+\t\t\treturn -1;\n \t\t}\n \t\targv_array_clear(&argv);\n \t}\n-- \n2.21.0 (Apple Git-122.2)\n\n"},{"id":"387721","messageId":"nycvar.QRO.7.76.6.1912082003420.31080@tvgsbejvaqbjf.bet","threadId":"52409","inReplyTo":"20191208172813.16518-1-mirucam@gmail.com","subject":"Re: [Outreachy] [PATCH] bisect--helper: refer branch.buf before strbuf_release(...)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-08T19:11:47Z","receivedAt":"2019-12-08T19:12:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nwelcome to the Git project!\n\nOn Sun, 8 Dec 2019, Miriam Rubio wrote:\n\n> From: Tanushree Tumane \n\nIs this really a patch authored by Tanushree, or is this a fix written by\nyou, intended to fix a patch authored by Tanushree?\n\nIf it is the latter, please use a completely new commit message and take\nthe authorship yourself.\n\n> Move `error(\"...%s...\", branch.buf);` before `strbuf_release(&branch);`\n> to release string buffer `branch` and the memory it used.\n\nThis describes the \"what?\", but the patch already does that. The commit\nmessage should be more about the \"why?\".\n\nIn this instance, I believe that the commit message should read more like\nthis:\n\n-- snip --\nbisect--helper: avoid free-after-use\n\nIn 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,\n2019-01-02), the `git bisect reset` subcommand was ported to C. When the\ncall to `git checkout` failed, an error message was reported to the\nuser.\n\nHowever, this error message used the `strbuf` that had just been\nreleased already. Let's switch that around: first use it, then release\nit.\n\nSigned-off-by: Miriam Rubio \n-- snap --\n\n>\n> Mentored-by: Johannes Schindelin \n> Mentored-by: Christian Couder \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 1fbe156e67..3055b2bb50 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)\n>\n> \t\targv_array_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n> \t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n> +\t\t\terror(_(\"could not check out original\"\n> +\t\t\t\t\" HEAD '%s'. Try 'git bisect\"\n> +\t\t\t\t\" reset '.\"), branch.buf);\n> \t\t\tstrbuf_release(&branch);\n> \t\t\targv_array_clear(&argv);\n> -\t\t\treturn error(_(\"could not check out original\"\n> -\t\t\t\t \" HEAD '%s'. Try 'git bisect\"\n> -\t\t\t\t \" reset '.\"), branch.buf);\n> +\t\t\treturn -1;\n\nThe patch looks good.\n\nThanks!\nJohannes\n\n> \t\t}\n> \t\targv_array_clear(&argv);\n> \t}\n> --\n> 2.21.0 (Apple Git-122.2)\n>\n>\n"},{"id":"387736","messageId":"CAN7CjDC7V+4ZR=1vy=BitVNUakKpuZVpvdf_BsDL2X6fjAjc7g@mail.gmail.com","threadId":"52409","inReplyTo":"nycvar.QRO.7.76.6.1912082003420.31080@tvgsbejvaqbjf.bet","subject":"Re: [Outreachy] [PATCH] bisect--helper: refer branch.buf before strbuf_release(...)","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2019-12-09T08:15:52Z","receivedAt":"2019-12-09T08:16:05Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"El dom., 8 dic. 2019 a las 20:12, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> welcome to the Git project!\n>\nThank you! :)\n\n> On Sun, 8 Dec 2019, Miriam Rubio wrote:\n>\n> > From: Tanushree Tumane \n>\n> Is this really a patch authored by Tanushree, or is this a fix written by\n> you, intended to fix a patch authored by Tanushree?\n>\n\nThis is really a patch authored by Tanushree.\n\n> If it is the latter, please use a completely new commit message and take\n> the authorship yourself.\n>\n> > Move `error(\"...%s...\", branch.buf);` before `strbuf_release(&branch);`\n> > to release string buffer `branch` and the memory it used.\n>\n> This describes the \"what?\", but the patch already does that. The commit\n> message should be more about the \"why?\".\n>\n> In this instance, I believe that the commit message should read more like\n> this:\n>\n> -- snip --\n> bisect--helper: avoid free-after-use\n>\n> In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,\n> 2019-01-02), the `git bisect reset` subcommand was ported to C. When the\n> call to `git checkout` failed, an error message was reported to the\n> user.\n>\n> However, this error message used the `strbuf` that had just been\n> released already. Let's switch that around: first use it, then release\n> it.\n>\n> Signed-off-by: Miriam Rubio \n> -- snap --\n>\n\nOk! Thank you for your advice with the message.\n\n> >\n> > Mentored-by: Johannes Schindelin \n> > Mentored-by: Christian Couder \n> > Signed-off-by: Tanushree Tumane \n> > Signed-off-by: Miriam Rubio \n> > ---\n> > builtin/bisect--helper.c | 7 ++++---\n> > 1 file changed, 4 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 1fbe156e67..3055b2bb50 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n> > @@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)\n> >\n> > argv_array_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n> > if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n> > + error(_(\"could not check out original\"\n> > + \" HEAD '%s'. Try 'git bisect\"\n> > + \" reset '.\"), branch.buf);\n> > strbuf_release(&branch);\n> > argv_array_clear(&argv);\n> > - return error(_(\"could not check out original\"\n> > - \" HEAD '%s'. Try 'git bisect\"\n> > - \" reset '.\"), branch.buf);\n> > + return -1;\n>\n> The patch looks good.\n>\nGreat :). I'll change the commit message and I'll resend the patch.\n\n> Thanks!\n> Johannes\nThanks,\nMiriam\n>\n> > }\n> > argv_array_clear(&argv);\n> > }\n> > --\n> > 2.21.0 (Apple Git-122.2)\n> >\n> >\n"}]}