Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Oct 30, 2017, 16:34 UTC
Message-ID: <af5f503a-6c2c-436f-ae74-cfbb11cb7a7f@gmx.net>
In-Reply-To: <0102015f5e5ee303-449f1b76-ff53-433c-8973-8b4850466810-000000@eu-west-1.amazonses.com>

On 10/27/2017 05:06 PM, Pranit Bauva wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0f9c3e63821b8..ab0580ce0089a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c

[...]

Show 14 quoted lines

> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	int i;
> +
> +	if (get_terms(terms))
> +		return error(_("no terms defined"));
> +
> +	if (argc > 1)
> +		return error(_("--bisect-term requires exactly one argument"));
> +
> +	if (argc == 0)
> +		return !printf(_("Your current terms are %s for the old state\n"
> +				 "and %s for the new state.\n"),
> +				 terms->term_good, terms->term_bad);

Same as in 1/8: you probably want "printf(...); return 0;" except there is a good reason.

Show 6 quoted lines

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

The last two printfs: I think there is no point in translating "%s\n", so using "%s\n" instead of _("%s\n") looks more reasonable.

Show 5 quoted lines

> +		else
> +			error(_("BUG: invalid argument %s for 'git bisect terms'.\n"
> +				  "Supported options are: "
> +				  "--term-good|--term-old and "
> +				  "--term-bad|--term-new."), argv[i]);

Should this be "return error(...)"?

Show 5 quoted lines

> +	}
> +
> +	return 0;
> +}
> +

Stephan