On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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
>
compensation it is.
probably have a single strlen() call and set the value to another variable.
>> + 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.
>
Not sure what you meant.
But this does not act on empty strings and that was intentional. It
does not make
sense to have an alignment on an empty string, where the caller could themselves
just do a `strbuf_addchars(&buf, " ", width)`.
>> + 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.
>
With regards to its usage in ref-filter, this is needed, don't you think?
>> + 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);
>> +}
Thanks!
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html