[PATCH 0/3] Respect column.ui in commented status in git-commit

[PATCH 0/3] Respect column.ui in commented status in git-commit

[Stefan Beller]

When studying the code, I was nerd-sniped by the commit message of
4d2292e9a9 (status: refactor colopts handling, 2012-05-07)
as I did not understand why it was so important to reset the s.colopts to 0
in builtin/commit.c.

In my adolescent hybris I nearly sent out a patch claiming that line to be
useless and wrong, but then I studied a bit more. After the background story
became clear, I decided to "just write the missing piece", how hard can it be?

I would consider the following three patches a hack, but they work. You can
have untracked files in column mode in the commented text for a commit.

Thanks,
Stefan

Stefan Beller (3):
  column.c: pass column_options to down to display_plain
  column: allow for custom printf
  WIP - Allow custom printf function for column printing

 builtin/commit.c |  1 -
 column.c         | 21 +++++++++++++--------
 column.h         |  3 +++
 wt-status.c      | 29 ++++++++++++++++++++++++-----
 4 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.12.2.511.g2abb8caf66

[PATCH 1/3] column.c: pass column_options to down to display_plain

[Stefan Beller]

In a later patch we want to use more of the column_options members at
places, where we do actual output, so it will be handy to have the whole
struct around in `display_plain`.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 column.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/column.c b/column.c
index d55ead18ef..4851b9aa04 100644
--- a/column.c
+++ b/column.c
@@ -109,12 +109,12 @@ static void shrink_columns(struct column_data *data)
 
 /* Display without layout when not enabled */
 static void display_plain(const struct string_list *list,
-			  const char *indent, const char *nl)
+			  const struct column_options *opts)
 {
 	int i;
 
 	for (i = 0; i < list->nr; i++)
-		printf("%s%s%s", indent, list->items[i].string, nl);
+		printf("%s%s%s", opts->indent, list->items[i].string, opts->nl);
 }
 
 /* Print a cell to stdout with all necessary leading/traling space */
@@ -201,12 +201,14 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 	nopts.padding = opts ? opts->padding : 1;
 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
 	if (!column_active(colopts)) {
-		display_plain(list, "", "\n");
+		nopts.indent = "";
+		nopts.nl = "\n";
+		display_plain(list, &nopts);
 		return;
 	}
 	switch (COL_LAYOUT(colopts)) {
 	case COL_PLAIN:
-		display_plain(list, nopts.indent, nopts.nl);
+		display_plain(list, &nopts);
 		break;
 	case COL_ROW:
 	case COL_COLUMN:
-- 
2.12.2.511.g2abb8caf66

[PATCH 2/3] column: allow for custom printf

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 column.c | 13 ++++++++-----
 column.h |  3 +++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/column.c b/column.c
index 4851b9aa04..522e554f29 100644
--- a/column.c
+++ b/column.c
@@ -114,7 +114,9 @@ static void display_plain(const struct string_list *list,
 	int i;
 
 	for (i = 0; i < list->nr; i++)
-		printf("%s%s%s", opts->indent, list->items[i].string, opts->nl);
+		opts->_printf("%s%s%s", opts->indent,
+					list->items[i].string,
+					opts->nl);
 }
 
 /* Print a cell to stdout with all necessary leading/traling space */
@@ -143,10 +145,10 @@ static int display_cell(struct column_data *data, int initial_width,
 	else
 		newline = x == data->cols - 1 || i == data->list->nr - 1;
 
-	printf("%s%s%s",
-	       x == 0 ? data->opts.indent : "",
-	       data->list->items[i].string,
-	       newline ? data->opts.nl : empty_cell + len);
+	data->opts._printf("%s%s%s",
+			   x == 0 ? data->opts.indent : "",
+			   data->list->items[i].string,
+			   newline ? data->opts.nl : empty_cell + len);
 	return 0;
 }
 
@@ -200,6 +202,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
 	nopts.padding = opts ? opts->padding : 1;
 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
+	nopts._printf = opts && opts->_printf ? opts->_printf : printf;
 	if (!column_active(colopts)) {
 		nopts.indent = "";
 		nopts.nl = "\n";
diff --git a/column.h b/column.h
index 0a61917fa7..c44a1525a9 100644
--- a/column.h
+++ b/column.h
@@ -24,6 +24,9 @@ struct column_options {
 	int padding;
 	const char *indent;
 	const char *nl;
+
+	/* when non-NULL, use this printing function, fallback to printf */
+	int (*_printf)(const char *__format, ...);
 };
 
 struct option;
-- 
2.12.2.511.g2abb8caf66

[PATCH 3/3] WIP - Allow custom printf function for column printing

[Stefan Beller]

Ever wondered why column.ui applies the untracked files in git-status,
but not for the help text comment in git-commit? Nobody wrote the code!

This is marked as WIP, as it barely demonstrates how the code may look
like. No tests, no documentation.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/commit.c |  1 -
 wt-status.c      | 29 ++++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..f482150df8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1649,7 +1649,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	status_init_config(&s, git_commit_config);
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
-	s.colopts = 0;
 
 	if (get_sha1("HEAD", oid.hash))
 		current_head = NULL;
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..cfba352683 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -43,12 +43,13 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static int status_vprintf(struct wt_status *s, int at_bol, const char *color,
 		const char *fmt, va_list ap, const char *trail)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf linebuf = STRBUF_INIT;
 	const char *line, *eol;
+	int ret = 0;
 
 	strbuf_vaddf(&sb, fmt, ap);
 	if (!sb.len) {
@@ -59,9 +60,9 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 		}
 		color_print_strbuf(s->fp, color, &sb);
 		if (trail)
-			fprintf(s->fp, "%s", trail);
+			ret += fprintf(s->fp, "%s", trail);
 		strbuf_release(&sb);
-		return;
+		return ret;
 	}
 	for (line = sb.buf; *line; line = eol + 1) {
 		eol = strchr(line, '\n');
@@ -78,15 +79,16 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 			strbuf_addstr(&linebuf, line);
 		color_print_strbuf(s->fp, color, &linebuf);
 		if (eol)
-			fprintf(s->fp, "\n");
+			ret += fprintf(s->fp, "\n");
 		else
 			break;
 		at_bol = 1;
 	}
 	if (trail)
-		fprintf(s->fp, "%s", trail);
+		ret += fprintf(s->fp, "%s", trail);
 	strbuf_release(&linebuf);
 	strbuf_release(&sb);
+	return ret;
 }
 
 void status_printf_ln(struct wt_status *s, const char *color,
@@ -834,6 +836,20 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
 	strbuf_release(&summary);
 }
 
+static struct wt_status *global_wt_status_hack;
+static int column_status_printf(const char *fmt, ...)
+{
+	va_list ap;
+	struct wt_status *s = global_wt_status_hack;
+	int ret;
+
+	va_start(ap, fmt);
+	ret = status_vprintf(s, 0, "", fmt, ap, NULL);
+	va_end(ap);
+
+	return ret;
+}
+
 static void wt_longstatus_print_other(struct wt_status *s,
 				      struct string_list *l,
 				      const char *what,
@@ -856,6 +872,7 @@ static void wt_longstatus_print_other(struct wt_status *s,
 		path = quote_path(it->string, s->prefix, &buf);
 		if (column_active(s->colopts)) {
 			string_list_append(&output, path);
+			global_wt_status_hack = s;
 			continue;
 		}
 		status_printf(s, color(WT_STATUS_HEADER, s), "\t");
@@ -876,6 +893,8 @@ static void wt_longstatus_print_other(struct wt_status *s,
 	copts.indent = buf.buf;
 	if (want_color(s->use_color))
 		copts.nl = GIT_COLOR_RESET "\n";
+
+	copts._printf = column_status_printf;
 	print_columns(&output, s->colopts, &copts);
 	string_list_clear(&output, 0);
 	strbuf_release(&buf);
-- 
2.12.2.511.g2abb8caf66

Re: [PATCH 2/3] column: allow for custom printf

[Jeff King]

On Wed, Mar 29, 2017 at 06:42:37PM -0700, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>

No justification?

I assume it will be used in a future patch.

> diff --git a/column.h b/column.h
> index 0a61917fa7..c44a1525a9 100644
> --- a/column.h
> +++ b/column.h
> @@ -24,6 +24,9 @@ struct column_options {
>  	int padding;
>  	const char *indent;
>  	const char *nl;
> +
> +	/* when non-NULL, use this printing function, fallback to printf */
> +	int (*_printf)(const char *__format, ...);
>  };

Avoid names with leading underscores. They're reserved by the C
standard.

I wonder if gcc is smart enough to let us mark this function pointer
with a "format" attribute so we can get compile-time checking of the
format string.

-Peff

Re: [PATCH 3/3] WIP - Allow custom printf function for column printing

[Jeff King]

On Wed, Mar 29, 2017 at 06:42:38PM -0700, Stefan Beller wrote:

> Ever wondered why column.ui applies the untracked files in git-status,
> but not for the help text comment in git-commit? Nobody wrote the code!
> 
> This is marked as WIP, as it barely demonstrates how the code may look
> like. No tests, no documentation.

I'm confused about what this patch is trying to do. Is it just to turn
on column.status support for the template shown by git-commit? Or is it
columnizing more than the untracked files list?

If the former, why isn't just setting s.colopts enough? I guess because
we write into a strbuf?

If the latter, then isn't that a separate logical patch from "should
commit use columns"?

I don't have an opinion on the whole thing myself, as I do not use
columns nor really know how they are supposed to work. You found my
4d2292e9a9, but I was explicitly trying _not_ to get involved in any
behavior changes there due to my cluelessness. :)

I think Duy, who wrote all of the column code, is a better person to cc.

-Peff

Re: [PATCH 3/3] WIP - Allow custom printf function for column printing

[Stefan Beller]

On Wed, Mar 29, 2017 at 8:22 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 29, 2017 at 06:42:38PM -0700, Stefan Beller wrote:
>
>> Ever wondered why column.ui applies the untracked files in git-status,
>> but not for the help text comment in git-commit? Nobody wrote the code!
>>
>> This is marked as WIP, as it barely demonstrates how the code may look
>> like. No tests, no documentation.
>
> I'm confused about what this patch is trying to do. Is it just to turn
> on column.status support for the template shown by git-commit? Or is it
> columnizing more than the untracked files list?

It is just columnizing the untracked files list.

>
> If the former, why isn't just setting s.colopts enough? I guess because
> we write into a strbuf?

Yes, because it is a strbuf, and not stdout. the column code could only write
to stdout (which was solved in prior patches), but we still have to provide a
"printf like" function, which is the majority of this patch.

The remaining part of the patch, which could go into a separate patch
is the removal of:
-       s.colopts = 0;

> If the latter, then isn't that a separate logical patch from "should
> commit use columns"?

yes, I was too quick there.


> I don't have an opinion on the whole thing myself, as I do not use
> columns nor really know how they are supposed to work. You found my
> 4d2292e9a9, but I was explicitly trying _not_ to get involved in any
> behavior changes there due to my cluelessness. :)

eh, ok. I thought you had deeper reasons than that.
I did not use columns either, but now it looks like I am
a new user of that feature. If only Git would users easily
discover cool features. ;)

>
> I think Duy, who wrote all of the column code, is a better person to cc.
>
> -Peff

Re: [PATCH 2/3] column: allow for custom printf

[Duy Nguyen]

On Wed, Mar 29, 2017 at 06:42:37PM -0700, Stefan Beller wrote:
> diff --git a/column.h b/column.h
> index 0a61917fa7..c44a1525a9 100644
> --- a/column.h
> +++ b/column.h
> @@ -24,6 +24,9 @@ struct column_options {
>  	int padding;
>  	const char *indent;
>  	const char *nl;
> +
> +	/* when non-NULL, use this printing function, fallback to printf */
> +	int (*_printf)(const char *__format, ...);

This function should take 'struct column_options *' as the first
argument so a future implementation have full information. Yes we need
to add a couple more lines for just wrapping plain printf, but I think
it's worth it.

>  };
>  
>  struct option;
> -- 
> 2.12.2.511.g2abb8caf66
> 

Re: [PATCH 3/3] WIP - Allow custom printf function for column printing

[Duy Nguyen]

On Wed, Mar 29, 2017 at 06:42:38PM -0700, Stefan Beller wrote:
> Ever wondered why column.ui applies the untracked files in git-status,
> but not for the help text comment in git-commit? Nobody wrote the code!

How do you decide text width for this help text? If the output is
terminal, we know how wide it is.

But editors are different animal. We might use terminal width for
text-based editors, but we won't even know if it's text or gui
based. Or should we just go with 72 columns? I don't think we even
enforce that anywhere now.

> diff --git a/wt-status.c b/wt-status.c
> index 308cf3779e..cfba352683 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -43,12 +43,13 @@ static const char *color(int slot, struct wt_status *s)
>  	return c;
>  }
>  
> -static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
> +static int status_vprintf(struct wt_status *s, int at_bol, const char *color,
>  		const char *fmt, va_list ap, const char *trail)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf linebuf = STRBUF_INIT;
>  	const char *line, *eol;
> +	int ret = 0;
>  
>  	strbuf_vaddf(&sb, fmt, ap);
>  	if (!sb.len) {
> @@ -59,9 +60,9 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
>  		}
>  		color_print_strbuf(s->fp, color, &sb);
>  		if (trail)
> -			fprintf(s->fp, "%s", trail);
> +			ret += fprintf(s->fp, "%s", trail);
>  		strbuf_release(&sb);
> -		return;
> +		return ret;
>  	}
>  	for (line = sb.buf; *line; line = eol + 1) {
>  		eol = strchr(line, '\n');
> @@ -78,15 +79,16 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
>  			strbuf_addstr(&linebuf, line);
>  		color_print_strbuf(s->fp, color, &linebuf);
>  		if (eol)
> -			fprintf(s->fp, "\n");
> +			ret += fprintf(s->fp, "\n");
>  		else
>  			break;
>  		at_bol = 1;
>  	}
>  	if (trail)
> -		fprintf(s->fp, "%s", trail);
> +		ret += fprintf(s->fp, "%s", trail);
>  	strbuf_release(&linebuf);
>  	strbuf_release(&sb);
> +	return ret;
>  }
>  
>  void status_printf_ln(struct wt_status *s, const char *color,
> @@ -834,6 +836,20 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
>  	strbuf_release(&summary);
>  }
>  
> +static struct wt_status *global_wt_status_hack;
> +static int column_status_printf(const char *fmt, ...)
> +{
> +	va_list ap;
> +	struct wt_status *s = global_wt_status_hack;
> +	int ret;
> +
> +	va_start(ap, fmt);
> +	ret = status_vprintf(s, 0, "", fmt, ap, NULL);
> +	va_end(ap);
> +
> +	return ret;
> +}
> +
>  static void wt_longstatus_print_other(struct wt_status *s,
>  				      struct string_list *l,
>  				      const char *what,
> @@ -856,6 +872,7 @@ static void wt_longstatus_print_other(struct wt_status *s,
>  		path = quote_path(it->string, s->prefix, &buf);
>  		if (column_active(s->colopts)) {
>  			string_list_append(&output, path);
> +			global_wt_status_hack = s;
>  			continue;
>  		}
>  		status_printf(s, color(WT_STATUS_HEADER, s), "\t");
> @@ -876,6 +893,8 @@ static void wt_longstatus_print_other(struct wt_status *s,
>  	copts.indent = buf.buf;
>  	if (want_color(s->use_color))
>  		copts.nl = GIT_COLOR_RESET "\n";
> +
> +	copts._printf = column_status_printf;

This is kinda ugly. I think status_vprintf() can handle multi-line
strings well. In that case maybe you just need to make
strbuf_print_columns() return a strbuf instead, then pass the entire
string to one status_vprintf() call.

There isn't need to abstract the printf() calls in column.c. Just
change it unconditionally to strbuf_addf() and print the whole thing
out to stdout in the end in print_coluns(), or pass the strbuf back
with strbuf_print_columns().

The small delay before printing the first line (because we know buffer
everything in strbuf first) won't be an issue because we already have
to queue all items for layout before we can print anything anyway.

>  	print_columns(&output, s->colopts, &copts);
>  	string_list_clear(&output, 0);
>  	strbuf_release(&buf);
> -- 
> 2.12.2.511.g2abb8caf66
>