Hey Stephan,
On Fri, Nov 18, 2016 at 3:02 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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?
Yes it was intentional but now I cannot recollect why. I think it was
because I found something similar. Nevertheless, I will fix this
indentation/
> (2) You have a "]" at the end of the first part of the string instead of
> the end of the second part.
This should be corrected.
> (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.
Sure that would be better.
>> +
>> + 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.
Ah. This is because of the message. If I do the other way, then it
won't match the output in one of the tests in t/t6030 thus, I am
keeping it that way in order to avoid modifying the file t/t6030.
>> + 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(...)".
I should use 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...
Will have to free it.
> ~Stephan