Re: [PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 17, 2016, 20:59 UTC
Message-ID: <b78a4cbf-86ed-938e-1d41-6c48e0df981e@gmx.net>
In-Reply-To: <01020157c38b1adb-ab4c90ed-d084-40b5-a037-f62c76e52ec4-000000@eu-west-1.amazonses.com>

Hi Pranit,

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

> Also reimplement `bisect_voc` shell function in C and call it from
> `bisect_next_check` implementation in C.

Please don't! ;D

Show 9 quoted lines

> +static char *bisect_voc(char *revision_type)
> +{
> +	if (!strcmp(revision_type, "bad"))
> +		return "bad|new";
> +	if (!strcmp(revision_type, "good"))
> +		return "good|old";
> +
> +	return NULL;
> +}

Why not simply use something like this:

static const char *voc[] = {
	"bad|new",
	"good|old",
};

Then...

Show 7 quoted lines

> +static int bisect_next_check(const struct bisect_terms *terms,
> +			     const char *current_term)
> +{
> +	int missing_good = 1, missing_bad = 1, retval = 0;
> +	char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> +	char *good_glob = xstrfmt("%s-*", terms->term_good);
> +	char *bad_syn, *good_syn;

...you don't need bad_syn and good_syn...

> +	bad_syn = xstrdup(bisect_voc("bad"));
> +	good_syn = xstrdup(bisect_voc("good"));

...and hence not these two lines...

Show 5 quoted lines

> +	if (!is_empty_or_missing_file(git_path_bisect_start())) {
> +		error(_("You need to give me at least one %s and "
> +			"%s revision. You can use \"git bisect %s\" "
> +			"and \"git bisect %s\" for that. \n"),
> +			bad_syn, good_syn, bad_syn, good_syn);

...and write
			voc[0], voc[1], voc[0], voc[1]);
instead...

Show 9 quoted lines

> +		retval = -1;
> +		goto finish;
> +	}
> +	else {
> +		error(_("You need to start by \"git bisect start\". You "
> +			"then need to give me at least one %s and %s "
> +			"revision. You can use \"git bisect %s\" and "
> +			"\"git bisect %s\" for that.\n"),
> +			good_syn, bad_syn, bad_syn, good_syn);

...and here
			voc[1], voc[0], voc[0], voc[1]);
...

Show 13 quoted lines

> +		retval = -1;
> +		goto finish;
> +	}
> +	goto finish;
> +finish:
> +	if (!bad_ref)
> +		free(bad_ref);
> +	if (!good_glob)
> +		free(good_glob);
> +	if (!bad_syn)
> +		free(bad_syn);
> +	if (!good_syn)
> +		free(good_syn);

...and you can remove the 4 lines above.

> +	return retval;
> +}

Besides that, there are again some things that I've already mentioned and that can be applied here, too, for example, not capitalizing TERM_GOOD and TERM_BAD, the goto fail simplification, the terms memory leak.

Cheers Stephan