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

Hey Stephan,

On Mon, Oct 30, 2017 at 10:04 PM, Stephan Beyer [off-list ref] wrote:

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

quoted

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

[...]

quoted

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

No good reason. I will make the change.

quoted

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

Also this probably does weird things with GETTEXT_POISON. I am
investigating what's happening as Martin pointed out in other thread..

quoted

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

Yeah. I missed this

quoted

+     }
+
+     return 0;
+}
+

Stephan

Regards,
Pranit Bauva