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
[...]
> +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....
> +
> + 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.
> +
> + 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.
> + 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(...)".
> @@ -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