Re: [PATCH] column: Fix an incorrect parse of the 'nodense' option token

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 12, 2012, 03:37 UTC
Message-ID: <CACsJy8B7=6OYBV-HVK3Cq4_cTdPvm_1MvVv=U2R=4yQw9t5X9Q@mail.gmail.com>
In-Reply-To: <4F36B64D.4030000@ramsay1.demon.co.uk>

On Sun, Feb 12, 2012 at 1:41 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:

Show 12 quoted lines

>
> The parse_option() function always saw the 'nodense' token as
> 'dense', since it stripped the 'no' off the input argument string
> earlier when checking for the presence of the 'color' token.
>
> In order to fix the parse, we use local variables (within the loop)
> initialised to the function input arguments instead, which allows
> each token check to be independent.
>
> Also, add some 'nodense' tests to t/t9002-column.sh.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Acked-by: Nguyen Thai Ngoc Duy <pclouds@gmail.com>

Show 9 quoted lines

> Note that the code supports the 'nocolor' token, but it is not documented
> in config.txt.
>
> If I understand correctly (and it's quite possible that I don't!), the
> 'nodense' token has the same meaning as the absence of the 'dense' token
> (and I assume a similar comment applies to the '[no]color' tokens). If that
> is the case, then maybe a better solution would be to simply remove the
> 'nodense' and 'nocolor' tokens. I will leave it to you to decide which
> solution is more appropriate.

It's about overriding config. If you set "dense" by default in column.ui but do not want it in this particular run, you can say --column=nodense.

The [no]color is for plumbing only. If a command produces colored output, "color" is required to calculate text length correctly. Overriding it with "nocolor" would break the layout badly so it's no use there. It does not make sense (to me) for users to put "color" in column.ui. Which is why it's not mentioned in document.

-- 
Duy