From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 13 Mar 2012 22:24:52 GMT
Subject: Re: [PATCH v7 01/9] Add column layout skeleton and git-column
Message-ID: <7vaa3kt9zf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1294ed94f981a7029b47a44143c3eb9fb6b820f1.1331638663.git.pclouds@gmail.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.

Regarding the "denser" mode, I very much like the simplicity of the idea.
I was wondering if a solution that aims for the maximum density that does
not shuffle the original order of items would end up taking the output
from "fmt" and distributing the words on each line evenly to the width,
which would be totally unusable. Your "punt at an item that does not fit
and restart from there" is simple and seems to work well.

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.

Thanks.