Hey Stephan,

On Fri, Nov 18, 2016 at 3:02 AM, Stephan Beyer [off-list ref] wrote:

Hi,

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

quoted

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

[...]

quoted

+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....

quoted

+
+     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.

quoted

+
+     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.

quoted

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

quoted

@@ -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