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

Hi,

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

[...]

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

Also here: "terms" is leaking...

~Stephan