threads / patch / 45543

patch, 3 partsRespect column.ui in commented status in git-commit

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

## tl;dr

9 messages between Mar 30, 2017 and Apr 10, 2017. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Stefan Beller· Mar 30, 2017, 01:42 UTC · lore

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
Stefan Beller· Mar 30, 2017, 01:42 UTC · re: Stefan Beller · lore

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

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

column.c, column.h

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
Jeff King· Mar 30, 2017, 03:12 UTC · re: Stefan Beller · lore

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

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.
Show 12 quoted lines
> 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
Duy Nguyen· Apr 10, 2017, 11:13 UTC · re: Stefan Beller · lore

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

On Wed, Mar 29, 2017 at 06:42:37PM -0700, Stefan Beller wrote:
Show 11 quoted lines
> 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.

Show 6 quoted lines
>  };
>  
>  struct option;
> -- 
> 2.12.2.511.g2abb8caf66
> 
Stefan Beller· Mar 30, 2017, 01:42 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +24 −6

builtin/commit.c, wt-status.c

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
Jeff King· Mar 30, 2017, 03:22 UTC · re: Stefan Beller · lore

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

On Wed, Mar 29, 2017 at 06:42:38PM -0700, Stefan Beller wrote:
Show 5 quoted lines
> 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
Stefan Beller· Mar 30, 2017, 18:53 UTC · re: Jeff King · lore

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

On Wed, Mar 29, 2017 at 8:22 PM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> 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
Duy Nguyen· Apr 10, 2017, 11:35 UTC · re: Stefan Beller · lore

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

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.

Show 85 quoted lines
> 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.

Show 6 quoted lines
>  	print_columns(&output, s->colopts, &copts);
>  	string_list_clear(&output, 0);
>  	strbuf_release(&buf);
> -- 
> 2.12.2.511.g2abb8caf66
> 
Stefan Beller· Mar 30, 2017, 01:42 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to column.c +6 −4
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

← back to recent threads