From: Stephan Beyer <s-beyer@gmx.net>
Date: Thu, 17 Nov 2016 20:25:05 GMT
Subject: Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
Message-ID: <b7691fac-9642-f87d-f23f-5175b5ead05b@gmx.net>
In-Reply-To: <01020157c38b1aca-0c26fb8c-404f-4f57-afe7-7ebb552a1002-000000@eu-west-1.amazonses.com>

Hi Pranit,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3f19b68..c6c11e3 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-clean-state"),
>  	N_("git bisect--helper --bisect-reset [<commit>]"),
>  	N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
> +	N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),

Here's the same as in the previous patch... I'd not use
TERM_GOOD/TERM_BAD in capitals.

>  	NULL
>  };
>  
> @@ -212,6 +213,38 @@ static int bisect_write(const char *state, const char *rev,
>  	return retval;
>  }
>  
> +static int set_terms(struct bisect_terms *terms, const char *bad,
> +		     const char *good)
> +{
> +	terms->term_good = xstrdup(good);
> +	terms->term_bad = xstrdup(bad);
> +	return write_terms(terms->term_bad, terms->term_good);

At this stage of the patch series I am wondering why you are setting
"terms" here, but I guess you'll need it later.

However, you are leaking memory here. Something like

	free(terms->term_good);
	free(terms->term_bad);
	terms->term_good = xstrdup(good);
	terms->term_bad = xstrdup(bad);

should be safe (because you've always used xstrdup() for the terms
members before). Or am I overseeing something?

> @@ -278,6 +314,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		terms.term_bad = xstrdup(argv[3]);
>  		res = bisect_write(argv[0], argv[1], &terms, nolog);
>  		break;
> +	case CHECK_AND_SET_TERMS:
> +		if (argc != 3)
> +			die(_("--check-and-set-terms requires 3 arguments"));
> +		terms.term_good = xstrdup(argv[1]);
> +		terms.term_bad = xstrdup(argv[2]);
> +		res = check_and_set_terms(&terms, argv[0]);
> +		break;

Ha! When I reviewed the last patch, I asked you why you changed the code
from returning directly from each subcommand to setting res; break; and
then return res at the bottom of the function.

Now I see why this was useful. The two members of "terms" are again
leaking memory: you are allocating memory by using xstrdup() but you are
not freeing it.
(That also applies to the last patch.)

Cheers,
Stephan