Re: [RFC PATCH v2 1/6] status: add noob format from status.noob config

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2023, 01:32 UTC
Message-ID: <xmqqjzr4gaie.fsf@gitster.g>
In-Reply-To: <20231026224615.675172-2-jacob@initialcommit.io>

Jacob Stopak <jacob@initialcommit.io> writes:

Show 5 quoted lines

> diff --git a/table.c b/table.c
> new file mode 100644
> index 0000000000..15600e117f
> --- /dev/null
> +++ b/table.c

Yuck, do we need an entirely new file? What trait are the things that are thrown into this file together supposed to share [*]? It is not very clear to me what the focus of this file is.

	Side note: for example, stuff in wt-status.c are to compute
	per-path status of the working tree and in-index files.

> @@ -0,0 +1,117 @@
> +#define USE_THE_INDEX_VARIABLE

I personally do not mind, but I suspect many people hate to see this compatibility set of macros used in a newly written source file.

Show 9 quoted lines

> +static const char *color(int slot, struct wt_status *s)
> +{
> +	const char *c = "";
> +	if (want_color(s->use_color))
> +		c = s->color_palette[slot];
> +	if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
> +		c = s->color_palette[WT_STATUS_HEADER];
> +	return c;
> +}

Do we need to duplicate this from other files? If this is about "git status", perhaps some parts of this patch, the truly new things (rather than what was copied, like this one) can be added to wt-status.c instead of adding a new file with unclear focus?

Show 5 quoted lines

> +static void build_table_border(struct strbuf *buf, int cols)
> +{
> +	strbuf_reset(buf);
> +	strbuf_addchars(buf, '-', cols);
> +}

This seems to be horizontal border; do we need a separate vertical border?

Show 12 quoted lines

> +static void build_table_entry(struct strbuf *buf, char *entry, int cols)
> +{
> +	strbuf_reset(buf);
> +	strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> +	strbuf_addstr(buf, entry);
> +
> +	/* Bump right padding if entry length is odd */
> +	if (!(strlen(entry) % 2))
> +		strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2 + 1);
> +	else
> +		strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> +}

The code assumes that one byte in string "entry" occupies one and only one display columns, which is so 20th centry assumption that does not care about i18n. Often what takes 3 bytes in a UTF-8 string occupies 2 display columns, for example. In addition, if you plan to color entries in the table, some substring would end up to be 0-width. Your pathname may be so long that 1/3 of a window width may not be sufficient to show it in its entirety, you might need to show it truncated in the middle.

utf8.c has support for measuring the display width of UTF-8 string, which is used elsewhere in our code. You may want to study it if you want to do a "tabular" output. The code in diff.c that shows diffstat has many gems to help what this code wants to do, including measuring display columns of a string, chomping a long string to fit in a desired display columns, etc., by using helpers defined in utf8.c

A potential excuse I can think of to have these outside wt-status.c and in a separate new file is to have a generic "table" layout machinery that is independent from "git status" or what each column of the table is showing (in other words, they may not be pathnames), and reusable by other subcommands that want to show things in the "table" layout. But even as a candidate for such a generic table mechanism, the above falls far short by hardcoding that it can only show 3-col table whose columns are evenly distributed and nothing else.

Show 10 quoted lines

> +static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
> +{
> +	printf(_("|"));
> +	color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
> +	printf(_("|"));
> +	color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
> +	printf(_("|"));
> +	color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
> +	printf(_("|\n"));
> +}

How does the code deal with unknown display width of translated version of "|" emitted here? Are you assuming that no matter how these are translated, they will always occupy one display column each?

Show 13 quoted lines

> +void print_noob_status(struct wt_status *s)
> +{
> +	struct winsize w;
> +	int cols;
> +	struct strbuf table_border = STRBUF_INIT;
> +	struct strbuf table_col_entry_1 = STRBUF_INIT;
> +	struct strbuf table_col_entry_2 = STRBUF_INIT;
> +	struct strbuf table_col_entry_3 = STRBUF_INIT;
> +	struct string_list_item *item;
> +
> +	/* Get terminal width */
> +	ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
> +	cols = w.ws_col;

Let's not reinvent an incomplete solution before studying and finding out what we already have in our codebase. Immediately after you got tempted to type TIOCGWINSZ, you can "git grep" the codebase for that particular constant to see if we already use it, as that is one very reasonable way to achieve what this piece of code wants to do (i.e. find out what the display width would be). You'd find pager.c:term_columns() and also learned that we want to prepare for the case where the ioctl() is not available.

> +	build_table_entry(&table_col_entry_1, "Untracked files", cols);
> +	build_table_entry(&table_col_entry_2, "Unstaged changes", cols);
> +	build_table_entry(&table_col_entry_3, "Staging area", cols);

Shouldn't these three strings be translatable?

What shoudl happen when these labels are wider than cols/3?

Show 12 quoted lines

> diff --git a/table.h b/table.h
> new file mode 100644
> index 0000000000..c9e8c386de
> --- /dev/null
> +++ b/table.h
> @@ -0,0 +1,6 @@
> +#ifndef TABLE_H
> +#define TABLE_H
> +
> +void print_noob_status(struct wt_status *s);
> +
> +#endif /* TABLE_H */

I am guessing that your plan is to add other "distim_noob_add()" and other "noob" variant of operations for various Git subcommands here, but I really do not think you want to add table.[ch] that has logic for such random set of Git subcommands copied and tweaked from all over the place, as the only trait being shared among them will become "they are written by Jacob Stopak", that is not a very useful grouping of the functions. It is not even "this file collects all the code that produce tabular output from Git"---"git status -s" already gives tabular output, for example, without using any of the "I only want to draw a table with three columns of equal width" logic. Adding code that are necessary to add yet another output mode for "git status" directly to where various output modes of "git status" are implemented, i.e. wt-status.c, and do similar changes for each command would make more sense, I would think.

Thanks.