On 10/10/2019 7:05 PM, Denton Liu wrote:
> 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@xxxxxxxxx>
>>>
>>> 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_).
I was concerned about this, too.
>> 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.
This is worth trying. In terms of CPU time, this should be a mere
blip compared to all of the commit walking that is going on. (Unless
we get to thousands of columns, but by then the output is useless.)
Of course, we should measure the impact, but it is worth a try.
-Stolee
