Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf

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

Karthik Nayak <karthik.188@gmail.com> writes:

> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
> +		       const char *s)
> +{
> +	int display_len = utf8_strnwidth(s, strlen(s), 0);
> +	int utf8_compenstation = strlen(s) - display_len;

compensation, perhaps?  But notice you are running two strlen and
then also a call to utf8-strnwidth here already, and then

> +	if (!strlen(s))
> +		return;

you return here without doing anything.

Worse yet, this logic looks very strange.  If you give it a width of
8 because you want to align like-item on each record at that column,
a record with 1-display-column item will be shown in 8-display-column
with 7 SP padding (either at the beginning, or at the end, or on
both ends to center).  If it happens to be an empty string, the entire
8-display-column disappears.

Is that really what you meant to do?  The logic does not make much
sense to me, even though the code may implement that logic correctly
and clearly.

> +	if (display_len >= width) {
> +		strbuf_addstr(buf, s);
> +		return;
> +	}

Mental note: this function values the information more than
presentation; it refuses to truncate (to lose information) and
instead accepts jaggy output.  OK.

> +	if (position == ALIGN_LEFT)
> +		strbuf_addf(buf, "%-*s", width + utf8_compenstation, s);
> +	else if (position == ALIGN_MIDDLE) {
> +		int left = (width - display_len)/2;
> +		strbuf_addf(buf, "%*s%-*s", left, "", width - left + utf8_compenstation, s);
> +	} else if (position == ALIGN_RIGHT)
> +		strbuf_addf(buf, "%*s", width + utf8_compenstation, s);
> +}