Re: [PATCH v7 01/9] Add column layout skeleton and git-column

Nguyễn Thái Ngọc Duy  [off-list ref] 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.

2012/3/14 Junio C Hamano [off-list ref]:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

+#define COL_ENABLE(c) ((c) & COL_ENABLE_MASK)

That is a misleading name for a boolean macro.  It looked as if this

quoted

+     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