Hey Stephan,
On Fri, Nov 18, 2016 at 1:55 AM, Stephan Beyer <[email protected]> wrote:
> 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.
Sure.
>> 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?
Yeah it is safe.
>> @@ -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.
I will take care about freeing the memory.
Regards,
Pranit Bauva
