[PATCH] Get correct column with for options in command usage

[PATCH] Get correct column with for options in command usage

[Jiang Xin]

Command usage would not align well if command options are translated,
especially to CJK. Call utf8_strwidth in function usage_argh, so that
the caller will get correct column width.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 parse-options.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 67e98..ca0e6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -473,7 +473,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
-	const char *s;
+	const char *s, *p;
 	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
 	if (opts->flags & PARSE_OPT_OPTARG)
 		if (opts->long_name)
@@ -482,7 +482,9 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	p = opts->argh ? _(opts->argh) : _("...");
+	fprintf(outfile, s, p);
+	return utf8_strwidth(p) + strlen(s) - 2;
 }
 
 #define USAGE_OPTS_WIDTH 24
-- 
1.8.1.1.367.g57acac9.dirty

Re: [PATCH] Get correct column with for options in command usage

[Duy Nguyen]

On Tue, Feb 05, 2013 at 03:40:32PM +0800, Jiang Xin wrote:
> Command usage would not align well if command options are translated,
> especially to CJK. Call utf8_strwidth in function usage_argh, so that
> the caller will get correct column width.

Yeah, I just noticed a misalignment in Vietnamese translation (just
two spaces to the left, hard to notice unless paying attention).

>  static int usage_argh(const struct option *opts, FILE *outfile)
>  {
> -	const char *s;
> +	const char *s, *p;
>  	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
>  	if (opts->flags & PARSE_OPT_OPTARG)
>  		if (opts->long_name)
> @@ -482,7 +482,9 @@ static int usage_argh(const struct option *opts, FILE *outfile)
>  			s = literal ? "[%s]" : "[<%s>]";
>  	else
>  		s = literal ? " %s" : " <%s>";
> -	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
> +	p = opts->argh ? _(opts->argh) : _("...");
> +	fprintf(outfile, s, p);
> +	return utf8_strwidth(p) + strlen(s) - 2;
>  }

First of all, #include "utf8.h" is required for utf8_strwidth().

The "strlen(s) - 2" is quite sensitive to how "s" is written. How
about this? A bit longer but clearer. Your version is OK too with a
comment explaining "strlen(s) - 2".

-- 8< --
diff --git a/parse-options.c b/parse-options.c
index 67e98a6..0f803bd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -474,6 +474,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
+	struct strbuf sb = STRBUF_INIT;
+	int columns;
 	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
 	if (opts->flags & PARSE_OPT_OPTARG)
 		if (opts->long_name)
@@ -482,7 +484,11 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	strbuf_addf(&sb, s, opts->argh ? _(opts->argh) : _("..."));
+	fprintf(outfile, sb.buf);
+	columns = utf8_strwidth(sb.buf);
+	strbuf_release(&sb);
+	return columns;
 }
 
 #define USAGE_OPTS_WIDTH 24
-- 8< --

While looking at parse-options.c, I notice "-NUM" is not marked for
translation. I think you might want to mark it so that you can
translate "NUM" to a similar abbreviation in a local language. A
similar patch like this is required so we get columns for "-NUM"
translation instead of the number of bytes.
--
Duy

Re: [PATCH] Get correct column with for options in command usage

[Duy Nguyen]

On Tue, Feb 5, 2013 at 7:15 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> +       fprintf(outfile, sb.buf);

Use fputs instead. I looked up fputs man page but somehow still left
fprintf there.
-- 
Duy

Re: [PATCH] Get correct column with for options in command usage

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 5, 2013 at 7:15 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> +       fprintf(outfile, sb.buf);
>
> Use fputs instead. I looked up fputs man page but somehow still left
> fprintf there.

Once the streams of "oops that was wrong" comments are done, I'd
appreciate if one of you guys send a finished patch for application,
instead of forcing me to follow all the messages in the discussion
and picking up pieces.

Thanks.

[PATCH v2 1/2] Get correct column with for options in command usage

[Jiang Xin]

Command usage would not align well if command options are translated,
especially to CJK. Call utf8_strwidth in function usage_argh, so that
the caller will get correct column width.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 67e98..cd029f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "utf8.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -473,7 +474,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
-	const char *s;
+	const char *s, *p;
 	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
 	if (opts->flags & PARSE_OPT_OPTARG)
 		if (opts->long_name)
@@ -482,7 +483,10 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	p = opts->argh ? _(opts->argh) : _("...");
+	fprintf(outfile, s, p);
+	/* Remove extra 2 chars ("%s" in s) to get column width of utf8 string */
+	return utf8_strwidth(p) + strlen(s) - 2;
 }
 
 #define USAGE_OPTS_WIDTH 24
-- 
1.8.1.1.368.g6034fad.dirty

[PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Jiang Xin]

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@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;
+		}
 
 		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
 		    !(opts->flags & PARSE_OPT_NOARG))
-- 
1.8.1.1.368.g6034fad.dirty

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@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.

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Jiang Xin]

2013/2/6 Junio C Hamano <gitster@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

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> 2013/2/6 Junio C Hamano <gitster@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.

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Jiang Xin]

2013/2/6 Junio C Hamano <gitster@pobox.com>:
> Jiang Xin <worldhello.net@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.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 010-51262007, 18601196889

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> 2013/2/6 Junio C Hamano <gitster@pobox.com>:
>> Jiang Xin <worldhello.net@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.

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Duy Nguyen]

On Wed, Feb 6, 2013 at 11:35 AM, Junio C Hamano <gitster@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

Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

Please, no.  That line of reasoning shows a horrible design taste
(or lack of taste).  "We don't use X right now" (or "We will promise
never to use X", for that matter) is never a good reason to abuse a
name that normal people would closely associate with X to something
that is completely different.  That leads to more confusion, not
less.

I guess utf8_fprintf() is not so bad after all.  fprintf() without
the utf8_ prefix is perfectly capable of showing a string encoded in
UTF-8, and anybody can correctly guess that the magic utf8_ prefix
would introduce (i.e. the difference between utf8_fprintf and
fprintf) can only be about the return value.  It can be reasonably
expected that everybody would then know that the display column
count can be the only sane return value that is different from what
fprintf() would return.

[PATCH v4] Add utf8_fprintf helper which returns correct columns

[Jiang Xin]

Since command usages can be translated, they may not align well especially
when they are translated to CJK. A wrapper utf8_fprintf can help to return
the correct columns required.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c |  5 +++--
 utf8.c          | 22 ++++++++++++++++++++++
 utf8.h          |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 67e98..a6ce9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "utf8.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
 }
 
 #define USAGE_OPTS_WIDTH 24
@@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->long_name)
 			pos += fprintf(outfile, "--%s", opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(outfile, "-NUM");
+			pos += utf8_fprintf(outfile, _("-NUM"));
 
 		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
 		    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/utf8.c b/utf8.c
index a4ee6..05925 100644
--- a/utf8.c
+++ b/utf8.c
@@ -430,6 +430,28 @@ int same_encoding(const char *src, const char *dst)
 }
 
 /*
+ * Wrapper for fprintf and returns the total number of columns required
+ * for the printed string, assuming that the string is utf8.
+ */
+int utf8_fprintf(FILE *stream, const char *format, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list arg;
+	int columns;
+
+	va_start (arg, format);
+	strbuf_vaddf(&buf, format, arg);
+	va_end (arg);
+
+	columns = fputs(buf.buf, stream);
+	/* If no error occurs, returns columns really required with utf8_strwidth. */
+	if (0 <= columns)
+		columns = utf8_strwidth(buf.buf);
+	strbuf_release(&buf);
+	return columns;
+}
+
+/*
  * Given a buffer and its encoding, return it re-encoded
  * with iconv.  If the conversion fails, returns NULL.
  */
diff --git a/utf8.h b/utf8.h
index a2142..501b2 100644
--- a/utf8.h
+++ b/utf8.h
@@ -8,6 +8,7 @@ int utf8_strwidth(const char *string);
 int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
 int same_encoding(const char *, const char *);
+int utf8_fprintf(FILE *, const char *, ...);
 
 void strbuf_add_wrapped_text(struct strbuf *buf,
 		const char *text, int indent, int indent2, int width);
-- 
1.8.1.1.368.g917cd65

Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns

[Torsten Bögershausen]

On 08.02.13 03:10, Jiang Xin wrote:
> Since command usages can be translated, they may not align well especially
> when they are translated to CJK. A wrapper utf8_fprintf can help to return
> the correct columns required.
> 
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  parse-options.c |  5 +++--
>  utf8.c          | 22 ++++++++++++++++++++++
>  utf8.h          |  1 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 67e98..a6ce9e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -3,6 +3,7 @@
>  #include "cache.h"
>  #include "commit.h"
>  #include "color.h"
> +#include "utf8.h"
>  
>  static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  			       const char * const *usagestr,
> @@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
>  			s = literal ? "[%s]" : "[<%s>]";
>  	else
>  		s = literal ? " %s" : " <%s>";
> -	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
> +	return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
>  }
>  
>  #define USAGE_OPTS_WIDTH 24
> @@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
>  		if (opts->long_name)
>  			pos += fprintf(outfile, "--%s", opts->long_name);
>  		if (opts->type == OPTION_NUMBER)
> -			pos += fprintf(outfile, "-NUM");
> +			pos += utf8_fprintf(outfile, _("-NUM"));
>  
>  		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
>  		    !(opts->flags & PARSE_OPT_NOARG))
> diff --git a/utf8.c b/utf8.c
> index a4ee6..05925 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -430,6 +430,28 @@ int same_encoding(const char *src, const char *dst)
>  }
>  
>  /*
> + * Wrapper for fprintf and returns the total number of columns required
> + * for the printed string, assuming that the string is utf8.
> + */
> +int utf8_fprintf(FILE *stream, const char *format, ...)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	va_list arg;
> +	int columns;
> +
> +	va_start (arg, format);
> +	strbuf_vaddf(&buf, format, arg);
> +	va_end (arg);
> +
> +	columns = fputs(buf.buf, stream);
> +	/* If no error occurs, returns columns really required with utf8_strwidth. */
> +	if (0 <= columns)
> +		columns = utf8_strwidth(buf.buf);
> +	strbuf_release(&buf);
> +	return columns;
> +}
> +

I don't think we handle the return code from fputs() correctly.

Please dee below for specifications on fprintf(),
something like the following could do:
 
int utf8_fprintf(FILE *stream, const char *format, ...)
{
	struct strbuf buf = STRBUF_INIT;
	va_list arg;
	int columns = 0;

	va_start (arg, format);
	strbuf_vaddf(&buf, format, arg);
	va_end (arg);

	if (EOF != fputs(buf.buf, stream))
		columns = utf8_strwidth(buf.buf);
	strbuf_release(&buf);
	return columns;
}

And as a side note: would fprintf_strwidth() be a better name?


Linux:
RETURN VALUE
       fputc(), putc() and  putchar()  return  the  character  written  as  an
       unsigned char cast to an int or EOF on error.

       puts()  and  fputs()  return a nonnegative number on success, or EOF on
       error.

Mac OS:
COMPATIBILITY
     fputs() now returns a non-negative number (as opposed to 0) on successful
     completion.  As a result, many tests (e.g., "fputs() == 0", "fputs() !=
     0") do not give the desired result.  Use "fputs() != EOF" or "fputs() ==
     EOF" to determine success or failure.

Posix:
RETURN VALUE
       Upon successful completion, fputs() shall return a non-negative number.
       Otherwise, it shall return EOF, set an error indicator for the  stream,
       and set errno to indicate the error.

Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns

[Torsten Bögershausen]

(Sorry for confusing: I should have written:)

Please see below for specifications on fputs()

Linux:
RETURN VALUE
fputc(), putc() and putchar() return the character written as an unsigned char cast to an int or EOF on error.
puts() and fputs() return a nonnegative number on success, or EOF on error.

Mac OS:
COMPATIBILITY
fputs() now returns a non-negative number (as opposed to 0) on successful completion. As a result, many tests (e.g., "fputs() == 0", "fputs() != 0") do not give the desired result.
Use "fputs() != EOF" or "fputs() == EOF" to determine success or failure.

Posix:
RETURN VALUE
Upon successful completion, fputs() shall return a non-negative number. Otherwise, it shall return EOF, set an error indicator for the stream, and set errno to indicate the error.

Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns

[Jiang Xin]

2013/2/8 Torsten Bögershausen <tboegi@web.de>:
> On 08.02.13 03:10, Jiang Xin wrote:
>> +     /* If no error occurs, returns columns really required with utf8_strwidth. */
>> +     if (0 <= columns)
>> +             columns = utf8_strwidth(buf.buf);
>> +     strbuf_release(&buf);
>> +     return columns;
>> +}
>> +
>
> I don't think we handle the return code from fputs() correctly.
>
> Please dee below for specifications on fprintf(),
> something like the following could do:
>
> int utf8_fprintf(FILE *stream, const char *format, ...)
> {
>         struct strbuf buf = STRBUF_INIT;
>         va_list arg;
>         int columns = 0;
>
>         va_start (arg, format);
>         strbuf_vaddf(&buf, format, arg);
>         va_end (arg);
>
>         if (EOF != fputs(buf.buf, stream))
>                 columns = utf8_strwidth(buf.buf);
>         strbuf_release(&buf);
>         return columns;
> }

As fputs() returns a non-negative number (as opposed to 0) on
successful completion,
Test fputs() return value as "fputs() >=0" is correct, while "fputs()
== 0", "fputs() != 0"
are wrong. I think it's OK, must I send a new re-roll for this?

EOF is defined as (-1) in stdio.h:

    #define EOF     (-1)

> And as a side note: would fprintf_strwidth() be a better name?

This is a nice candidate.


-- 
Jiang Xin

Re: [PATCH v4] Add utf8_fprintf helper which returns correct columns

[Torsten Bögershausen]

On 08.02.13 08:20, Jiang Xin wrote:
> 2013/2/8 Torsten Bögershausen <tboegi@web.de>:
>> On 08.02.13 03:10, Jiang Xin wrote:
>>> +     /* If no error occurs, returns columns really required with utf8_strwidth. */
>>> +     if (0 <= columns)
>>> +             columns = utf8_strwidth(buf.buf);
>>> +     strbuf_release(&buf);
>>> +     return columns;
>>> +}
>>> +
>>
>> I don't think we handle the return code from fputs() correctly.
>>
>> Please dee below for specifications on fprintf(),
>> something like the following could do:
>>
>> int utf8_fprintf(FILE *stream, const char *format, ...)
>> {
>>         struct strbuf buf = STRBUF_INIT;
>>         va_list arg;
>>         int columns = 0;
>>
>>         va_start (arg, format);
>>         strbuf_vaddf(&buf, format, arg);
>>         va_end (arg);
>>
>>         if (EOF != fputs(buf.buf, stream))
>>                 columns = utf8_strwidth(buf.buf);
>>         strbuf_release(&buf);
>>         return columns;
>> }
> 
> As fputs() returns a non-negative number (as opposed to 0) on
> successful completion,
> Test fputs() return value as "fputs() >=0" is correct, while "fputs()
> == 0", "fputs() != 0"
> are wrong. I think it's OK, must I send a new re-roll for this?
> 
> EOF is defined as (-1) in stdio.h:
> 
>     #define EOF     (-1)
> 
>> And as a side note: would fprintf_strwidth() be a better name?
> 
> This is a nice candidate.
> 
> 

Doing a slurp(coffe) followed by a re-read,
I think we are save with the existng code.

However, I feel that the commit message can be improved, 
as it fixes problems not only for CJK but e.g. european languages
(and all systems using utf-8)

>Since command usages can be translated, they may not align well especially
>when they are translated to CJK. A wrapper utf8_fprintf can help to return
>the correct columns required.
 
Since command usages can be translated, they may include utf-8 encoded strings,
and strlen() is different from strwidth().
A wrapper utf8_fprintf() helps to return the correct columns required.


Thanks for working on this.

[PATCH v5] Add utf8_fprintf helper that returns correct columns

[Jiang Xin]

Since command usages can be translated, they may include utf-8 encoded
strings, and the output in console may not align well any more. This is
because strlen() is different from strwidth() on utf-8 strings. A wrapper
utf8_fprintf() can help to return the correct columns required.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Torsten Bögershausen <tboegi@web.de>
---
 parse-options.c |  5 +++--
 utf8.c          | 21 +++++++++++++++++++++
 utf8.h          |  1 +
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 67e98..a6ce9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "utf8.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
 }
 
 #define USAGE_OPTS_WIDTH 24
@@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->long_name)
 			pos += fprintf(outfile, "--%s", opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(outfile, "-NUM");
+			pos += utf8_fprintf(outfile, _("-NUM"));
 
 		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
 		    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/utf8.c b/utf8.c
index a4ee6..c09dc 100644
--- a/utf8.c
+++ b/utf8.c
@@ -430,6 +430,27 @@ int same_encoding(const char *src, const char *dst)
 }
 
 /*
+ * Wrapper for fprintf and returns the total number of columns required
+ * for the printed string, assuming that the string is utf8.
+ */
+int utf8_fprintf(FILE *stream, const char *format, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list arg;
+	int columns;
+
+	va_start (arg, format);
+	strbuf_vaddf(&buf, format, arg);
+	va_end (arg);
+
+	columns = fputs(buf.buf, stream);
+	if (0 <= columns) /* keep the error from the I/O */
+		columns = utf8_strwidth(buf.buf);
+	strbuf_release(&buf);
+	return columns;
+}
+
+/*
  * Given a buffer and its encoding, return it re-encoded
  * with iconv.  If the conversion fails, returns NULL.
  */
diff --git a/utf8.h b/utf8.h
index a2142..501b2 100644
--- a/utf8.h
+++ b/utf8.h
@@ -8,6 +8,7 @@ int utf8_strwidth(const char *string);
 int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
 int same_encoding(const char *, const char *);
+int utf8_fprintf(FILE *, const char *, ...);
 
 void strbuf_add_wrapped_text(struct strbuf *buf,
 		const char *text, int indent, int indent2, int width);
-- 
1.8.1.1.370.gf39fc7e

[PATCH v3] Add utf8_fprintf helper which returns correct columns

[Jiang Xin]

Since command usages can be translated, they may not align well especially
when they are translated to CJK. A wrapper utf8_fprintf can help to return
the correct columns required.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c |  5 +++--
 utf8.c          | 20 ++++++++++++++++++++
 utf8.h          |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 67e98..a6ce9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "utf8.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -482,7 +483,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+	return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
 }
 
 #define USAGE_OPTS_WIDTH 24
@@ -541,7 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->long_name)
 			pos += fprintf(outfile, "--%s", opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(outfile, "-NUM");
+			pos += utf8_fprintf(outfile, _("-NUM"));
 
 		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
 		    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/utf8.c b/utf8.c
index a4ee6..52dbd 100644
--- a/utf8.c
+++ b/utf8.c
@@ -430,6 +430,26 @@ int same_encoding(const char *src, const char *dst)
 }
 
 /*
+ * Wrapper for fprintf and returns the total number of columns required
+ * for the printed string, assuming that the string is utf8.
+ */
+int utf8_fprintf(FILE *stream, const char *format, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list arg;
+	int columns;
+
+	va_start (arg, format);
+	strbuf_vaddf(&buf, format, arg);
+	va_end (arg);
+
+	fputs(buf.buf, stream);
+	columns = utf8_strwidth(buf.buf);
+	strbuf_release(&buf);
+	return columns;
+}
+
+/*
  * Given a buffer and its encoding, return it re-encoded
  * with iconv.  If the conversion fails, returns NULL.
  */
diff --git a/utf8.h b/utf8.h
index a2142..501b2 100644
--- a/utf8.h
+++ b/utf8.h
@@ -8,6 +8,7 @@ int utf8_strwidth(const char *string);
 int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
 int same_encoding(const char *, const char *);
+int utf8_fprintf(FILE *, const char *, ...);
 
 void strbuf_add_wrapped_text(struct strbuf *buf,
 		const char *text, int indent, int indent2, int width);
-- 
1.8.1.1.370.g47b6ee8.dirty