Re: [Outreachy] [PATCH] bisect--helper: refer branch.buf before strbuf_release(...)

From: Miriam R. <mirucam@gmail.com>
Date: Dec 9, 2019, 08:15 UTC
Message-ID: <CAN7CjDC7V+4ZR=1vy=BitVNUakKpuZVpvdf_BsDL2X6fjAjc7g@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1912082003420.31080@tvgsbejvaqbjf.bet>

El dom., 8 dic. 2019 a las 20:12, Johannes Schindelin (<Johannes.Schindelin@gmx.de>) escribió:

Show 5 quoted lines

>
> Hi Miriam,
>
> welcome to the Git project!
>

Thank you! :)

Show 7 quoted lines

> On Sun, 8 Dec 2019, Miriam Rubio wrote:
>
> > From: Tanushree Tumane <tanushreetumane@gmail.com>
>
> Is this really a patch authored by Tanushree, or is this a fix written by
> you, intended to fix a patch authored by Tanushree?
>

This is really a patch authored by Tanushree.

Show 27 quoted lines

> If it is the latter, please use a completely new commit message and take
> the authorship yourself.
>
> > Move `error("...%s...", branch.buf);` before `strbuf_release(&branch);`
> > to release string buffer `branch` and the memory it used.
>
> This describes the "what?", but the patch already does that. The commit
> message should be more about the "why?".
>
> In this instance, I believe that the commit message should read more like
> this:
>
> -- snip --
> bisect--helper: avoid free-after-use
>
> In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,
> 2019-01-02), the `git bisect reset` subcommand was ported to C. When the
> call to `git checkout` failed, an error message was reported to the
> user.
>
> However, this error message used the `strbuf` that had just been
> released already. Let's switch that around: first use it, then release
> it.
>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> -- snap --
>

Ok! Thank you for your advice with the message.

Show 29 quoted lines

> >
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
> >  builtin/bisect--helper.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 1fbe156e67..3055b2bb50 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)
> >
> >               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
> >               if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> > +                     error(_("could not check out original"
> > +                             " HEAD '%s'. Try 'git bisect"
> > +                             " reset <commit>'."), branch.buf);
> >                       strbuf_release(&branch);
> >                       argv_array_clear(&argv);
> > -                     return error(_("could not check out original"
> > -                                    " HEAD '%s'. Try 'git bisect"
> > -                                    " reset <commit>'."), branch.buf);
> > +                     return -1;
>
> The patch looks good.
>

Great :). I'll change the commit message and I'll resend the patch.

> Thanks!
> Johannes

Thanks, Miriam

Show 8 quoted lines

>
> >               }
> >               argv_array_clear(&argv);
> >       }
> > --
> > 2.21.0 (Apple Git-122.2)
> >
> >