Re: [PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C
From: Stephan Beyer <hidden>
Date: 2016-11-17 20:25:16
Hi Pranit, On 10/14/2016 04:14 PM, Pranit Bauva wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
@@ -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