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

From: Denton Liu <liu.denton@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: James Coglan via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	James Coglan <jcoglan@gmail.com>
Subject: Re: [PATCH 01/11] graph: automatically track visible width of `strbuf`
Date: Thu, 10 Oct 2019 16:05:50 -0700	[thread overview]
Message-ID: <20191010230550.GA42541@generichostname> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1910102303330.46@tvgsbejvaqbjf.bet>

Hi Dscho,

On Thu, Oct 10, 2019 at 11:07:35PM +0200, Johannes Schindelin wrote:
> Hi James,
> 
> On Thu, 10 Oct 2019, James Coglan via GitGitGadget wrote:
> 
> > From: James Coglan <jcoglan@gmail.com>
> >
> > All the output functions in `graph.c` currently keep track of how many
> > printable chars they've written to the buffer, before calling
> > `graph_pad_horizontally()` to pad the line with spaces. Some functions
> > do this by incrementing a counter whenever they write to the buffer, and
> > others do it by encoding an assumption about how many chars are written,
> > as in:
> >
> >     graph_pad_horizontally(graph, sb, graph->num_columns * 2);
> >
> > This adds a fair amount of noise to the functions' logic and is easily
> > broken if one forgets to increment the right counter or update the
> > calculations used for padding.
> >
> > To make this easier to use, I'm adding a `width` field to `strbuf` that
> > tracks the number of printing characters added after the line prefix.
> 
> This is a big heavy-handed: adding a `width` field to `struct strbuf`
> and maintaining it _just_ for the purpose of `graph.c` puts an
> unnecssary load on every other `strbuf` user (of which there are a
> _lot_).
> 
> So my obvious question is: what makes `width` different from `len`?
> Since we exclusively use ASCII characters for the graph part, we should
> be able to use the already-existing `len`, for free, no?

From what I can gleam from looking at the code, `width` is different
from `len` because when we're printing with colours, there'll be a bunch
of termcodes that don't actually count for the width.

I think that we should either leave the `chars_written` variable as is
or maybe calculate it after the fact. Here's an untested and uncompiled
implementation of something that might do that:

	static int calculate_width(const struct strbuf *row)
	{
		int in_termcode = 0;
		int width = 0;
		int i;

		for (i = 0; i < row.len; i++) {
			if (row.buf[i] == '\033')
				in_termcode = 1;

			if (!in_termcode)
				width++;
			else if (row.buf[i] == 'm')
				in_termcode = 0;
		}
	}

If we include this, I'm not sure what kind of performance hit we might
take if the graph we're generating is particularly big, though.

> 
> I could imagine that the `strbuf` might receive more than one line, but
> then we still would only need to remember the offset of the last newline
> character in that `strbuf`, no?
> 
> Ciao,
> Johannes