Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 20, 2016, 20:01 UTC
Message-ID: <52f6241f-e584-d830-ca66-084dc509c7fe@gmx.net>
In-Reply-To: <01020157c38b1ad3-ea75ed97-2514-427e-8e57-9f10efd4e6e9-000000@eu-west-1.amazonses.com>

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

Show 45 quoted lines

> 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);

[...]

Show 7 quoted lines

> @@ -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