From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Wed, 14 Mar 2012 11:17:59 GMT
Subject: Re: [PATCH v7 01/9] Add column layout skeleton and git-column
Message-ID: <CACsJy8D8U7L4P+z9DT6hq0Rze=p2M6UZKMCqia1kN-QGJFE_zg@mail.gmail.com>
In-Reply-To: <7vaa3kt9zf.fsf@alter.siamese.dyndns.org>

2012/3/14 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +#define COL_ENABLE(c) ((c) & COL_ENABLE_MASK)
>
> That is a misleading name for a boolean macro.  It looked as if this
>
>> +     assert(COL_ENABLE(colopts) != COL_AUTO);
>
> was asking the helper to *enable* the column machinery with the given set
> of option in colopts, and expecting the helper to answer how it enabled
> ("I took the 'automatic' decision path").  But that is not what is
> happening.
>
> Unfortunately, COL_ENABLED?(c) is not an option, but this seriously needs
> a better name to avoid reader confusion.

I'm running out of names. Suggestions are welcome.

> I haven't formed an opinion on your "grouping" mode yet.  The hardcoded
> slash hierarchy delimiter somewhat bothers me, but I haven't thought it
> deeply enough to judge if it is worth making it more generic. My gut
> feeling is that '/' probably is OK.

The column user should be able to decide how to display the group
line. I rely a lot on coloring, and I was already thinking about
giving group line a highlight color, which must be configurable
because different column user uses different base colors.
-- 
Duy