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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 8, 2019, 19:11 UTC
Message-ID: <nycvar.QRO.7.76.6.1912082003420.31080@tvgsbejvaqbjf.bet>
In-Reply-To: <20191208172813.16518-1-mirucam@gmail.com>

Hi Miriam,

welcome to the Git project!

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?

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 --

Show 26 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.

Thanks! Johannes

Show 7 quoted lines

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