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

> Jacob Stopak <jacob@initialcommit.io> writes:
> 
> > 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.

> > +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?

> > +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?

> > +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.

> 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.  

> > +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?

> > +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?

> > 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.