Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()

From: Torsten Bögershausen <tboegi@web.de>
Date: Sep 26, 2022, 18:43 UTC
Message-ID: <20220926184308.5oaaoopod36igq6i@tb-raspi4>
In-Reply-To: <xmqqpmfx52qj.fsf@gitster.g>

On Wed, Sep 14, 2022 at 09:40:04AM -0700, Junio C Hamano wrote:

[]

Show 19 quoted lines

> I think I spotted two remaining "bugs" that are left unfixed with
> this patch..
>
> There is "stat_width is -1 (auto)" case, which reads like so:
>
> 	if (options->stat_width == -1)
> 		width = term_columns() - strlen(line_prefix);
> 	else
> 		width = options->stat_width ? options->stat_width : 80;
>
> Here line_prefix eventually comes from the "git log --graph" and
> shows the colored graph segments on the same output line as the
> diffstat.
>
> This patch is probably not making anything worse, but by leaving it
> strlen(), it is likely overcounting the width of it.  We can
> presumably use utf8_strnwidth() that can optionally be told to be
> aware of the ANSI color sequence to count its width correctly to fix
> it.

[]

> This is the other remaining bug.

[]

> I think the remainder of the patch I did not quote looked quite
> straight-forward and correct.
>
> Thanks for working on this topic.

How should we proceed here ? This patch fixes one, and only one, reported bug, which is now verfied by a test case using unicode instead of ASCII. Fixing additional bugs in diff.c (or anywhere else) had never been part of this.

Things that needs more fixing and cleanups had been layed out as the result of a review, that is good.

"git log --graph" was mentioned. Do we have test cases, that test this ? How easy are they converted into unicode instead of ASCII ?

I am not even sure, if I ever used "git log --graph" myself. Digging further here, is somewhat out of my scope. At least for the moment.