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

[Junio C Hamano <gitster@pobox.com>]

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.