git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 01/10] Add git-column for columnar display

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 28, 2012, 18:10 UTC
Message-ID
<7v8vjm25iy.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1330430331-19945-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 17 quoted lines
> 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".
Show 38 quoted lines
> 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?

Show 50 quoted lines
> +	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?

Show 22 quoted lines
> +		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?
Show 7 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?

Show 13 quoted lines
> +/*
> + * 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.
Previous: Nguyễn Thái Ngọc DuyNext: Nguyen Thai Ngoc Duy
Message 25 of 66 in “Column display”
  1. 00/11 Column displayNguyễn Thái Ngọc Duy, Feb 25, 2012
  2. 01/11 column: add API to print items in columnsNguyễn Thái Ngọc Duy, Feb 25, 2012
  3. 02/11 Add git-column and column mode parsingNguyễn Thái Ngọc Duy, Feb 25, 2012
  4. Ramsay JonesFeb 27, 2012
  5. Nguyen Thai Ngoc DuyFeb 28, 2012
  6. 03/11 Stop starting pager recursivelyNguyễn Thái Ngọc Duy, Feb 25, 2012
  7. 04/11 column: add columnar layoutNguyễn Thái Ngọc Duy, Feb 25, 2012
  8. 05/11 column: support columns with different widthsNguyễn Thái Ngọc Duy, Feb 25, 2012
  9. Junio C HamanoFeb 26, 2012
  10. 06/11 column: add column.ui for default column output settingsNguyễn Thái Ngọc Duy, Feb 25, 2012
  11. Junio C HamanoFeb 27, 2012
  12. Nguyen Thai Ngoc DuyFeb 27, 2012
  13. 07/11 help: reuse print_columns() for help -aNguyễn Thái Ngọc Duy, Feb 25, 2012
  14. 08/11 branch: add --columnNguyễn Thái Ngọc Duy, Feb 25, 2012
  15. 09/11 status: add --columnNguyễn Thái Ngọc Duy, Feb 25, 2012
  16. 10/11 column: support piping stdout to external git-column processNguyễn Thái Ngọc Duy, Feb 25, 2012
  17. 11/11 tag: add --columnNguyễn Thái Ngọc Duy, Feb 25, 2012
  18. Junio C HamanoFeb 26, 2012
  19. Nguyen Thai Ngoc DuyFeb 27, 2012
  20. Junio C HamanoFeb 27, 2012
  21. Junio C HamanoFeb 27, 2012
  22. Nguyen Thai Ngoc DuyFeb 27, 2012
  23. 00/10 Column displayNguyễn Thái Ngọc Duy, Feb 28, 2012
  24. 01/10 Add git-column for columnar displayNguyễn Thái Ngọc Duy, Feb 28, 2012
  25. Junio C HamanoFeb 28, 2012
  26. Nguyen Thai Ngoc DuyMar 2, 2012
  27. 02/10 Stop starting pager recursivelyNguyễn Thái Ngọc Duy, Feb 28, 2012
  28. Junio C HamanoFeb 28, 2012
  29. Junio C HamanoFeb 28, 2012
  30. Nguyen Thai Ngoc DuyFeb 29, 2012
  31. Junio C HamanoFeb 29, 2012
  32. Nguyen Thai Ngoc DuyFeb 29, 2012
  33. Junio C HamanoFeb 29, 2012
  34. 03/10 column: add columnar layoutNguyễn Thái Ngọc Duy, Feb 28, 2012
  35. Junio C HamanoFeb 28, 2012
  36. 04/10 column: add dense layout supportNguyễn Thái Ngọc Duy, Feb 28, 2012
  37. Junio C HamanoFeb 28, 2012
  38. Nguyen Thai Ngoc DuyMar 2, 2012
  39. Junio C HamanoMar 2, 2012
  40. 05/10 column: add column.ui for default column output settingsNguyễn Thái Ngọc Duy, Feb 28, 2012
  41. Junio C HamanoFeb 28, 2012
  42. 06/10 help: reuse print_columns() for help -aNguyễn Thái Ngọc Duy, Feb 28, 2012
  43. 07/10 branch: add --columnNguyễn Thái Ngọc Duy, Feb 28, 2012
  44. 08/10 status: add --columnNguyễn Thái Ngọc Duy, Feb 28, 2012
  45. 09/10 column: support piping stdout to external git-column processNguyễn Thái Ngọc Duy, Feb 28, 2012
  46. 10/10 tag: add --columnNguyễn Thái Ngọc Duy, Feb 28, 2012
  47. Thomas RastMar 2, 2012
  48. Nguyen Thai Ngoc DuyMar 11, 2012
  49. Junio C HamanoMar 12, 2012
  50. 00/9 Column displayNguyễn Thái Ngọc Duy, Mar 13, 2012
  51. Nguyen Thai Ngoc DuyMar 13, 2012
  52. 01/9 Add column layout skeleton and git-columnNguyễn Thái Ngọc Duy, Mar 13, 2012
  53. 02/9 Stop starting pager recursivelyNguyễn Thái Ngọc Duy, Mar 13, 2012
  54. 03/9 column: add columnar layoutNguyễn Thái Ngọc Duy, Mar 13, 2012
  55. 04/9 column: add dense layout supportNguyễn Thái Ngọc Duy, Mar 13, 2012
  56. 05/9 help: reuse print_columns() for help -aNguyễn Thái Ngọc Duy, Mar 13, 2012
  57. 06/9 branch: add --columnNguyễn Thái Ngọc Duy, Mar 13, 2012
  58. 07/9 status: add --columnNguyễn Thái Ngọc Duy, Mar 13, 2012
  59. 08/9 column: support piping stdout to external git-column processNguyễn Thái Ngọc Duy, Mar 13, 2012
  60. 09/9 tag: add --columnNguyễn Thái Ngọc Duy, Mar 13, 2012
  61. 10/9 ls-files: support --columnNguyễn Thái Ngọc Duy, Mar 13, 2012
  62. 11/9 column: support "denser" modeNguyễn Thái Ngọc Duy, Mar 13, 2012
  63. 12/9 column: support grouping entriesNguyễn Thái Ngọc Duy, Mar 13, 2012
  64. Junio C HamanoMar 13, 2012
  65. Nguyen Thai Ngoc DuyMar 14, 2012
  66. Junio C HamanoMar 14, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.