[PATCH/RFC 0/7] Column output

[PATCH/RFC 0/7] Column output

[Nguyễn Thái Ngọc Duy]

This series was made a year ago [1]. I'm quite happy with it so far
and also tired of maintaining off-tree. So here's another attempt to
clean it up and put it upstream.

In short, the series is very simple: give porcelain commands column
output, just like good old "ls". There could be a few more candidates,
I believe René Scharfe mentioned other files in "git status".

Another thing is how you want to customize this. Currently it supports
two modes: equal columns, and dense mode, specified with
--column=column or --column=dense. Obviously not very convenient.
There's core.column but I'm not sure how to specify modes there, and
if people like some more modes (fill rows before columns for example).

[1] http://mid.gmane.org/1267963785-473-1-git-send-email-pclouds@gmail.com

Nguyễn Thái Ngọc Duy (7):
  Move term_columns() to pager.c and save terminal width before pager
  Add column layout
  parseopt: OPT_COLUMN to set struct column_layout.mode
  add core.column
  help: reuse struct column_layout
  tag: support column output with --column
  branch: support column output with --column

 .gitignore                   |    1 +
 Documentation/config.txt     |    8 ++
 Documentation/git-branch.txt |    8 ++
 Documentation/git-tag.txt    |   11 ++-
 Makefile                     |    3 +
 builtin/branch.c             |   22 ++++-
 builtin/tag.c                |   19 ++++-
 cache.h                      |    2 +
 column.c                     |  195 ++++++++++++++++++++++++++++++++++++++++++
 column.h                     |   23 +++++
 config.c                     |   16 ++++
 environment.c                |    1 +
 help.c                       |   55 +++----------
 pager.c                      |   32 +++++++
 parse-options.h              |    2 +
 t/t9002-column.sh            |  108 +++++++++++++++++++++++
 test-column.c                |   59 +++++++++++++
 17 files changed, 514 insertions(+), 51 deletions(-)
 create mode 100644 column.c
 create mode 100644 column.h
 create mode 100755 t/t9002-column.sh
 create mode 100644 test-column.c

-- 
1.7.2.2

[PATCH 1/7] Move term_columns() to pager.c and save terminal width before pager

[Nguyễn Thái Ngọc Duy]

term_columns() checks for terminal width via ioctl(1). After
redirecting, stdin is no longer terminal to get terminal width.

Check terminal width and save it before redirect stdin in setup_pager()
and let term_columns() reuse the value.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h |    1 +
 help.c  |   22 ----------------------
 pager.c |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/cache.h b/cache.h
index d83d68c..bcbd5f2 100644
--- a/cache.h
+++ b/cache.h
@@ -1045,6 +1045,7 @@ extern void setup_pager(void);
 extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
+extern int term_columns();
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git a/help.c b/help.c
index 7654f1b..1344208 100644
--- a/help.c
+++ b/help.c
@@ -5,28 +5,6 @@
 #include "help.h"
 #include "common-cmds.h"
 
-/* most GUI terminals set COLUMNS (although some don't export it) */
-static int term_columns(void)
-{
-	char *col_string = getenv("COLUMNS");
-	int n_cols;
-
-	if (col_string && (n_cols = atoi(col_string)) > 0)
-		return n_cols;
-
-#ifdef TIOCGWINSZ
-	{
-		struct winsize ws;
-		if (!ioctl(1, TIOCGWINSZ, &ws)) {
-			if (ws.ws_col)
-				return ws.ws_col;
-		}
-	}
-#endif
-
-	return 80;
-}
-
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
diff --git a/pager.c b/pager.c
index dac358f..ad447cf 100644
--- a/pager.c
+++ b/pager.c
@@ -12,6 +12,7 @@
  */
 
 static int spawned_pager;
+static int max_columns;
 
 #ifndef WIN32
 static void pager_preexec(void)
@@ -80,6 +81,15 @@ void setup_pager(void)
 
 	spawned_pager = 1; /* means we are emitting to terminal */
 
+#ifdef TIOCGWINSZ
+	{
+		struct winsize ws;
+		if (!ioctl(1, TIOCGWINSZ, &ws)) {
+			if (ws.ws_col)
+				max_columns = ws.ws_col;
+		}
+	}
+#endif
 	/* spawn the pager */
 	pager_argv[0] = pager;
 	pager_process.use_shell = 1;
@@ -116,3 +126,25 @@ int pager_in_use(void)
 	env = getenv("GIT_PAGER_IN_USE");
 	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
 }
+
+int term_columns()
+{
+	char *col_string = getenv("COLUMNS");
+	int n_cols;
+
+	if (col_string && (n_cols = atoi(col_string)) > 0)
+		return n_cols;
+
+	else if (spawned_pager && max_columns)
+		return max_columns;
+#ifdef TIOCGWINSZ
+	else {
+		struct winsize ws;
+		if (!ioctl(1, TIOCGWINSZ, &ws)) {
+			if (ws.ws_col)
+				return ws.ws_col;
+		}
+	}
+#endif
+	return 80;
+}
-- 
1.7.2.2

Re: [PATCH 1/7] Move term_columns() to pager.c and save terminal width before pager

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> term_columns() checks for terminal width via ioctl(1).

ioctl(2). :)

I like the idea of checking width before launching a pager
and exporting it, yes.

> --- a/cache.h
> +++ b/cache.h
> @@ -1045,6 +1045,7 @@ extern void setup_pager(void);
>  extern const char *pager_program;
>  extern int pager_in_use(void);
>  extern int pager_use_color;
> +extern int term_columns();

Should say (void) rather than leaving the parameter list unspecified,
no?

It will only be needed by a few translation units, I hope.  Putting
it in column.h can avoid some pointless recompilation.

[...]
> +++ b/pager.c
> @@ -12,6 +12,7 @@
>   */
>  
>  static int spawned_pager;
> +static int max_columns;

Could be stored in the COLUMNS environment variable, but an integer
variable like this is is simpler.

[...]
> @@ -80,6 +81,15 @@ void setup_pager(void)
>  
>  	spawned_pager = 1; /* means we are emitting to terminal */
>  
> +#ifdef TIOCGWINSZ
> +	{
> +		struct winsize ws;
> +		if (!ioctl(1, TIOCGWINSZ, &ws)) {
> +			if (ws.ws_col)
> +				max_columns = ws.ws_col;
> +		}
> +	}
> +#endif

Mm, repeated code.

How about something like this on top?  Not sure if the
"if (ws.ws_col)" is useful.  I've left it alone.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    1 +
 cache.h  |    1 -
 column.h |    6 ++++++
 help.c   |    1 +
 pager.c  |   48 ++++++++++++++++++++++++++----------------------
 5 files changed, 34 insertions(+), 23 deletions(-)
 create mode 100644 column.h

diff --git a/Makefile b/Makefile
index 775ee83..ed9e94b 100644
--- a/Makefile
+++ b/Makefile
@@ -1956,6 +1956,7 @@ builtin/prune.o builtin/reflog.o reachable.o: reachable.h
 builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
 builtin/tar-tree.o archive-tar.o: tar.h
 connect.o transport.o http-backend.o: url.h
+pager.o help.o: column.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
 http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 
diff --git a/cache.h b/cache.h
index bcbd5f2..d83d68c 100644
--- a/cache.h
+++ b/cache.h
@@ -1045,7 +1045,6 @@ extern void setup_pager(void);
 extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
-extern int term_columns();
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git a/column.h b/column.h
new file mode 100644
index 0000000..55d8067
--- /dev/null
+++ b/column.h
@@ -0,0 +1,6 @@
+#ifndef COLUMN_H
+#define COLUMN_H
+
+extern int term_columns(void);
+
+#endif
diff --git a/help.c b/help.c
index 1344208..62a479b 100644
--- a/help.c
+++ b/help.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
+#include "column.h"
 #include "levenshtein.h"
 #include "help.h"
 #include "common-cmds.h"
diff --git a/pager.c b/pager.c
index ad447cf..e6f7d86 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "column.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -14,6 +15,21 @@
 static int spawned_pager;
 static int max_columns;
 
+#ifdef TIOCGWINSZ
+static int retrieve_terminal_width(void)
+{
+	struct winsize ws;
+	if (ioctl(1, TIOCGWINSZ, &ws))	/* e.g., ENOSYS */
+		return 0;
+	return ws.ws_col;
+}
+#else
+static int retrieve_terminal_width(void)
+{
+	return 0;
+}
+#endif
+
 #ifndef WIN32
 static void pager_preexec(void)
 {
@@ -75,21 +91,17 @@ const char *git_pager(int stdout_is_tty)
 void setup_pager(void)
 {
 	const char *pager = git_pager(isatty(1));
+	int width;
 
 	if (!pager)
 		return;
 
 	spawned_pager = 1; /* means we are emitting to terminal */
 
-#ifdef TIOCGWINSZ
-	{
-		struct winsize ws;
-		if (!ioctl(1, TIOCGWINSZ, &ws)) {
-			if (ws.ws_col)
-				max_columns = ws.ws_col;
-		}
-	}
-#endif
+	width = retrieve_terminal_width();
+	if (width)
+		max_columns = width;
+
 	/* spawn the pager */
 	pager_argv[0] = pager;
 	pager_process.use_shell = 1;
@@ -127,24 +139,16 @@ int pager_in_use(void)
 	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
 }
 
-int term_columns()
+int term_columns(void)
 {
 	char *col_string = getenv("COLUMNS");
 	int n_cols;
 
 	if (col_string && (n_cols = atoi(col_string)) > 0)
 		return n_cols;
-
-	else if (spawned_pager && max_columns)
+	if (spawned_pager && max_columns)
 		return max_columns;
-#ifdef TIOCGWINSZ
-	else {
-		struct winsize ws;
-		if (!ioctl(1, TIOCGWINSZ, &ws)) {
-			if (ws.ws_col)
-				return ws.ws_col;
-		}
-	}
-#endif
-	return 80;
+
+	n_cols = retrieve_terminal_width();
+	return n_cols ? n_cols : 80;
 }
-- 
1.7.4

[PATCH 2/7] Add column layout

[Nguyễn Thái Ngọc Duy]

This mainly add display_columns() that will display a table of items,
filling columns before rows.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .gitignore        |    1 +
 Makefile          |    3 +
 column.c          |  177 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 column.h          |   20 ++++++
 t/t9002-column.sh |  108 ++++++++++++++++++++++++++++++++
 test-column.c     |   59 ++++++++++++++++++
 6 files changed, 368 insertions(+), 0 deletions(-)
 create mode 100644 column.c
 create mode 100644 column.h
 create mode 100755 t/t9002-column.sh
 create mode 100644 test-column.c

diff --git a/.gitignore b/.gitignore
index 3dd6ef7..a1a1202 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,6 +162,7 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.min.*
 /test-chmtime
+/test-column
 /test-ctype
 /test-date
 /test-delta
diff --git a/Makefile b/Makefile
index 775ee83..6d74956 100644
--- a/Makefile
+++ b/Makefile
@@ -417,6 +417,7 @@ PROGRAM_OBJS += http-backend.o
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-column
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
@@ -498,6 +499,7 @@ LIB_H += builtin.h
 LIB_H += cache.h
 LIB_H += cache-tree.h
 LIB_H += color.h
+LIB_H += column.h
 LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
@@ -575,6 +577,7 @@ LIB_OBJS += branch.o
 LIB_OBJS += bundle.o
 LIB_OBJS += cache-tree.o
 LIB_OBJS += color.o
+LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
 LIB_OBJS += commit.o
 LIB_OBJS += config.o
diff --git a/column.c b/column.c
new file mode 100644
index 0000000..4b92fa5
--- /dev/null
+++ b/column.c
@@ -0,0 +1,177 @@
+#include "cache.h"
+#include "column.h"
+#include "parse-options.h"
+
+static int item_length(const struct column_layout *c, const char *s)
+{
+	int a_len = 0;
+
+	if (!(c->mode & COL_ANSI))
+		return strlen(s);
+
+	while ((s = strstr(s, "\033[")) != NULL) {
+		int len = strspn(s+2, "0123456789;");
+		s += len+3; /* \033[<len><func char> */
+		a_len += len+3;
+	}
+	return a_len;
+}
+
+static void layout(const struct column_layout *c,
+		   int total_width, int padding,
+		   int *width,
+		   int *rows, int *cols)
+{
+	int i;
+
+	*width = 0;
+	/* Find maximum column width */
+	for (i = 0; i < c->items.nr; i++) {
+		const char *s = c->items.items[i].string;
+		int len = item_length(c, s);
+		if (*width < len)
+			*width = len;
+	}
+	*width += padding;
+
+	*cols = total_width / *width;
+	if (*cols == 0)
+		*cols = 1;
+	/* items.nr <= rows*cols -> rows >= items.nr/cols */
+	*rows = c->items.nr / *cols;
+	if (c->items.nr > *rows * *cols)
+		(*rows)++;
+}
+
+static int squeeze_columns(const struct column_layout *c,
+			   int *width, int padding,
+			   int rows, int cols)
+{
+	int x, y, i, len, item_len, spare = 0;
+	const char *s;
+
+	for (x = 0; x < cols; x++) {
+		for (y = len = 0; y < rows; y++) {
+			i = x * rows + y;
+			if (i >= c->items.nr)
+				break;
+			s = c->items.items[i].string;
+			item_len = item_length(c, s);
+			if (len < item_len)
+				len = item_len;
+		}
+		len += padding;
+		if (len < width[x]) {
+			spare += width[x] - len;
+			width[x] = len;
+		}
+	}
+	return spare;
+}
+
+static void relayout(const struct column_layout *c,
+		     int padding, int spare,
+		     int *initial_width, int **width,
+		     int *rows, int *cols)
+{
+	int new_rows, new_cols, new_initial_width;
+	int i, *new_width, new_spare, total_width;
+
+	/*
+	 * Assume all columns have same width, we would need
+	 * initial_width*cols. But then after squeezing, we have
+	 * "spare" more chars. Assume a new total_width with
+	 * additional chars, then re-squeeze to see if it fits
+	 * c->width.
+	 */
+	total_width = (*initial_width)*(*cols) + spare;
+	layout(c, total_width, padding,
+	       &new_initial_width, &new_rows, &new_cols);
+	new_width = xmalloc(sizeof(*new_width) * new_cols);
+	for (i = 0; i < new_cols; i++)
+		new_width[i] = new_initial_width;
+	new_spare = squeeze_columns(c, new_width, padding, new_rows, new_cols);
+
+	/* Does it fit? */
+	if (total_width - new_spare < c->width) {
+		free(*width);
+		*width = new_width;
+		*initial_width = new_initial_width;
+		*rows = new_rows;
+		*cols = new_cols;
+	}
+}
+
+static void display_columns_first(const struct column_layout *c,
+				  int padding, const char *indent)
+{
+	int x, y, i, cols, rows, initial_width, *width;
+	char *empty_cell;
+
+	layout(c, c->width, padding, &initial_width, &rows, &cols);
+	width = xmalloc(sizeof(*width) * cols);
+	for (i = 0; i < cols; i++)
+		width[i] = initial_width;
+
+	if (c->mode & COL_DENSE) {
+		int spare = squeeze_columns(c, width, padding, rows, cols);
+		/* is it worth relayout? */
+		if (spare >= initial_width/2)
+			relayout(c, padding, spare,
+				 &initial_width, &width, &rows, &cols);
+	}
+
+	empty_cell = xmalloc(initial_width + 1);
+	memset(empty_cell, ' ', initial_width);
+	empty_cell[initial_width] = '\0';
+	for (y = 0; y < rows; y++) {
+		for (x = 0; x < cols; x++) {
+			const char *s;
+			int len;
+
+			i = x * rows + y;
+			if (i >= c->items.nr)
+				break;
+			s = c->items.items[i].string;
+			len = item_length(c, s);
+			if (width[x] < initial_width)
+				len += initial_width - width[x];
+
+			printf("%s%s%s",
+			       x == 0 ? indent : "",
+			       c->items.items[i].string,
+			       /* If the next column at same row is
+				  out of range, end the line. Else pad
+				  some space.  */
+			       i + rows >= c->items.nr ? "\n" : empty_cell + len);
+		}
+	}
+}
+
+static void display_plain(const struct column_layout *c, const char *indent)
+{
+	int i;
+
+	for (i = 0; i < c->items.nr; i++)
+		printf("%s%s\n", indent, c->items.items[i].string);
+}
+
+void display_columns(const struct column_layout *c, int padding, const char *indent)
+{
+	int mode = c->mode & COL_MODE;
+	if (!indent)
+		indent = "";
+	if (c->width <= 1)
+		mode = COL_PLAIN;
+	switch (mode) {
+	case COL_PLAIN:
+		display_plain(c, indent);
+		break;
+
+	case COL_COLUMN_FIRST:
+		display_columns_first(c, padding, indent);
+		break;
+	default:
+		die("BUG: invalid mode %d", c->mode & COL_MODE);
+	}
+}
diff --git a/column.h b/column.h
new file mode 100644
index 0000000..34435b0
--- /dev/null
+++ b/column.h
@@ -0,0 +1,20 @@
+#ifndef COLUMN_H
+#define COLUMN_H
+
+#include "string-list.h"
+
+#define COL_MODE          0x000F
+#define COL_PLAIN         0 /* Single column */
+#define COL_COLUMN_FIRST  1 /* Fill columns before rows */
+#define COL_ANSI          (1 << 4) /* Remove ANSI from string length */
+#define COL_DENSE         (1 << 5) /* "Ragged-right" mode, relayout if enough space */
+
+struct column_layout {
+	int mode;
+	int width;
+	struct string_list items;
+};
+
+extern void display_columns(const struct column_layout *c, int padding, const char *indent);
+
+#endif
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
new file mode 100755
index 0000000..1d030b0
--- /dev/null
+++ b/t/t9002-column.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='git column'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	cat >lista <<\EOF
+one
+two
+three
+four
+five
+six
+seven
+eight
+nine
+ten
+eleven
+EOF
+'
+
+test_expect_success '80 columns' '
+	cat >expected <<\EOF &&
+one    two    three  four   five   six    seven  eight  nine   ten    eleven
+EOF
+	COLUMNS=80 test-column < lista > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'COLUMNS = 1' '
+	cat >expected <<\EOF &&
+one
+two
+three
+four
+five
+six
+seven
+eight
+nine
+ten
+eleven
+EOF
+	COLUMNS=1 test-column < lista > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'width = 1' '
+	test-column --width=1 < lista > actual &&
+	test_cmp expected actual
+'
+
+COLUMNS=20
+export COLUMNS
+
+test_expect_success '20 columns' '
+	cat >expected <<\EOF &&
+one    seven
+two    eight
+three  nine
+four   ten
+five   eleven
+six
+EOF
+	test-column < lista > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '20 columns, densed' '
+	cat >expected <<\EOF &&
+one   seven
+two   eight
+three nine
+four  ten
+five  eleven
+six
+EOF
+	test-column --mode=column,dense < lista > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '20 columns, padding 2' '
+	cat >expected <<\EOF &&
+one     seven
+two     eight
+three   nine
+four    ten
+five    eleven
+six
+EOF
+	test-column --padding 2 < lista > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '20 columns, left indented' '
+	cat >expected <<\EOF &&
+  one    seven
+  two    eight
+  three  nine
+  four   ten
+  five   eleven
+  six
+EOF
+	test-column --left=2 < lista > actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/test-column.c b/test-column.c
new file mode 100644
index 0000000..4f56cd8
--- /dev/null
+++ b/test-column.c
@@ -0,0 +1,59 @@
+#include "cache.h"
+#include "strbuf.h"
+#include "parse-options.h"
+#include "column.h"
+
+static const char * const builtin_column_usage[] = {
+	"git column [--mode=<mode>] [--width=<width>] [--left-space=<N>] [--right-space=<N>]",
+	NULL
+};
+
+int main(int argc, const char **argv)
+{
+	struct column_layout c;
+	const char *mode = NULL;
+	struct strbuf sb = STRBUF_INIT;
+	int left_space = 0, right_space = 0, padding = 1;
+	int term_width = term_columns();
+	struct option options[] = {
+		OPT_INTEGER(0, "width", &term_width, "Maximum width"),
+		OPT_INTEGER(0, "left", &left_space, "Padding space on left border"),
+		OPT_INTEGER(0, "right", &right_space, "Padding space on right border"),
+		OPT_INTEGER(0, "padding", &padding, "Padding space between columns"),
+		OPT_STRING(0, "mode", &mode, "mode", "Which layout mode to use"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "", options, builtin_column_usage, 0);
+
+	memset(&c, 0, sizeof(c));
+	c.width = term_width - right_space - left_space;
+	c.items.strdup_strings = 1;
+
+	while (mode && *mode) {
+		int len = strcspn(mode, ",");
+		if (len == 6 && !strncmp(mode, "column", len))
+			c.mode |= COL_COLUMN_FIRST;
+		else if (len == 5 && !strncmp(mode, "dense", len))
+			c.mode |= COL_DENSE;
+		else if (len == 4 && !strncmp(mode, "ansi", len))
+			c.mode |= COL_ANSI;
+		else
+			die("Unknown mode %s", mode);
+		mode += len;
+		while (*mode == ',')
+			mode++;
+	}
+	if (!c.mode)
+		c.mode = COL_COLUMN_FIRST;
+	if ((c.mode & COL_MODE) == 0)
+		die("Invalid mode '%s'", mode);
+
+	while (!strbuf_getline(&sb, stdin, '\n'))
+		string_list_append(&c.items, sb.buf);
+
+	strbuf_setlen(&sb, left_space);
+	memset(sb.buf, ' ', left_space);
+	display_columns(&c, padding, sb.buf);
+	return 0;
+}
-- 
1.7.2.2

Re: [PATCH 2/7] Add column layout

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> +++ b/column.c
> @@ -0,0 +1,177 @@
[...]
> +static int item_length(const struct column_layout *c, const char *s)
> +{
> +	int a_len = 0;
> +
> +	if (!(c->mode & COL_ANSI))
> +		return strlen(s);
> +
> +	while ((s = strstr(s, "\033[")) != NULL) {
> +		int len = strspn(s+2, "0123456789;");
> +		s += len+3; /* \033[<len><func char> */
> +		a_len += len+3;
> +	}
> +	return a_len;
> +}

I think you mean "return strlen(orig_s) - a_len".

Something like the following could be more obvious, though
it is unfortunately verbose.

	int len = 0;
	while (*s) {
		const char *next;

		/* \033[<len><func char> */
		if (!prefixcmp(s, "\033[")) {
			s += strlen("\033[");
			s += strspn(s, "0123456789;");
			if (!*s)
				... handle somehow ...
			s++;
		}

		next = strchrnul(s, '\033');
		len += next - s;
		s = next;
	}

Both miscompute the width of "Dépôt".  Something like this can do ok
if the string is modifiable and we know it is UTF-8:

		char save;
		...
		next = strchrnul(s, '\033');

		/*
		 * NEEDSWORK: a utf8_strwidth variant that
		 * accepts a memory area not terminated by \0
		 * would avoid this ugliness.
		 */
		save = *next;
		*next = '\0';
		len += utf8_strwidth(s);
		*next = save;

POSIX does not provide a strwidth function, so if we want to handle
encodings like SHIFT-JIS then something uglier[1] might come to life.

> +static void layout(const struct column_layout *c,
> +		   int total_width, int padding,
> +		   int *width,
> +		   int *rows, int *cols)
> +{
[...]
> +	*rows = c->items.nr / *cols;
> +	if (c->items.nr > *rows * *cols)
> +		(*rows)++;

Maybe
	*rows = DIV_ROUND_UP(c->items.nr, *cols);
?

> +static int squeeze_columns(const struct column_layout *c,
> +			   int *width, int padding,
> +			   int rows, int cols)

What does this function do?

Ah, it is for computing smaller, unequal column widths.

> +{
> +	int x, y, i, len, item_len, spare = 0;
> +	const char *s;
> +
> +	for (x = 0; x < cols; x++) {

		/* Find minimum width for column. */
		int len = 0;

> +		for (y = len = 0; y < rows; y++) {
[...]
> +		len += padding;
> +		if (len < width[x]) {
> +			spare += width[x] - len;
> +			width[x] = len;
> +		}

This "if" is unnecessary, I hope.  A simple

		assert(len <= width[x]);

would check that.

[...]
> +static void relayout(const struct column_layout *c,
> +		     int padding, int spare,
> +		     int *initial_width, int **width,
> +		     int *rows, int *cols)
> +{
> +	int new_rows, new_cols, new_initial_width;
> +	int i, *new_width, new_spare, total_width;
> +
> +	/*
> +	 * Assume all columns have same width, we would need
> +	 * initial_width*cols. But then after squeezing, we have
> +	 * "spare" more chars. Assume a new total_width with
> +	 * additional chars, then re-squeeze to see if it fits
> +	 * c->width.
> +	 */

Might be easier to debug if this part were deferred to a separate
patch. :)

> +	total_width = (*initial_width)*(*cols) + spare;

An odd heuristic.  Does it work well in practice?

> +static void display_columns_first(const struct column_layout *c,
> +				  int padding, const char *indent)
> +{
[...]
> +	for (y = 0; y < rows; y++) {
> +		for (x = 0; x < cols; x++) {
[...]
> +			const char *s;
> +			int len;
> +
> +			i = x * rows + y;
> +			if (i >= c->items.nr)
> +				break;
> +			s = c->items.items[i].string;
> +			len = item_length(c, s);
> +			if (width[x] < initial_width)
> +				len += initial_width - width[x];

The "if" is unnecessary, I hope.  (With that hope being checkable
by assert(width[x] <= initial_width).)

> +
> +			printf("%s%s%s",
> +			       x == 0 ? indent : "",
> +			       c->items.items[i].string,
> +			       /* If the next column at same row is
> +				  out of range, end the line. Else pad
> +				  some space.  */
> +			       i + rows >= c->items.nr ? "\n" : empty_cell + len);

Nice.  The loop body could be a separate display_cell() function for
easier contemplation.

[...]
> +++ b/column.h
> @@ -0,0 +1,20 @@
[...]
> +#define COL_DENSE         (1 << 5) /* "Ragged-right" mode, relayout if enough space */

I might be confused, but would it not be clearer to call it "unequal
column width" mode?  In other words, is it about columns not all
having the same width as one another or about having a ragged right
margin?

Thanks for a pleasant read.
Jonathan

[1] Based on tuklib_mbstr_width:

	size_t remaining = strlen(s);
	size_t len = 0;
	mbstate_t state;

	memset(&state, 0, sizeof(state));
	while (remaining) {
		wchar_t ch;
		size_t nbytes;
		int width;

		if (!prefixcmp(s, "\033["))
			...
		nbytes = mbrtowc(&ch, s, remaining, &state);
		if (!nbytes || nbytes > remaining)
			... handle error ...
		s += nbytes;
		remaining -= nbytes;
		width = wcwidth(ch);
		if (width < 0)
			... handle nonprintable character ...
		len += width;
	}

Re: [PATCH 2/7] Add column layout

[Nguyen Thai Ngoc Duy]

2011/2/9 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> +++ b/column.c
>> @@ -0,0 +1,177 @@
> [...]
>> +static int item_length(const struct column_layout *c, const char *s)
>> +{
>> +     int a_len = 0;
>> +
>> +     if (!(c->mode & COL_ANSI))
>> +             return strlen(s);
>> +
>> +     while ((s = strstr(s, "\033[")) != NULL) {
>> +             int len = strspn(s+2, "0123456789;");
>> +             s += len+3; /* \033[<len><func char> */
>> +             a_len += len+3;
>> +     }
>> +     return a_len;
>> +}
>
> I think you mean "return strlen(orig_s) - a_len".
>
> Something like the following could be more obvious, though
> it is unfortunately verbose.
>
>        int len = 0;
>        while (*s) {
>                const char *next;
>
>                /* \033[<len><func char> */
>                if (!prefixcmp(s, "\033[")) {
>                        s += strlen("\033[");
>                        s += strspn(s, "0123456789;");
>                        if (!*s)
>                                ... handle somehow ...
>                        s++;
>                }
>
>                next = strchrnul(s, '\033');
>                len += next - s;
>                s = next;
>        }
>
> Both miscompute the width of "Dépôt".  Something like this can do ok
> if the string is modifiable and we know it is UTF-8:
>
>                char save;
>                ...
>                next = strchrnul(s, '\033');
>
>                /*
>                 * NEEDSWORK: a utf8_strwidth variant that
>                 * accepts a memory area not terminated by \0
>                 * would avoid this ugliness.
>                 */
>                save = *next;
>                *next = '\0';
>                len += utf8_strwidth(s);
>                *next = save;
>
> POSIX does not provide a strwidth function, so if we want to handle
> encodings like SHIFT-JIS then something uglier[1] might come to life.

I think UTF-8 is enough. Git does not produce complex ansi escape
codes. Perhaps checking them at the beginning and reset code at the
end is enough.

> [...]
>> +static void relayout(const struct column_layout *c,
>> +                  int padding, int spare,
>> +                  int *initial_width, int **width,
>> +                  int *rows, int *cols)
>> +{
>> +     int new_rows, new_cols, new_initial_width;
>> +     int i, *new_width, new_spare, total_width;
>> +
>> +     /*
>> +      * Assume all columns have same width, we would need
>> +      * initial_width*cols. But then after squeezing, we have
>> +      * "spare" more chars. Assume a new total_width with
>> +      * additional chars, then re-squeeze to see if it fits
>> +      * c->width.
>> +      */
>
> Might be easier to debug if this part were deferred to a separate
> patch. :)
>
>> +     total_width = (*initial_width)*(*cols) + spare;
>
> An odd heuristic.  Does it work well in practice?

It seems so. If it does not, I guess I would need to look into how GNU
ls does it.
-- 
Duy

[PATCH 3/7] parseopt: OPT_COLUMN to set struct column_layout.mode

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 column.c        |   18 ++++++++++++++++++
 column.h        |    3 +++
 parse-options.h |    2 ++
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/column.c b/column.c
index 4b92fa5..c4a90d5 100644
--- a/column.c
+++ b/column.c
@@ -175,3 +175,21 @@ void display_columns(const struct column_layout *c, int padding, const char *ind
 		die("BUG: invalid mode %d", c->mode & COL_MODE);
 	}
 }
+
+int parseopt_column_callback(const struct option *opt, const char *arg, int unset)
+{
+	struct column_layout *cp = opt->value;
+	if (unset) {
+		cp->mode = COL_PLAIN;
+		return 0;
+	}
+	if (!arg || !strcmp(arg, "column")) {
+		cp->mode |= COL_COLUMN_FIRST;
+		return 0;
+	}
+	if (!strcmp(arg, "dense")) {
+		cp->mode |= COL_DENSE;
+		return 0;
+	}
+	return error("unsupported style '%s'", arg);
+}
diff --git a/column.h b/column.h
index 34435b0..7f0ae74 100644
--- a/column.h
+++ b/column.h
@@ -17,4 +17,7 @@ struct column_layout {
 
 extern void display_columns(const struct column_layout *c, int padding, const char *indent);
 
+struct option;
+extern int parseopt_column_callback(const struct option *opt, const char *arg, int unset);
+
 #endif
diff --git a/parse-options.h b/parse-options.h
index 31ec5d2..2fed453 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -145,6 +145,8 @@ struct option {
 #define OPT_COLOR_FLAG(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), "when", (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }
+#define OPT_COLUMN(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "style", (h), PARSE_OPT_OPTARG, parseopt_column_callback }
 
 
 /* parse_options() will filter out the processed options and leave the
-- 
1.7.2.2

[PATCH 4/7] add core.column

[Nguyễn Thái Ngọc Duy]

This takes auto, never or always to enable column output for
porcelain commands.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |    8 ++++++++
 cache.h                  |    1 +
 config.c                 |   16 ++++++++++++++++
 environment.c            |    1 +
 4 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c5e1835..6d7151b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -567,6 +567,14 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.column::
+	Specify whether a command should output in columns. Only commands
+	that support `--column` will be affected by this. This variable can
+	take either `never` (do not output in columns), `auto` (output in
+	columns if the output is to a terminal) or `always` (always output in
+	columns). Setting `--column` or `--no-column` will override this
+	variable. This option defaults to never.
+
 add.ignore-errors::
 add.ignoreErrors::
 	Tells 'git add' to continue adding files when some files cannot be
diff --git a/cache.h b/cache.h
index bcbd5f2..1fa7656 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int core_column;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,
diff --git a/config.c b/config.c
index 625e051..071899f 100644
--- a/config.c
+++ b/config.c
@@ -9,6 +9,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 #include "quote.h"
+#include "color.h"
+#include "column.h"
 
 #define MAXNAME (256)
 
@@ -660,6 +662,20 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.column")) {
+		switch (git_config_colorbool(var, value, -1)) {
+		case 0:
+			core_column = COL_PLAIN;
+			break;
+		case 1:
+			core_column = COL_COLUMN_FIRST;
+			break;
+		case -1:
+			return -1;
+		}
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 9564475..b420ea2 100644
--- a/environment.c
+++ b/environment.c
@@ -56,6 +56,7 @@ char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
 struct startup_info *startup_info;
+int core_column;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
-- 
1.7.2.2

[PATCH 5/7] help: reuse struct column_layout

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 help.c |   33 +++++++++++----------------------
 1 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/help.c b/help.c
index 1344208..dc06f63 100644
--- a/help.c
+++ b/help.c
@@ -4,6 +4,7 @@
 #include "levenshtein.h"
 #include "help.h"
 #include "common-cmds.h"
+#include "column.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
@@ -72,29 +73,17 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
 
 static void pretty_print_string_list(struct cmdnames *cmds, int longest)
 {
-	int cols = 1, rows;
-	int space = longest + 1; /* min 1 SP between words */
-	int max_cols = term_columns() - 1; /* don't print *on* the edge */
-	int i, j;
-
-	if (space < max_cols)
-		cols = max_cols / space;
-	rows = DIV_ROUND_UP(cmds->cnt, cols);
+	struct column_layout c;
+	int i;
 
-	for (i = 0; i < rows; i++) {
-		printf("  ");
+	memset(&c, 0, sizeof(c));
+	c.mode = COL_COLUMN_FIRST;
+	c.width = term_columns();
 
-		for (j = 0; j < cols; j++) {
-			int n = j * rows + i;
-			int size = space;
-			if (n >= cmds->cnt)
-				break;
-			if (j == cols-1 || n + rows >= cmds->cnt)
-				size = 1;
-			printf("%-*s", size, cmds->names[n]->name);
-		}
-		putchar('\n');
-	}
+	for (i = 0; i < cmds->cnt; i++)
+		string_list_append(&c.items, cmds->names[i]->name);
+	display_columns(&c, 2, "  ");
+	string_list_clear(&c.items, 0);
 }
 
 static int is_executable(const char *name)
-- 
1.7.2.2

Re: [PATCH 5/7] help: reuse struct column_layout

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

[...]
> +++ b/help.c
> @@ -72,29 +73,17 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
[...]
> +	struct column_layout c;
> +	int i;
> +
> +	memset(&c, 0, sizeof(c));

Might be nice to have a COLUMN_LAYOUT_INIT, I suppose.

Looks sane.  What output changes would be noticeable, if any?

Re: [PATCH 5/7] help: reuse struct column_layout

[Nguyen Thai Ngoc Duy]

2011/2/9 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
> [...]
>> +++ b/help.c
>> @@ -72,29 +73,17 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
> [...]
>> +     struct column_layout c;
>> +     int i;
>> +
>> +     memset(&c, 0, sizeof(c));
>
> Might be nice to have a COLUMN_LAYOUT_INIT, I suppose.
>
> Looks sane.  What output changes would be noticeable, if any?

Two spaces between column instead of one. Will fix.
-- 
Duy

[PATCH 6/7] tag: support column output with --column

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-tag.txt |   11 ++++++++++-
 builtin/tag.c             |   19 ++++++++++++++++---
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 8b169e3..7a3ae28 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,8 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>]
+'git tag' [-n[<num>]] -l [--[no-]column[=<options>[,<option>]*]]
+	[--contains <commit>] [<pattern>]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -71,6 +72,14 @@ OPTIONS
 	List tags with names that match the given pattern (or all if no pattern is given).
 	Typing "git tag" without arguments, also lists all tags.
 
+--column::
+	Show tags in columns. This option is only applicable if `git tag` is
+	used to list tags without annotation lines.
+
+--no-column::
+	Show tags in a single list. This option is used to override core.columns
+	if set. This option is only applicable if `git tag` is used to list tags.
+
 --contains <commit>::
 	Only list tags which contain the specified commit.
 
diff --git a/builtin/tag.c b/builtin/tag.c
index aa1f87d..b74b8bf 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -12,6 +12,7 @@
 #include "tag.h"
 #include "run-command.h"
 #include "parse-options.h"
+#include "column.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
@@ -22,6 +23,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static char signingkey[1000];
+static struct column_layout layout;
 
 struct tag_filter {
 	const char *pattern;
@@ -52,7 +54,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 		}
 
 		if (!filter->lines) {
-			printf("%s\n", refname);
+			string_list_append(&layout.items, refname);
 			return 0;
 		}
 		printf("%-15s ", refname);
@@ -383,6 +385,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_STRING('u', NULL, &keyid, "key-id",
 					"use another key to sign the tag"),
 		OPT__FORCE(&force, "replace the tag if exists"),
+		OPT_COLUMN(0, "column", &layout, "show tag list in columns" ),
 
 		OPT_GROUP("Tag listing options"),
 		{
@@ -395,6 +398,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_tag_config, NULL);
+	layout.mode = core_column;
+	layout.width = term_columns();
+	layout.items.strdup_strings = 1;
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
@@ -413,9 +419,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	if (list + delete + verify > 1)
 		usage_with_options(git_tag_usage, options);
-	if (list)
-		return list_tags(argv[0], lines == -1 ? 0 : lines,
+	if (list) {
+		int ret;
+
+		if (lines != -1)
+			layout.mode = COL_PLAIN;
+		ret =  list_tags(argv[0], lines == -1 ? 0 : lines,
 				 with_commit);
+		display_columns(&layout, 2, "");
+		return ret;
+	}
 	if (lines != -1)
 		die("-n option is only allowed with -l.");
 	if (with_commit)
-- 
1.7.2.2

Re: [PATCH 6/7] tag: support column output with --column

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -52,7 +54,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>  		}
>  
>  		if (!filter->lines) {
> -			printf("%s\n", refname);
> +			string_list_append(&layout.items, refname);

Hmm, is this robbing streaming output from people who do not care about
columnar output?

I tend to agree with others who suggested you to port column to platforms
that lack it.

Re: [PATCH 6/7] tag: support column output with --column

[Nguyen Thai Ngoc Duy]

2011/2/10 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> @@ -52,7 +54,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>>               }
>>
>>               if (!filter->lines) {
>> -                     printf("%s\n", refname);
>> +                     string_list_append(&layout.items, refname);
>
> Hmm, is this robbing streaming output from people who do not care about
> columnar output?

Yes, but tag list is short enough streaming does not really matter. I
can check if column mode is active there, if it's not, just printf.

> I tend to agree with others who suggested you to port column to platforms
> that lack it.

The first version of this series created git-column and made a pipe to
git-column when column output is active. I'll resurrect the command
then. Original column does not understand ANSI escape codes for
coloring and mess up column output.
-- 
Duy

Re: [PATCH 6/7] tag: support column output with --column

[Miles Bader]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> Hmm, is this robbing streaming output from people who do not care about
>> columnar output?
>
> Yes, but tag list is short enough streaming does not really matter. I
> can check if column mode is active there, if it's not, just printf.

...but it's important to note, I think that in many projects the tag
list is at least long enough that columnar output would be hugely
useful...

-miles

-- 
Custard, n. A vile concoction produced by a malevolent conspiracy of the hen,
the cow, and the cook.

[PATCH 7/7] branch: support column output with --column

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-branch.txt |    8 ++++++++
 builtin/branch.c             |   22 +++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 9106d38..3ec997a 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
+	[--[no-]column[=<options>[,<option>]*]]
 	[-v [--abbrev=<length> | --no-abbrev]]
 	[(--merged | --no-merged | --contains) [<commit>]]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
@@ -99,6 +100,13 @@ OPTIONS
 	default to color output.
 	Same as `--color=never`.
 
+--column::
+	Show branches in columns. This option is ignored in verbose mode.
+
+--no-column::
+	Show branches in a single list. This option is used to override
+	core.columns if set.
+
 -r::
 	List or delete (if used with -d) the remote-tracking branches.
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 9e546e4..803fa5f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -15,6 +15,7 @@
 #include "branch.h"
 #include "diff.h"
 #include "revision.h"
+#include "column.h"
 
 static const char * const builtin_branch_usage[] = {
 	"git branch [options] [-r | -a] [--merged | --no-merged]",
@@ -53,6 +54,8 @@ static enum merge_filter {
 } merge_filter;
 static unsigned char merge_filter_ref[20];
 
+static struct column_layout layout;
+
 static int parse_branch_color_slot(const char *var, int ofs)
 {
 	if (!strcasecmp(var+ofs, "plain"))
@@ -451,7 +454,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		strbuf_release(&stat);
 		strbuf_release(&subject);
 	}
-	printf("%s\n", out.buf);
+	string_list_append(&layout.items, out.buf);
 	strbuf_release(&name);
 	strbuf_release(&out);
 }
@@ -660,6 +663,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
 			opt_parse_merge_filter, (intptr_t) "HEAD",
 		},
+		OPT_COLUMN(0, "column", &layout, "list branches in columns" ),
 		OPT_END(),
 	};
 
@@ -686,6 +690,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	}
 	hashcpy(merge_filter_ref, head_sha1);
 
+	memset(&layout, 0, sizeof(layout));
+	layout.mode = core_column;
+	layout.width = term_columns();
+	layout.items.strdup_strings = 1;
+
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 	if (!!delete + !!rename + !!force_create > 1)
@@ -693,8 +702,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	if (delete)
 		return delete_branches(argc, argv, delete > 1, kinds);
-	else if (argc == 0)
-		return print_ref_list(kinds, detached, verbose, abbrev, with_commit);
+	else if (argc == 0) {
+		int ret;
+		if (verbose)
+			layout.mode = COL_PLAIN;
+
+		ret = print_ref_list(kinds, detached, verbose, abbrev, with_commit);
+		display_columns(&layout, 2, "");
+		return ret;
+	}
 	else if (rename && (argc == 1))
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
-- 
1.7.2.2

Re: [PATCH/RFC 0/7] Column output

[Jeff King]

On Tue, Feb 08, 2011 at 10:22:14PM +0700, Nguyễn Thái Ngọc Duy wrote:

> In short, the series is very simple: give porcelain commands column
> output, just like good old "ls". There could be a few more candidates,
> I believe René Scharfe mentioned other files in "git status".

I don't really care for column output myself, so maybe you have a good
reason why my idea won't work. But why not use the BSD column program?
I.e.:

  git config pager.tag column
  git config pager.branch column

True it will use "column" no matter whether the specific branch or tag
operation produces a lot of output or not, but in general that should be
OK. Most "action" commands like "tag -d" or "branch new old" just
produce one line of output and look fine piped through column.

-Peff

Re: [PATCH/RFC 0/7] Column output

[Nguyen Thai Ngoc Duy]

2011/2/9 Jeff King <peff@peff.net>:
> On Tue, Feb 08, 2011 at 10:22:14PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> In short, the series is very simple: give porcelain commands column
>> output, just like good old "ls". There could be a few more candidates,
>> I believe René Scharfe mentioned other files in "git status".
>
> I don't really care for column output myself, so maybe you have a good
> reason why my idea won't work. But why not use the BSD column program?

Solaris did not have one last time I checked. Windows obviously does
not either, but I don't use msysgit so it does not count.
-- 
Duy

Re: [PATCH/RFC 0/7] Column output

[Jonathan Nieder]

Some general reflections.

Nguyen Thai Ngoc Duy wrote:
> 2011/2/9 Jeff King <peff@peff.net>:

>> I don't really care for column output myself,

Me neither.  I tricked myself into reading with a vague hope that it
might lead to a porcelain-level "git ls" command.

[...]
>> But why not use the BSD column program?
>
> Solaris did not have one last time I checked. Windows obviously does
> not either, but I don't use msysgit so it does not count.

A good reason to port the "column" program, no?  I admit I find this a
compelling excuse not to bother with getting --column options and
core.column configuration right.

All that said.

Patches 1, 2, and 5 replace git help's existing columnation code
with something more reusable and functional.  After those patches,

	git --paginate help -a

no longer forces 80-column output.  Another application of
term_columns could be to teach "git log" and "git log --graph"
to wrap to terminal width some day.

So if I ran the world, I would put those three patches before
--column et al in a possible reroll. :)

Thanks for some clear and pleasant reading.
Jonathan

Re: [PATCH/RFC 0/7] Column output

[Nguyen Thai Ngoc Duy]

On Wed, Feb 9, 2011 at 12:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Some general reflections.
>
> Nguyen Thai Ngoc Duy wrote:
>> 2011/2/9 Jeff King <peff@peff.net>:
>
>>> I don't really care for column output myself,
>
> Me neither.  I tricked myself into reading with a vague hope that it
> might lead to a porcelain-level "git ls" command.

Wait for it. I have something hackish but usable. Non-recursive,
colorized column output. Not full of feature as GNU ls though (for
example, sorting by date can't be implemented).
-- 
Duy