Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
> new file mode 100644
> index 0000000..508b85f
> --- /dev/null
> +++ b/Documentation/git-column.txt
> @@ -0,0 +1,49 @@
> +git-column(1)
> +=============
> +
> +NAME
> +----
> +git-column - Display data in columns
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git column' [--mode=<mode> | --rawmode=<n>] [--width=<width>]
Please spell this "--raw-mode".
> diff --git a/builtin/column.c b/builtin/column.c
> new file mode 100644
> index 0000000..3b0f74e
> --- /dev/null
> +++ b/builtin/column.c
> @@ -0,0 +1,41 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "strbuf.h"
> +#include "parse-options.h"
> +#include "string-list.h"
> +#include "column.h"
> +
> +static const char * const builtin_column_usage[] = {
> + "git column [options]",
> + NULL
> +};
> +static unsigned int colopts;
> +
> +int cmd_column(int argc, const char **argv, const char *prefix)
> +{
> + struct string_list list = STRING_LIST_INIT_DUP;
> + struct strbuf sb = STRBUF_INIT;
> + struct column_options copts;
> + struct option options[] = {
> + OPT_COLUMN(0, "mode", &colopts, "layout to use"),
> + OPT_INTEGER(0, "rawmode", &colopts, "layout to use"),
> + OPT_INTEGER(0, "width", &copts.width, "Maximum width"),
> + OPT_STRING(0, "indent", &copts.indent, "string", "Padding space on left border"),
> + OPT_INTEGER(0, "nl", &copts.nl, "Padding space on right border"),
> + OPT_INTEGER(0, "padding", &copts.padding, "Padding space between columns"),
> + OPT_END()
> + };
> +
> + 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?
> + if (argc)
> + usage_with_options(builtin_column_usage, options);
> +
> + while (!strbuf_getline(&sb, stdin, '\n'))
> + string_list_append(&list, sb.buf);
> +
> + print_columns(&list, colopts, &copts);
> + return 0;
> +}
> diff --git a/column.c b/column.c
> new file mode 100644
> index 0000000..d61da81
> --- /dev/null
> +++ b/column.c
> @@ -0,0 +1,170 @@
> +#include "cache.h"
> +#include "column.h"
> +#include "string-list.h"
> +#include "parse-options.h"
> +
> +#define MODE(mode) ((mode) & COL_MODE)
> +
> +/* Display without layout when COL_ENABLED is not set */
> +static void display_plain(const struct string_list *list,
> + const char *indent, const char *nl)
> +{
> + int i;
> +
> + for (i = 0; i < list->nr; i++)
> + printf("%s%s%s", indent, list->items[i].string, nl);
> +}
> +
> +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?
> + display_plain(list, indent, nl);
> + return;
> + }
> + die("BUG: invalid mode %d", MODE(mode));
> +}
> +
> +struct colopt {
> + enum {
> + ENABLE,
> + MODE,
> + OPTION
> + } type;
> + const char *name;
> + int value;
> +};
> +
> +/*
> + * Set COL_ENABLED and COL_ENABLED_SET. If 'set' is -1, check if
> + * stdout is tty.
> + */
> +static int set_enable_bit(unsigned int *mode, int set, int stdout_is_tty)
> +{
Somehow it looks to me that this is setting the ENABLED bit, not enable
bit.
> + 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?
> + }
> + 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?
> +/*
> + * Set COL_MODE_*. mode is intially copied from column.ui. If
> + * COL_ENABLED_SET is not set, then neither 'always', 'never' nor
> + * 'auto' has been used. Default to 'always'.
> + */
> +static int set_mode(unsigned int *mode, unsigned int value)
> +{
> + *mode = (*mode & ~COL_MODE) | value;
> + if (!(*mode & COL_ENABLED_SET))
> + *mode |= COL_ENABLED | COL_ENABLED_SET;
> +
> + return 0;
> +}
I am *imagining* that "*mode" begins with some compiled-in default, then
git_config() will read from column.* variables and set "*mode" but not in
this codepath, and by the time command line option triggers this codepath,
we should have seen everything the config wants to tell us, so if the
config did not say anything (i.e. COL_ENABLED_SET is not set yet), we
force enable the feature (the user wanting it to operate in some mode is
an indication enough that the user wants to enable the machinery as a
whole). So this function is designed to be called from command line option
parsing, but never from the configuration parsing.
But the parse_option() function below calls this, which would mean it is
also for command line option parsing and not configuration parsing. The
same function however calls set_enable_bit() we saw earlier that can only
be called from configuration codepath. What is going on?
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.
The remainder of the patch unreviewed.
--
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