Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2019, 00:27 UTC
Message-ID: <xmqqo8ymg64c.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <79463ee5-a171-53a8-4903-84a825c4e8d8@gmail.com>

James Coglan <jcoglan@gmail.com> writes:

> - We don't want a general solution to this problem for everything
> `strbuf` could be used for; it only needs to address the graph
> padding problem.

Of course. Somebody may use strbuf to hold rows of a table and you do not want to contaminate strbuf with fields like width of each column etc, that are very specific to the application. IOW, strbuf is merely _one_ component of a larger solution to each specific problem, and the latter may be things like "struct graphbuf" like Dscho suggested, which might use strbuf as an underlying <byte-string, length> tuple mechanism, but that is an implementation detail that should not be exposed to the users of the struct (and that is why he did not call, and you should not call, the data structure "graph-strbuf" or anything with "strbuf").

> - We only want to count printing characters, not color formatting sequences.

OK. But I'd phrase "count printing characters" as "measure display width" for at least two reasons. Whitespaces are typically counted as non-printing, but you do want to take them into account for this application. Also the graph may not be limited to ASCII graphics forever, and byte- or character-count may not match display width on a fixed-width display.

> - We only need to consider the width of a small set of characters:
> { | / \ _ - . * }. We don't need to worry about Unicode, and the
> simple character counting in graph.c was working fine.

I have to warn you that we saw attempts to step outside these ASCII graphics and use Unicode characters for prettier output in the past. If you can do so without too much complexity, I'd suggest you try not to close the door to those people who follow your footsteps to further improve the system by pursuing the avenue further.

Show 8 quoted lines

> To this end I've prepared a different implementation that
> introduces the indirection described above, and does not modify
> `strbuf`:
>
> +struct graph_strbuf {
> +	struct strbuf *buf;
> +	size_t width;
> +};

Is there a reason why you need a pointer to a strbuf that is allocated separately? E.g. would it make it harder to manage if the above were

	struct graphbuf {
		struct strbuf buf;
		int width; /* display width in columns */
	};

which is essentially what Dscho suggested?

Show 17 quoted lines

> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
> +{
> +	strbuf_addch(sb->buf, c);
> +	sb->width++;
> +}
> +
> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
> +{
> +	strbuf_addchars(sb->buf, c, n);
> +	sb->width += n;
> +}
> +
> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char *s)
> +{
> +	strbuf_addstr(sb->buf, s);
> +	sb->width += strlen(s);
> +}

I'd probably introduce another helper that takes color code and graphbuf (also notice how I name the variables and types; calling something sb that is not a strbuf is misleading and inviting unnecessary bugs):

    static inline void graphbuf_addcolor(struct graphbuf *gb, unsigned short color)
    {
            strbuf_addstr(gb->buf, column_get_color_code(color));
    }

if I were writing this code. The goal is to make any strbuf_add*() that is done directly on gb->buf outside these helpers a bug--that way, grepping for strbuf_add* in this file would become a very easy way to catch such a bug that bypasses the graphbuf_add*() layer and potentially screw up the column counting.

Other than these three points (i.e. naming without "strbuf" that is an implementation detail, embedded vs pointer of strbuf in the graphbuf, and making sure everything can be done via graphbuf_add*() wrappers to make it easier to spot bugs), it seems you are going in the right direction. Thanks for working on this.