2012/2/29 Junio C Hamano <gitster@xxxxxxxxx>:
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html