{"thread":{"id":"60950","subject":"[PATCH] diff.c: use utf8_strwidth() instead of strlen() for display width","startedAt":"2024-02-18T18:37:26Z","lastAt":"2024-02-19T07:01:33Z","messageCount":3,"participants":["Chandra Pratap via GitGitGadget","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488878","messageId":"pull.1668.git.1708281443289.gitgitgadget@gmail.com","threadId":"60950","inReplyTo":null,"subject":"[PATCH] diff.c: use utf8_strwidth() instead of strlen() for display width","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-18T18:37:23Z","receivedAt":"2024-02-18T18:37:26Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nSigned-off-by: Chandra Pratap \n---\n diff.c: use utf8_strwidth() instead of strlen() for display width\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1668%2FChand-ra%2Fdiff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1668/Chand-ra/diff-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1668\n\n diff.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ccfa1fca0d0..02d60af6749 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2712,13 +2712,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t * making the line longer than the maximum width.\n \t */\n \n-\t/*\n-\t * NEEDSWORK: line_prefix is often used for \"log --graph\" output\n-\t * and contains ANSI-colored string. utf8_strnwidth() should be\n-\t * used to correctly count the display width instead of strlen().\n-\t */\n \tif (options->stat_width == -1)\n-\t\twidth = term_columns() - strlen(line_prefix);\n+\t\twidth = term_columns() - utf8_strwidth(line_prefix);\n \telse\n \t\twidth = options->stat_width ? options->stat_width : 80;\n \tnumber_width = decimal_width(max_change) > number_width ?\n\nbase-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90\n-- \ngitgitgadget\n"},{"id":"488904","messageId":"ZdLzxYpY-klokgpI@tanuki","threadId":"60950","inReplyTo":"pull.1668.git.1708281443289.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff.c: use utf8_strwidth() instead of strlen() for display width","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T06:23:01Z","receivedAt":"2024-02-19T06:23:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 18, 2024 at 06:37:23PM +0000, Chandra Pratap via GitGitGadget wrote:\n> From: Chandra Pratap \n> \n> Signed-off-by: Chandra Pratap \n> ---\n> diff.c: use utf8_strwidth() instead of strlen() for display width\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1668%2FChand-ra%2Fdiff-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1668/Chand-ra/diff-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1668\n> \n> diff.c | 7 +------\n> 1 file changed, 1 insertion(+), 6 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index ccfa1fca0d0..02d60af6749 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2712,13 +2712,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t * making the line longer than the maximum width.\n> \t */\n> \n> -\t/*\n> -\t * NEEDSWORK: line_prefix is often used for \"log --graph\" output\n> -\t * and contains ANSI-colored string. utf8_strnwidth() should be\n> -\t * used to correctly count the display width instead of strlen().\n> -\t */\n> \tif (options->stat_width == -1)\n> -\t\twidth = term_columns() - strlen(line_prefix);\n> +\t\twidth = term_columns() - utf8_strwidth(line_prefix);\n\nIt would be nice to add a test demonstrating that this indeed fixes an\nissue. This would also help to keep this from regressing in the future.\n\nAlso, do you know why we didn't use `utf8_strwidth()` right from the\nstart? It would have saved the writer some time to just use\n`utf8_strwidth()` instead of writing a whole paragraph explaining that\nwe should do it eventually. Makes me wonder whether there is anything\nelse going on here.\n\nPatrick\n\n> \telse\n> \t\twidth = options->stat_width ? options->stat_width : 80;\n> \tnumber_width = decimal_width(max_change) > number_width ?\n> \n> base-commit: 2996f11c1d11ab68823f0939b6469dedc2b9ab90\n> -- \n> gitgitgadget\n> \n"},{"id":"488905","messageId":"xmqqh6i5ey4b.fsf@gitster.g","threadId":"60950","inReplyTo":"ZdLzxYpY-klokgpI@tanuki","subject":"Re: [PATCH] diff.c: use utf8_strwidth() instead of strlen() for display width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T07:01:24Z","receivedAt":"2024-02-19T07:01:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Also, do you know why we didn't use `utf8_strwidth()` right from the\n> start? It would have saved the writer some time to just use\n> `utf8_strwidth()` instead of writing a whole paragraph explaining that\n> we should do it eventually. Makes me wonder whether there is anything\n> else going on here.\n\nI suspect that it is because it is not just that single strlen()\ncall. The code assumed that byte count and display width were\ninterchangeable, and use of strlen() there was merely an example.\nStarting there, the value returned by strlen() is treated as if it\nwere interchangeable with a display width, and then later used to\ncount how many bytes to trim from either end of the string so that\nthe trimmed string would eventually fit a given display width, which\nmeans that the code to compute how much to trim (which is not that\nstrlen() the patch in question is touching) would have to compute\nthe reverse, i.e. \"if we need to recover N display columns, how many\nbytes do we need to trim from that string?\"\n\n\n"}]}