git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 6, 2018, 02:33 UTC
Message-ID
<xmqqpnvjexzf.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<20181105192059.20303-13-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 14 quoted lines
> 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.
>
> Kill it and produce the option name with optname(). The user will use
> error() directly. This solves the second and third problems.

The proposed log message is not very friendly to reviewers, as there is no hint what optname() does nor where it came from; it turns out that this patch introduces it.

    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.
or something like that perhaps.

Theoretically not very friendly to topics in flight, but I do not expect there would be any right now that wants to add new callers of opterror().

I do agree with the reasoning behind this change. Thanks for working on it.

Show 239 quoted lines
>
> 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>
> ---
>  builtin/merge.c     |  2 +-
>  builtin/revert.c    |  3 ++-
>  parse-options-cb.c  |  7 ++++---
>  parse-options.c     | 46 +++++++++++++++++++++++++--------------------
>  parse-options.h     |  5 +----
>  ref-filter.c        |  8 +++++---
>  t/t4211-line-log.sh |  2 +-
>  7 files changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 92ba7e1c6d..82248d43c3 100644
> --- 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
> -		return opterror(opt, "requires a value", 0);
> +		return error(_("option `%s' requires a value"), opt->long_name);
>  
>  	if (buf->len)
>  		strbuf_addch(buf, '\n');
> diff --git a/builtin/revert.c b/builtin/revert.c
> index c93393c89b..11190d2ab4 100644
> --- 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)
> -		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;
>  }
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index e8236534ac..813eb6301b 100644
> --- 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)
> -			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)
> @@ -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)
> -		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;
>  }
> diff --git a/parse-options.c b/parse-options.c
> index 3b874a83a0..0bf817193d 100644
> --- 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
> -		return opterror(opt, "requires a value", flags);
> +		return error(_("%s requires a value"), optname(opt, flags));
>  	return 0;
>  }
>  
> @@ -49,7 +49,6 @@ static int opt_command_mode_error(const struct option *opt,
>  				  int flags)
>  {
>  	const struct option *that;
> -	struct strbuf message = 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(&message, ": incompatible with %s", that_name.buf);
> +		error(_("%s is incompatible with %s"),
> +		      optname(opt, flags), that_name.buf);
>  		strbuf_release(&that_name);
> -		opterror(opt, message.buf, flags);
> -		strbuf_release(&message);
>  		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,
> @@ -86,11 +85,11 @@ static int get_value(struct parse_opt_ctx_t *p,
>  	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))
> -		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))
> -		return opterror(opt, "takes no value", flags);
> +		return error(_("%s takes no value"), optname(opt, flags));
>  
>  	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 opterror(opt, "expects a numerical value", flags);
> +			return error(_("%s expects a numerical value"),
> +				     optname(opt, flags));
>  		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))
> -			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:
> @@ -257,7 +257,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
>  			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;
> @@ -773,12 +774,17 @@ void NORETURN usage_msg_opt(const char *msg,
>  	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)
> -		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 dd14911a29..2e146aa6fa 100644
> --- 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);
> -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 -----*/
>  
> diff --git a/ref-filter.c b/ref-filter.c
> index 2a05619211..0681359100 100644
> --- 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) {
> -			return opterror(opt, "is incompatible with --merged", 0);
> +			return error(_("option `%s' is incompatible with --merged"),
> +				     opt->long_name);
>  		} 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)
> -		return opterror(opt, "must point to a commit", 0);
> +		return error(_("option `%s' must point to a commit"), opt->long_name);
>  
>  	return 0;
>  }
> diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
> index ef1322148e..bd5fe4d148 100755
> --- 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 &&
> -		grep '$2' errors
> +		test_i18ngrep '$2' errors
>  	"
>  }
Previous: Nguyễn Thái Ngọc DuyNext: Ramsay Jones
Message 46 of 71 in “Mark more strings for translation”
  1. 00/12 Mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  2. 01/12 git.c: mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  3. 02/12 alias.c: mark split_cmdline_strerror() strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  4. 03/12 archive.c: mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  5. 04/12 attr.c: mark more string for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  6. 05/12 read-cache.c: mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  7. 06/12 read-cache.c: add missing colon separatorsNguyễn Thái Ngọc Duy, Oct 28, 2018
  8. 07/12 reflog: mark strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  9. 09/12 repack: mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  10. 08/12 remote.c: mark messages for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  11. Junio C HamanoOct 29, 2018
  12. Duy NguyenOct 29, 2018
  13. 11/12 parse-options.c: mark more strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  14. 10/12 parse-options: replace opterror() with optname()Nguyễn Thái Ngọc Duy, Oct 28, 2018
  15. 12/12 fsck: mark strings for translationNguyễn Thái Ngọc Duy, Oct 28, 2018
  16. SZEDER GáborOct 29, 2018
  17. Junio C HamanoOct 29, 2018
  18. Duy NguyenOct 29, 2018
  19. Ævar Arnfjörð BjarmasonOct 29, 2018
  20. Ævar Arnfjörð BjarmasonOct 29, 2018
  21. Jiang XinNov 1, 2018
  22. Jonathan NiederOct 30, 2018
  23. Duy NguyenNov 5, 2018
  24. 00/16 Mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  25. 01/16 git.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  26. Junio C HamanoNov 6, 2018
  27. 02/16 alias.c: mark split_cmdline_strerror() strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  28. 03/16 archive.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  29. Junio C HamanoNov 6, 2018
  30. 04/16 attr.c: mark more string for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  31. 05/16 read-cache.c: turn die("internal error") to BUG()Nguyễn Thái Ngọc Duy, Nov 5, 2018
  32. Junio C HamanoNov 6, 2018
  33. 06/16 read-cache.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  34. 07/16 read-cache.c: add missing colon separatorsNguyễn Thái Ngọc Duy, Nov 5, 2018
  35. Junio C HamanoNov 6, 2018
  36. 09/16 remote.c: turn some error() or die() to BUG()Nguyễn Thái Ngọc Duy, Nov 5, 2018
  37. Junio C HamanoNov 6, 2018
  38. 08/16 reflog: mark strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  39. Junio C HamanoNov 6, 2018
  40. 11/16 repack: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  41. 10/16 remote.c: mark messages for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  42. 13/16 parse-options.c: turn some die() to BUG()Nguyễn Thái Ngọc Duy, Nov 5, 2018
  43. Junio C HamanoNov 6, 2018
  44. 14/16 parse-options.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  45. 12/16 parse-options: replace opterror() with optname()Nguyễn Thái Ngọc Duy, Nov 5, 2018
  46. Junio C HamanoNov 6, 2018
  47. Ramsay JonesNov 6, 2018
  48. Jeff KingNov 6, 2018
  49. Duy NguyenNov 10, 2018
  50. Ramsay JonesNov 10, 2018
  51. 15/16 fsck: reduce word legos to help i18nNguyễn Thái Ngọc Duy, Nov 5, 2018
  52. Junio C HamanoNov 6, 2018
  53. Duy NguyenNov 10, 2018
  54. 16/16 fsck: mark strings for translationNguyễn Thái Ngọc Duy, Nov 5, 2018
  55. 00/16 Mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  56. 16/16 fsck: mark strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  57. 03/16 archive.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  58. 02/16 alias.c: mark split_cmdline_strerror() strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  59. 04/16 attr.c: mark more string for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  60. 06/16 read-cache.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  61. 07/16 read-cache.c: add missing colon separatorsNguyễn Thái Ngọc Duy, Nov 10, 2018
  62. 14/16 parse-options.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  63. 01/16 git.c: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  64. 09/16 remote.c: turn some error() or die() to BUG()Nguyễn Thái Ngọc Duy, Nov 10, 2018
  65. 05/16 read-cache.c: turn die("internal error") to BUG()Nguyễn Thái Ngọc Duy, Nov 10, 2018
  66. 12/16 parse-options: replace opterror() with optname()Nguyễn Thái Ngọc Duy, Nov 10, 2018
  67. 10/16 remote.c: mark messages for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  68. 15/16 fsck: reduce word legos to help i18nNguyễn Thái Ngọc Duy, Nov 10, 2018
  69. 13/16 parse-options.c: turn some die() to BUG()Nguyễn Thái Ngọc Duy, Nov 10, 2018
  70. 11/16 repack: mark more strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018
  71. 08/16 reflog: mark strings for translationNguyễn Thái Ngọc Duy, Nov 10, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.