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

["Torsten Bögershausen" <tboegi@web.de>]

On Fri, Oct 21, 2022 at 02:59:09PM -0700, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > For the moment I don't have any spare time to spend on Git.
> > All your comments are noted, and I hope to get time to address them later.
> > If you kick out the branch from seen and the whats cooking list,
> > that would be fine with me.
>
> I'd rather not waste the efforts so far.  I am tempted to queue the
> following on top or squash it in.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] diff: leave NEEDWORK notes in show_stats() function
>
> The previous step made an attempt to correctly compute display
> columns allocated and padded different parts of diffstat output.
> There are at least two known codepaths in the function that still
> mixes up display widths and byte length that need to be fixed.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index 2751cae131..1d222d87b2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2675,6 +2675,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	 * making the line longer than the maximum width.
>  	 */
>
> +	/*
> +	 * NEEDSWORK: line_prefix is often used for "log --graph" output
> +	 * and contains ANSI-colored string.  utf8_strnwidth() should be
> +	 * used to correctly count the display width instead of strlen().
> +	 */
>  	if (options->stat_width == -1)
>  		width = term_columns() - strlen(line_prefix);
>  	else
> @@ -2750,6 +2755,16 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  			char *slash;
>  			prefix = "...";
>  			len -= 3;
> +			/*
> +			 * NEEDSWORK: (name_len - len) counts the display
> +			 * width, which would be shorter than the byte
> +			 * length of the corresponding substring.
> +			 * Advancing "name" by that number of bytes does
> +			 * *NOT* skip over that many columns, so it is
> +			 * very likely that chomping the pathname at the
> +			 * slash we will find starting from "name" will
> +			 * leave the resulting string still too long.
> +			 */
>  			name += name_len - len;
>  			slash = strchr(name, '/');
>  			if (slash)


That looks good to me -
my preferred version would be a patch on it's own on top.