Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer <hidden>
Date: 2016-11-20 20:01:37
Hi, 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 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);[...]
quoted hunk ↗ jump to hunk
@@ -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