parse-options: replace opterror() with optname()

Introduce optname() that does the early half of original opterror() to
come up with the name of the option reported back to the user, and use
it to kill opterror().  The callers of opterror() now directly call
error() using the string returned by opterror() instead.

There are a few issues with opterror()

- it tries to assemble an English sentence from pieces. This is not
great for translators because we give them pieces instead of a full
sentence.

- It's a wrapper around error() and needs some hack to let the
compiler know it always returns -1.

- Since it takes a string instead of printf format, one call site has
to assemble the string manually before passing to it.

Using error() directly solves the second and third problems.

It kind helps the first problem as well because "%s does foo" does
give a translator a full sentence in a sense and let them reorder if
needed. But it has limitations, if the subject part has to change
based on the rest of the sentence, that language is screwed. This is
also why I try to avoid calling optname() when 'flags' is known in
advance.

Mark of these strings for translation as well while at there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/merge.c b/builtin/merge.c
index 92ba7e1c6dc2a7d3cce1259ec6faaead188f53d9..82248d43c3f7e21743b2ce3bc60aaa4e2af5ea6a 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -128,7 +128,7 @@ static int option_read_message(struct parse_opt_ctx_t *ctx,
                ctx->argc--;
                arg = *++ctx->argv;
        } else
                ctx->argc--;
                arg = *++ctx->argv;
        } else

-               return opterror(opt, "requires a value", 0);
+               return error(_("option `%s' requires a value"), opt->long_name);

 
        if (buf->len)
                strbuf_addch(buf, '\n');
 
        if (buf->len)
                strbuf_addch(buf, '\n');

diff --git a/builtin/revert.c b/builtin/revert.c
index c93393c89be0290ebd9e300046ff56a2ee441715..11190d2ab43c91354c640e77c71d7a814e251eb0 100644 (file)
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -69,7 +69,8 @@ static int option_parse_m(const struct option *opt,
 
        replay->mainline = strtol(arg, &end, 10);
        if (*end || replay->mainline <= 0)
 
        replay->mainline = strtol(arg, &end, 10);
        if (*end || replay->mainline <= 0)

-               return opterror(opt, "expects a number greater than zero", 0);
+               return error(_("option `%s' expects a number greater than zero"),
+                            opt->long_name);

 
        return 0;
 }
 
        return 0;
 }

diff --git a/parse-options-cb.c b/parse-options-cb.c
index e8236534ac8aa00fde2a5a9ae27e85c72d062f3a..813eb6301bc4e1132ea7b5198b7277ad10d6fb4f 100644 (file)
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -18,7 +18,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
        } else {
                v = strtol(arg, (char **)&arg, 10);
                if (*arg)
        } else {
                v = strtol(arg, (char **)&arg, 10);
                if (*arg)

-                       return opterror(opt, "expects a numerical value", 0);
+                       return error(_("option `%s' expects a numerical value"),
+                                    opt->long_name);

                if (v && v < MINIMUM_ABBREV)
                        v = MINIMUM_ABBREV;
                else if (v > 40)
                if (v && v < MINIMUM_ABBREV)
                        v = MINIMUM_ABBREV;
                else if (v > 40)


@@ -54,8 +55,8 @@ int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
                arg = unset ? "never" : (const char *)opt->defval;
        value = git_config_colorbool(NULL, arg);
        if (value < 0)
                arg = unset ? "never" : (const char *)opt->defval;
        value = git_config_colorbool(NULL, arg);
        if (value < 0)

-               return opterror(opt,
-                       "expects \"always\", \"auto\", or \"never\"", 0);
+               return error(_("option `%s' expects \"always\", \"auto\", or \"never\""),
+                            opt->long_name);

        *(int *)opt->value = value;
        return 0;
 }
        *(int *)opt->value = value;
        return 0;
 }

diff --git a/parse-options.c b/parse-options.c
index 3b874a83a0c897845deeec37c1a3b85e0b625050..0bf817193d02fe3741b589719adebc5c57547009 100644 (file)
--- a/parse-options.c
+++ b/parse-options.c
@@ -32,7 +32,7 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
                p->argc--;
                *arg = *++p->argv;
        } else
                p->argc--;
                *arg = *++p->argv;
        } else

-               return opterror(opt, "requires a value", flags);
+               return error(_("%s requires a value"), optname(opt, flags));

        return 0;
 }
 
        return 0;
 }
 


@@ -49,7 +49,6 @@ static int opt_command_mode_error(const struct option *opt,
                                  int flags)
 {
        const struct option *that;
                                  int flags)
 {
        const struct option *that;

-       struct strbuf message = STRBUF_INIT;

        struct strbuf that_name = STRBUF_INIT;
 
        /*
        struct strbuf that_name = STRBUF_INIT;
 
        /*


@@ -67,13 +66,13 @@ static int opt_command_mode_error(const struct option *opt,
                        strbuf_addf(&that_name, "--%s", that->long_name);
                else
                        strbuf_addf(&that_name, "-%c", that->short_name);
                        strbuf_addf(&that_name, "--%s", that->long_name);
                else
                        strbuf_addf(&that_name, "-%c", that->short_name);

-               strbuf_addf(&message, ": incompatible with %s", that_name.buf);
+               error(_("%s is incompatible with %s"),
+                     optname(opt, flags), that_name.buf);

                strbuf_release(&that_name);
                strbuf_release(&that_name);

-               opterror(opt, message.buf, flags);
-               strbuf_release(&message);

                return -1;
        }
                return -1;
        }

-       return opterror(opt, ": incompatible with something else", flags);
+       return error(_("%s : incompatible with something else"),
+                    optname(opt, flags));

 }
 
 static int get_value(struct parse_opt_ctx_t *p,
 }
 
 static int get_value(struct parse_opt_ctx_t *p,


@@ -86,11 +85,11 @@ static int get_value(struct parse_opt_ctx_t *p,
        int err;
 
        if (unset && p->opt)
        int err;
 
        if (unset && p->opt)

-               return opterror(opt, "takes no value", flags);
+               return error(_("%s takes no value"), optname(opt, flags));

        if (unset && (opt->flags & PARSE_OPT_NONEG))
        if (unset && (opt->flags & PARSE_OPT_NONEG))

-               return opterror(opt, "isn't available", flags);
+               return error(_("%s isn't available"), optname(opt, flags));

        if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
        if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))

-               return opterror(opt, "takes no value", flags);
+               return error(_("%s takes no value"), optname(opt, flags));

 
        switch (opt->type) {
        case OPTION_LOWLEVEL_CALLBACK:
 
        switch (opt->type) {
        case OPTION_LOWLEVEL_CALLBACK:


@@ -176,7 +175,8 @@ static int get_value(struct parse_opt_ctx_t *p,
                        return -1;
                *(int *)opt->value = strtol(arg, (char **)&s, 10);
                if (*s)
                        return -1;
                *(int *)opt->value = strtol(arg, (char **)&s, 10);
                if (*s)

-                       return opterror(opt, "expects a numerical value", flags);
+                       return error(_("%s expects a numerical value"),
+                                    optname(opt, flags));

                return 0;
 
        case OPTION_MAGNITUDE:
                return 0;
 
        case OPTION_MAGNITUDE:


@@ -191,9 +191,9 @@ static int get_value(struct parse_opt_ctx_t *p,
                if (get_arg(p, opt, flags, &arg))
                        return -1;
                if (!git_parse_ulong(arg, opt->value))
                if (get_arg(p, opt, flags, &arg))
                        return -1;
                if (!git_parse_ulong(arg, opt->value))

-                       return opterror(opt,
-                               "expects a non-negative integer value with an optional k/m/g suffix",
-                               flags);
+                       return error(_("%s expects a non-negative integer value"
+                                      " with an optional k/m/g suffix"),
+                                    optname(opt, flags));

                return 0;
 
        default:
                return 0;
 
        default:


@@ -257,7 +257,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
                        if (!rest)
                                continue;
                        if (*rest == '=')
                        if (!rest)
                                continue;
                        if (*rest == '=')

-                               return opterror(options, "takes no value", flags);
+                               return error(_("%s takes no value"),
+                                            optname(options, flags));

                        if (*rest)
                                continue;
                        p->out[p->cpidx++] = arg - 2;
                        if (*rest)
                                continue;
                        p->out[p->cpidx++] = arg - 2;


@@ -773,12 +774,17 @@ void NORETURN usage_msg_opt(const char *msg,
        usage_with_options(usagestr, options);
 }
 
        usage_with_options(usagestr, options);
 }
 

-#undef opterror
-int opterror(const struct option *opt, const char *reason, int flags)
+const char *optname(const struct option *opt, int flags)

 {
 {

+       static struct strbuf sb = STRBUF_INIT;
+
+       strbuf_reset(&sb);

        if (flags & OPT_SHORT)
        if (flags & OPT_SHORT)

-               return error("switch `%c' %s", opt->short_name, reason);
-       if (flags & OPT_UNSET)
-               return error("option `no-%s' %s", opt->long_name, reason);
-       return error("option `%s' %s", opt->long_name, reason);
+               strbuf_addf(&sb, "switch `%c'", opt->short_name);
+       else if (flags & OPT_UNSET)
+               strbuf_addf(&sb, "option `no-%s'", opt->long_name);
+       else
+               strbuf_addf(&sb, "option `%s'", opt->long_name);
+
+       return sb.buf;

 }
 }

diff --git a/parse-options.h b/parse-options.h
index dd14911a297a5b10705ecb31243c55a7dc2f193c..2e146aa6fabb0f9c10e28eacd7db9aee04a023b5 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -189,10 +189,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
                                   const struct option *options);
 
 extern int optbug(const struct option *opt, const char *reason);
                                   const struct option *options);
 
 extern int optbug(const struct option *opt, const char *reason);

-extern int opterror(const struct option *opt, const char *reason, int flags);
-#if defined(__GNUC__)
-#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
-#endif
+const char *optname(const struct option *opt, int flags);

 
 /*----- incremental advanced APIs -----*/
 
 
 /*----- incremental advanced APIs -----*/
 

diff --git a/ref-filter.c b/ref-filter.c
index 2a056192117b0d119dcd4cf9da0de982a0444b33..0681359100ba11ca1bd502a9752404f39fea2de6 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2302,9 +2302,11 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 
        if (rf->merge) {
                if (no_merged) {
 
        if (rf->merge) {
                if (no_merged) {

-                       return opterror(opt, "is incompatible with --merged", 0);
+                       return error(_("option `%s' is incompatible with --merged"),
+                                    opt->long_name);

                } else {
                } else {

-                       return opterror(opt, "is incompatible with --no-merged", 0);
+                       return error(_("option `%s' is incompatible with --no-merged"),
+                                    opt->long_name);

                }
        }
 
                }
        }
 


@@ -2318,7 +2320,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
        rf->merge_commit = lookup_commit_reference_gently(the_repository,
                                                          &oid, 0);
        if (!rf->merge_commit)
        rf->merge_commit = lookup_commit_reference_gently(the_repository,
                                                          &oid, 0);
        if (!rf->merge_commit)

-               return opterror(opt, "must point to a commit", 0);
+               return error(_("option `%s' must point to a commit"), opt->long_name);

 
        return 0;
 }
 
        return 0;
 }

diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index ef1322148e1413e4b90f46b6bea4f2d9b0300189..bd5fe4d1483005175d9c2211dee1901c2b132573 100755 (executable)
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -25,7 +25,7 @@ canned_test_failure () {
 test_bad_opts () {
        test_expect_success "invalid args: $1" "
                test_must_fail git log $1 2>errors &&
 test_bad_opts () {
        test_expect_success "invalid args: $1" "
                test_must_fail git log $1 2>errors &&

-               grep '$2' errors
+               test_i18ngrep '$2' errors

        "
 }
 
        "
 }