From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Wed, 09 Feb 2011 11:24:23 GMT
Subject: Re: [PATCH 2/7] Add column layout
Message-ID: <AANLkTimuae1rDn2+UH3mg1zBS5LsS5++8z1-yY0cBmhB@mail.gmail.com>
In-Reply-To: <20110209073655.GA2135@elie>

2011/2/9 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> +++ b/column.c
>> @@ -0,0 +1,177 @@
> [...]
>> +static int item_length(const struct column_layout *c, const char *s)
>> +{
>> +     int a_len = 0;
>> +
>> +     if (!(c->mode & COL_ANSI))
>> +             return strlen(s);
>> +
>> +     while ((s = strstr(s, "\033[")) != NULL) {
>> +             int len = strspn(s+2, "0123456789;");
>> +             s += len+3; /* \033[<len><func char> */
>> +             a_len += len+3;
>> +     }
>> +     return a_len;
>> +}
>
> I think you mean "return strlen(orig_s) - a_len".
>
> Something like the following could be more obvious, though
> it is unfortunately verbose.
>
>        int len = 0;
>        while (*s) {
>                const char *next;
>
>                /* \033[<len><func char> */
>                if (!prefixcmp(s, "\033[")) {
>                        s += strlen("\033[");
>                        s += strspn(s, "0123456789;");
>                        if (!*s)
>                                ... handle somehow ...
>                        s++;
>                }
>
>                next = strchrnul(s, '\033');
>                len += next - s;
>                s = next;
>        }
>
> Both miscompute the width of "Dépôt".  Something like this can do ok
> if the string is modifiable and we know it is UTF-8:
>
>                char save;
>                ...
>                next = strchrnul(s, '\033');
>
>                /*
>                 * NEEDSWORK: a utf8_strwidth variant that
>                 * accepts a memory area not terminated by \0
>                 * would avoid this ugliness.
>                 */
>                save = *next;
>                *next = '\0';
>                len += utf8_strwidth(s);
>                *next = save;
>
> POSIX does not provide a strwidth function, so if we want to handle
> encodings like SHIFT-JIS then something uglier[1] might come to life.

I think UTF-8 is enough. Git does not produce complex ansi escape
codes. Perhaps checking them at the beginning and reset code at the
end is enough.

> [...]
>> +static void relayout(const struct column_layout *c,
>> +                  int padding, int spare,
>> +                  int *initial_width, int **width,
>> +                  int *rows, int *cols)
>> +{
>> +     int new_rows, new_cols, new_initial_width;
>> +     int i, *new_width, new_spare, total_width;
>> +
>> +     /*
>> +      * Assume all columns have same width, we would need
>> +      * initial_width*cols. But then after squeezing, we have
>> +      * "spare" more chars. Assume a new total_width with
>> +      * additional chars, then re-squeeze to see if it fits
>> +      * c->width.
>> +      */
>
> Might be easier to debug if this part were deferred to a separate
> patch. :)
>
>> +     total_width = (*initial_width)*(*cols) + spare;
>
> An odd heuristic.  Does it work well in practice?

It seems so. If it does not, I guess I would need to look into how GNU
ls does it.
-- 
Duy