From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Fri, 02 Mar 2012 12:36:30 GMT
Subject: Re: [PATCH v7 01/10] Add git-column for columnar display
Message-ID: <CACsJy8BQy_j08x5k_bzcngTy4qKi9DPkKCDA1hEUR6-osRTf2g@mail.gmail.com>
In-Reply-To: <7v8vjm25iy.fsf@alter.siamese.dyndns.org>

2012/2/29 Junio C Hamano <gitster@pobox.com>:
>> +     memset(&copts, 0, sizeof(copts));
>> +     copts.width = term_columns();
>> +     copts.padding = 1;
>> +     argc = parse_options(argc, argv, "", options, builtin_column_usage, 0);
>
> Curious. The usual pattern is to set up the built-in default, call
> git_config() to override it with the configured values, and then call
> parse_options() to override at the runtime.  I see configuration callback
> defined in column.c but no call to git_config() here?

History reason. It was test-column and I did not care much whether it
respected config. Will fix.

>> +void print_columns(const struct string_list *list, unsigned int mode,
>> +                struct column_options *opts)
>> +{
>> +     const char *indent = "", *nl = "\n";
>> +     int padding = 1, width = term_columns();
>> +
>> +     if (!list->nr)
>> +             return;
>> +     if (opts) {
>> +             if (opts->indent)
>> +                     indent = opts->indent;
>> +             if (opts->nl)
>> +                     nl = opts->nl;
>> +             if (opts->width)
>> +                     width = opts->width;
>> +             padding = opts->padding;
>> +     }
>> +     if (width <= 1 || !(mode & COL_ENABLED)) {
>
> Curious why this is "1".  If your terminal is only 2 columns wide, you
> wouldn't be able to show your list items in two columns as you would want
> to have an inter-column gap, no?

Yeah, should drop that "width <= 1". Other layout functions should be
able to deal with too narrow width anyway.


>> +     if (set < 0) {  /* auto */
>> +             if (stdout_is_tty < 0)
>> +                     stdout_is_tty = isatty(1);
>> +             set = stdout_is_tty || (pager_in_use() && pager_use_color);
>
> Why does this have anything to do with the use of color?

Hm.. no it does not have to.

>> +     }
>> +     if (set)
>> +             *mode = *mode | COL_ENABLED | COL_ENABLED_SET;
>> +     else
>> +             *mode = (*mode & ~COL_ENABLED) | COL_ENABLED_SET;
>> +     return 0;
>> +}
>
> OK, so we record the desired value (either COL_ENABLED or not) and the
> fact that a call to set_enable_bit() function set it.  Which implies that
> this function must be called from only one codepath (either setting from
> the configuration mechanism, or by parsing the command line option) but
> not both.  I guess this is only called from configuration codepath?

Both share the same code path. COL_ENABLED_SET is probably badly
named, perhaps COL_ENABLED_VALID is better. It's there to deal
distinguish "never" and not explicitly stating any of
never/auto/always. In both cases, COL_ENABLED is 0. COL_ENABLED_SET is
set in the former, but not in the latter. I depend on that to make
default behavior "always". Now thinking again, it would make more
sense to set default "never", and drop COL_ENABLED_SET.

> I am afraid that we do not have enough to judge if this is sane in this
> patch, as there is no support for column.ui at this stage.  Perhaps the
> series is not structured well and has things that are not yet relevant in
> early part of it.  Sigh.

If it's not clear whether it's sane, it's more likely not. I'll
re-read the series again and redo. One good thing about git
development is only tidy things can become part of upstream, which
makes archaelogy a pleasant experience.
-- 
Duy