Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 17, 2016, 21:32 UTC
Message-ID: <b54f7f46-a3c0-3334-24fa-e8d1e7d8f653@gmx.net>
In-Reply-To: <01020157c38b1ad5-0f90c88e-2077-4155-94e9-7d71dbbac38f-000000@eu-west-1.amazonses.com>

Hi,

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

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 317d671..6a5878c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c

[...]

Show 6 quoted lines

> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	int i;
> +	const char bisect_term_usage[] =
> +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
> +"--term-old | --term-new";

Three things:

(1) Is that indentation intentional?

(2) You have a "]" at the end of the first part of the string instead of the end of the second part.

(3) After the correction, bisect_term_usage and git_bisect_helper_usage[7] are the same strings. I don't recommend to use git_bisect_helper_usage[7] instead because keeping the index up-to-date is a maintenance hell. (At the end of your patch series it is a 3 instead of a 7.) However, if - for whatever reason - the usage of bisect--helper --bisect-terms changes, you always have to sync the two strings which is also nasty....

Show 8 quoted lines

> +
> +	if (get_terms(terms))
> +		return error(_("no terms defined"));
> +
> +	if (argc > 1) {
> +		usage(bisect_term_usage);
> +		return -1;
> +	}

...and since you only use it once, why not simply do something like

return error(_("--bisect-term requires exactly one argument"));

and drop the definition of bisect_term_usage.

Show 5 quoted lines

> +
> +	if (argc == 0) {
> +		printf(_("Your current terms are %s for the old state\nand "
> +		       "%s for the new state.\n"), terms->term_good,
> +		       terms->term_bad);

Very minor: It improves the readability if you'd split the string after the \n and put the "and "in the next line.

Show 13 quoted lines

> +		return 0;
> +	}
> +
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(argv[i], "--term-good"))
> +			printf("%s\n", terms->term_good);
> +		else if (!strcmp(argv[i], "--term-bad"))
> +			printf("%s\n", terms->term_bad);
> +		else
> +			die(_("invalid argument %s for 'git bisect "
> +				  "terms'.\nSupported options are: "
> +				  "--term-good|--term-old and "
> +				  "--term-bad|--term-new."), argv[i]);

Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in this case. Because I am always looking from a library perspective, I'd prefer "return error(...)".

Show 9 quoted lines

> @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		terms.term_bad = xstrdup(argv[1]);
>  		res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);
>  		break;
> +	case BISECT_TERMS:
> +		if (argc > 1)
> +			die(_("--bisect-terms requires 0 or 1 argument"));
> +		res = bisect_terms(&terms, argv, argc);
> +		break;

Also here: "terms" is leaking...

~Stephan