Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms,
> const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos, retval = 0;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp = NULL;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + }
> +
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + const char *arg = argv[i];
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + } else if (!strcmp(arg, "--no-checkout")) {
> + no_checkout = 1;
> + } else if (!strcmp(arg, "--term-good") ||
> + !strcmp(arg, "--term-old")) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(argv[++i]);
All these xstrdup() for the terms here and below will leak memory.
I recommend to use xstrdup() also at (*) below, and use
free(terms->term_good) above this line (and for every occurrence below,
of course).
> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
[...]
> @@ -497,6 +705,11 @@ int cmd_bisect__helper(int argc, const char **argv,
> const char *prefix)
> die(_("--bisect-terms requires 0 or 1 argument"));
> res = bisect_terms(&terms, argv, argc);
> break;
> + case BISECT_START:
> + terms.term_good = "good";
> + terms.term_bad = "bad";
Here is (*): use xstrdup("good") etc.
And then, as already mentioned for another patch, free(terms.*) below.
I personally am a friend of small functions and would prefer something
like as follows... (This is a comment about several patches of your
series, not only this one.)
First, replace the current set_terms() by
static void set_terms(struct bisect_terms *terms, const char *bad,
const char *good)
{
terms->term_good = xstrdup(good);
terms->term_bad = xstrdup(bad);
}
ie, without calling write_terms(...).
And then replace the *current* set_terms() calls by set_terms(...);
write_terms(...); calls.
Second, add
static void get_default_terms(struct bisect_terms *terms)
{
set_terms(terms, "bad", "good");
}
and use this instead of the two lines quoted above (and all its other
occurrences).
Third, use the new set_terms() everywhere instead of settings terms
members directly (with the exception of get_terms()).
This sounds like a safer variant (with respect to leaks and handling
them) to me than doing it the current way.
~Stephan
