Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 6, 2015, 19:38 UTC
Message-ID: <vpqlhbfg52l.fsf@grenoble-inp.fr>
In-Reply-To: <CAOLa=ZSr19+0OVoyACdN_YfSFeWugtS40WNu0GB9=uZFEu8N5A@mail.gmail.com>

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

Show 19 quoted lines

> On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> which does not play well with the implementation of --column as done
>>> in tag.c. Where, If I'm not wrong the --column option captures all
>>> output, formats it and then prints it to stdout. Hence when using
>>> colors, we're told that the printing isn't done to a tty which
>>> supports colors, hence we lose out on colors.
>>
>> What I don't understand is how --column is different from --no-column
>> wrt colors.
>>
>> In any case, this should be explained better in comments.
>
> Well, If we use column the way we do in tag.c then it'll replace the tty
> and color will not print color because it will assume the output tty doesn't
> support colors.
>
> I hope that's what you're asking

Looking a bit more closely at the code, I think I understand what's going on. branch.c used print_columns which does all the clever things wrt columns display. tag.c used run_column_filter which pipes the output to "git column" (hence, indeed, color detection does not work since we're writting to a pipe).

The branch.c way seems good to me (why fork another process when we can format the list ourselves ?). Probably tag.c could (should?) be modified to use it too. It should just be justified, like, replacing this commit message with:

-- 8< -- To allow column display, we will need to first render the output in a string list to allow print_columns() to compute the proper size of each column before starting the actual output. Introduce the function format_ref_array_item() that does the formatting of a ref_array_item to an strbuf.

show_ref_array_item() is kept as a convenience wrapper around it which obtains the strbuf and prints it the standard output. -- 8< --

and adding a comment next to if (column_active(colopts)) { in patch 8/9:

	/* format to a string_list to let print_columns() do its job */

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/