Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
On Wed, Feb 6, 2013 at 11:35 AM, Junio C Hamano gits...@pobox.com wrote: How about this since [PATCH v3]: diff --git a/utf8.c b/utf8.c index 52dbd..b893a 100644 --- a/utf8.c +++ b/utf8.c @@ -443,8 +443,11 @@ int utf8_fprintf(FILE *stream, const char *format, ...) strbuf_vaddf(buf, format, arg); va_end (arg); - fputs(buf.buf, stream); - columns = utf8_strwidth(buf.buf); + columns = fputs(buf.buf, stream); + /* If no error occurs, and really write something (columns 0), +* calculate really columns width with utf8_strwidth. */ + if (columns 0) + columns = utf8_strwidth(buf.buf); strbuf_release(buf); return columns; } The above bugfix does not address my original concern about the name, though. How about utf8_fwprintf? wprintf() deals with wide characters and returns the number of wide characters, I think the name fits. And we could just drop utf8_ and use the existing name, because we don't use wchar_t* anyway. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
Jiang Xin worldhello@gmail.com writes:
Signed-off-by: Jiang Xin worldhello@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
parse-options.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index cd029f..be916 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -497,6 +497,8 @@ static int usage_with_options_internal(struct
parse_opt_ctx_t *ctx,
const struct option *opts, int full, int
err)
{
FILE *outfile = err ? stderr : stdout;
+ const char *opt_num_buff = _(-NUM);
+ int opt_num_size = utf8_strwidth(opt_num_buff);
if (!usagestr)
return PARSE_OPT_HELP;
@@ -544,8 +546,10 @@ static int usage_with_options_internal(struct
parse_opt_ctx_t *ctx,
pos += fprintf(outfile, , );
if (opts-long_name)
pos += fprintf(outfile, --%s, opts-long_name);
- if (opts-type == OPTION_NUMBER)
- pos += fprintf(outfile, -NUM);
+ if (opts-type == OPTION_NUMBER) {
+ fputs(opt_num_buff, outfile);
+ pos += opt_num_size;
+ }
I somehow suspect that this is going in a direction that makes this
piece of code much less maintainable.
Look at the entire function and see how many places you do fprintf
on strings that are marked with _(). short_name and long_name are
not likely to be translated, but everything else is, especially
multiple places that show _(opts-help) neither of these patches
touch.
I wonder if it makes more sense to add a helper function that
returns the number of column positions (not bytes) with a signature
similar to fprintf() and use that throughout the function instead.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
2013/2/6 Junio C Hamano gits...@pobox.com: I somehow suspect that this is going in a direction that makes this piece of code much less maintainable. Look at the entire function and see how many places you do fprintf on strings that are marked with _(). short_name and long_name are not likely to be translated, but everything else is, especially multiple places that show _(opts-help) neither of these patches touch. I wonder if it makes more sense to add a helper function that returns the number of column positions (not bytes) with a signature similar to fprintf() and use that throughout the function instead. I agree, a helper named 'utf8_fprintf' in utf8.c is better. I will send a patch latter. -- Jiang Xin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
Jiang Xin worldhello@gmail.com writes: 2013/2/6 Junio C Hamano gits...@pobox.com: I somehow suspect that this is going in a direction that makes this piece of code much less maintainable. Look at the entire function and see how many places you do fprintf on strings that are marked with _(). short_name and long_name are not likely to be translated, but everything else is, especially multiple places that show _(opts-help) neither of these patches touch. I wonder if it makes more sense to add a helper function that returns the number of column positions (not bytes) with a signature similar to fprintf() and use that throughout the function instead. I agree, a helper named 'utf8_fprintf' in utf8.c is better. I will send a patch latter. Yeah, the idea of a helper function I agree with; I am not thrilled with the name utf8_fprintf() though. People use the return value of fprintf() for error detection (negative return value means an error) most of the time (even though non-negative value gives the number of bytes shown), but the primary use of the return value from the utf8_fprintf() function will be to get the display width, and the name does not quite capture that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
2013/2/6 Junio C Hamano gits...@pobox.com: Jiang Xin worldhello@gmail.com writes: I agree, a helper named 'utf8_fprintf' in utf8.c is better. I will send a patch latter. Yeah, the idea of a helper function I agree with; I am not thrilled with the name utf8_fprintf() though. People use the return value of fprintf() for error detection (negative return value means an error) most of the time (even though non-negative value gives the number of bytes shown), but the primary use of the return value from the utf8_fprintf() function will be to get the display width, and the name does not quite capture that. How about this since [PATCH v3]: diff --git a/utf8.c b/utf8.c index 52dbd..b893a 100644 --- a/utf8.c +++ b/utf8.c @@ -443,8 +443,11 @@ int utf8_fprintf(FILE *stream, const char *format, ...) strbuf_vaddf(buf, format, arg); va_end (arg); - fputs(buf.buf, stream); - columns = utf8_strwidth(buf.buf); + columns = fputs(buf.buf, stream); + /* If no error occurs, and really write something (columns 0), +* calculate really columns width with utf8_strwidth. */ + if (columns 0) + columns = utf8_strwidth(buf.buf); strbuf_release(buf); return columns; } -- 蒋鑫 北京群英汇信息技术有限公司 邮件: worldhello@gmail.com 网址: http://www.ossxp.com/ 博客: http://www.worldhello.net/ 微博: http://weibo.com/gotgit/ 电话: 010-51262007, 18601196889 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
Jiang Xin worldhello@gmail.com writes: 2013/2/6 Junio C Hamano gits...@pobox.com: Jiang Xin worldhello@gmail.com writes: I agree, a helper named 'utf8_fprintf' in utf8.c is better. I will send a patch latter. Yeah, the idea of a helper function I agree with; I am not thrilled with the name utf8_fprintf() though. People use the return value of fprintf() for error detection (negative return value means an error) most of the time (even though non-negative value gives the number of bytes shown), but the primary use of the return value from the utf8_fprintf() function will be to get the display width, and the name does not quite capture that. How about this since [PATCH v3]: diff --git a/utf8.c b/utf8.c index 52dbd..b893a 100644 --- a/utf8.c +++ b/utf8.c @@ -443,8 +443,11 @@ int utf8_fprintf(FILE *stream, const char *format, ...) strbuf_vaddf(buf, format, arg); va_end (arg); - fputs(buf.buf, stream); - columns = utf8_strwidth(buf.buf); + columns = fputs(buf.buf, stream); + /* If no error occurs, and really write something (columns 0), +* calculate really columns width with utf8_strwidth. */ + if (columns 0) + columns = utf8_strwidth(buf.buf); strbuf_release(buf); return columns; } Yeah, the error checking is necessary; it would make your intention more clear to say: if (0 = columns) columns = utf8_strwidth(buf.buf); though, as buf.buf _may_ be an empty string, and with the if statement you are saying we return the width only when output did not result in an error. The above bugfix does not address my original concern about the name, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
