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:

> On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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
>>
>
> compensation it is.
> probably have a single strlen() call and set the value to another variable.

That is not what I meant.  If you want to keep the early return of
"doing nothing for an empty string" (which you should *NOT*, by the
way), declare these variables upfront, do the early return and then
call utf8_strnwidth() to compute the value you are going to use,
only when you know you are going to use them.  That is what I meant.

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

If it is truly intentional, then the design is wrong.  You are
writing a public function that can be called by other people.

Which one makes more sense?  Remember that you are going to have
many callers over time in the course of project in the future.

 (A) The caller is forced to always check the string that he is
     merely passing on, i.e.

	struct strbuf buf = STRBUF_INIT;
        struct strbuf out = STRBUF_INIT;

	fill_some_placeholder(&buf, fmt, args);
        if (buf.len)
        	strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);
        else
        	strbuf_addchars(&out, ' ', 8);

     only because the callee strbuf_utf8_align() refuses to do the
     trivial work.

 (B) The caller does not have to care, i.e.

	struct strbuf buf = STRBUF_INIT;
        struct strbuf out = STRBUF_INIT;

	fill_some_placeholder(&buf, fmt, args);
       	strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);

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

It simply does not make sense to force the caller to even care.
What they want is a series of lines, whose columns are aligned.

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

That is exactly why I said "OK".

I was primarily trying to lead other reviewers by example,
demonstrating that a review will become more credible if it shows
that the reviewer actually read the patch by explaining the thinking
behind what the code does in his own words.  I see too many "FWIW,
reviewed by me" without indicating if it is "from just a cursory
read it looks nice" or "I fully read and understood it and I agree
with the design choices it makes", which does not help me very much
when queuing a patch.