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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2015, 18:22 UTC
Message-ID: <xmqq8u9hsn0j.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1439129506-9989-5-git-send-email-Karthik.188@gmail.com>

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

Show 5 quoted lines

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

Show 8 quoted lines

> +	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);
> +}