Re: [PATCH v7 01/10] Add git-column for columnar display
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Mar 2, 2012, 12:36 UTC
- 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>:
Show 9 quoted lines
>> + 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.
Show 22 quoted lines
>> +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.
Show 6 quoted lines
>> + 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.
Show 13 quoted lines
>> + } >> + 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