{"thread":{"id":"58282","subject":"[BUG] Unicode filenames handling in `git log --stat`","startedAt":"2022-08-09T13:12:26Z","lastAt":"2022-10-23T20:07:39Z","messageCount":42,"participants":["Alexander Meshcheryakov","Calvin Wan","Junio C Hamano","Torsten Bögershausen","tboegi@web.de","Eric Sunshine","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"460911","messageId":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","threadId":"58282","inReplyTo":null,"subject":"[BUG] Unicode filenames handling in `git log --stat`","fromName":"Alexander Meshcheryakov","fromEmail":"alexander.s.m@gmail.com","sentAt":"2022-08-09T13:11:44Z","receivedAt":"2022-08-09T13:12:26Z","isPatch":false,"sender":{"key":"alexander.s.m@gmail.com","avatar":"https://gravatar.com/avatar/b746bedd5dfb858b4b10909a07c7db58e3c23507abde0931efcf3e5c34f875ba?d=mp&s=160"},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\ntouch Kyiv.txt Odesa.txt\ngit add -A\ngit commit -m 'Proper column widths'\ntouch Київ.txt Одеса.txt\ngit add -A\ngit commit -m 'Improper unicode width'\ngit log --stat\n\nWhat did you expect to happen? (Expected behavior)\nStats column for added/removed lines should be properly aligned.\n\nWhat happened instead? (Actual behavior)\nOnly changes for ASCII filenames are properly aligned.\nHere is how stats look for ASCII filenames:\nKyiv.txt | 0\nOdesa.txt | 0\n\nCompare with unicode filenames. I change actual letters to ASCII X to\navoid the issue in letter formatting:\nXXXX.txt | 0\nXXXXX.txt | 0\n\nWhat's different between what you expected and what actually happened?\nSee above\n\nAnything else you want to add:\nLooks like width of unicode strings is incorrectly calculated when\nformatting log --stat output. It considers number of bytes as number\nof characters in the string, but this is not correct for unicode\nstrings.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.34.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.0-40-generic #43-Ubuntu SMP Wed Jun 15 12:54:21 UTC\n2022 x86_64\ncompiler info: gnuc: 11.2\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"460929","messageId":"20220809182045.568598-1-calvinwan@google.com","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-08-09T18:20:44Z","receivedAt":"2022-08-09T18:47:56Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Hi Alexander,\n\nThank you for the report! I attempted to reproduce with the steps you\nprovided, but was unable to do so. What commands would I have to run\non a clean git repository to reproduce this?\n\n- Calvin\n"},{"id":"460946","messageId":"CA+VDVVVQQ4=um_L_h=EQASPHD_oYjwZxecYewLrCAbQV_m4hwQ@mail.gmail.com","threadId":"58282","inReplyTo":"20220809182045.568598-1-calvinwan@google.com","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Alexander Meshcheryakov","fromEmail":"alexander.s.m@gmail.com","sentAt":"2022-08-09T19:03:17Z","receivedAt":"2022-08-09T19:12:39Z","isPatch":false,"sender":{"key":"alexander.s.m@gmail.com","avatar":"https://gravatar.com/avatar/b746bedd5dfb858b4b10909a07c7db58e3c23507abde0931efcf3e5c34f875ba?d=mp&s=160"},"body":"Hi Calvin,\n\nSure, let me demonstrate with clean git repo:\n\nmkdir git_test; cd git_test\ngit init\ntouch Київ.txt Kyiv.txt Маріуполь.txt Mariupol.txt\ngit add -A\ngit commit -m 'foobar'\n\nNow let's check with GNU awk `git log --stat` strings width in bytes:\n$ git log --stat | LC_ALL=C awk '/txt/{print length($0), $0}'\n27 Kyiv.txt | 0\n27 Mariupol.txt | 0\n27 Київ.txt | 0\n27 Маріуполь.txt | 0\n\nAnd strings width in unicode characters:\n$ git log --stat | LC_ALL=en_US.UTF-8 awk '/txt/{print length($0), $0}'\n27 Kyiv.txt | 0\n27 Mariupol.txt | 0\n23 Київ.txt | 0\n18 Маріуполь.txt | 0\n\nSee, all lines are aligned to have length 27 bytes. But on the screen\nthis looks distorted because length in characters differs.\n\nOn Tue, 9 Aug 2022 at 22:20, Calvin Wan wrote:\n>\n> Hi Alexander,\n>\n> Thank you for the report! I attempted to reproduce with the steps you\n> provided, but was unable to do so. What commands would I have to run\n> on a clean git repository to reproduce this?\n>\n> - Calvin\n"},{"id":"460948","messageId":"CAFySSZBJU=NUR7HRWEUM-n2e4ww_FOPdkWUGLrFt4pybP-qq7Q@mail.gmail.com","threadId":"58282","inReplyTo":"CA+VDVVVQQ4=um_L_h=EQASPHD_oYjwZxecYewLrCAbQV_m4hwQ@mail.gmail.com","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-08-09T21:36:00Z","receivedAt":"2022-08-09T21:36:21Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Thank you for the additional information! I have reproduced the\nissue after setting 'core.quotepath' to false. Are you interested in\nsubmitting a patch to fix this? No pressure, but if you are I can\nwalk you through the process. If not I can take it from here :)\n\nCalvin\n"},{"id":"460961","messageId":"xmqqsfm4prqk.fsf@gitster.g","threadId":"58282","inReplyTo":"20220809182045.568598-1-calvinwan@google.com","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T05:55:31Z","receivedAt":"2022-08-10T05:56:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> Hi Alexander,\n>\n> Thank you for the report! I attempted to reproduce with the steps you\n> provided, but was unable to do so. What commands would I have to run\n> on a clean git repository to reproduce this?\n\nSounds like a symptom observable when the width computed by\nutf8.c::git_gcwidth(), using the width table imported from\nunicode.org, and the width the terminal thinks each of the displayed\ncharacter has, do not match (e.g. seen when ambiguous characters are\ninvolved, https://unicode.org/reports/tr11/#Ambiguous).\n\n\n"},{"id":"460964","messageId":"20220810084017.gnnodcbt5lyibbf6@tb-raspi4","threadId":"58282","inReplyTo":"xmqqsfm4prqk.fsf@gitster.g","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-10T08:40:17Z","receivedAt":"2022-08-10T08:40:33Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 09, 2022 at 10:55:31PM -0700, Junio C Hamano wrote:\n> Calvin Wan writes:\n>\n> > Hi Alexander,\n> >\n> > Thank you for the report! I attempted to reproduce with the steps you\n> > provided, but was unable to do so. What commands would I have to run\n> > on a clean git repository to reproduce this?\n>\n> Sounds like a symptom observable when the width computed by\n> utf8.c::git_gcwidth(), using the width table imported from\n> unicode.org, and the width the terminal thinks each of the displayed\n> character has, do not match (e.g. seen when ambiguous characters are\n> involved, https://unicode.org/reports/tr11/#Ambiguous).\n>\n\nI am not fully sure about that - I can reproduce it with Latin based\nfile names as well:\n\n git log --stat\n[snip]\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nFrom this very first experiment I would suspect that we use\nstrlen() somewhere rather then utf8.c::git_gcwidth()\n\nMore digging needed (but I don't promise anything today)\n"},{"id":"460966","messageId":"CA+VDVVUKf48Q9A0hWPnBE+qG_7tBDuXKkdo+wWDU7iC3Wg=oEg@mail.gmail.com","threadId":"58282","inReplyTo":"20220810084017.gnnodcbt5lyibbf6@tb-raspi4","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Alexander Meshcheryakov","fromEmail":"alexander.s.m@gmail.com","sentAt":"2022-08-10T08:56:11Z","receivedAt":"2022-08-10T08:56:47Z","isPatch":false,"sender":{"key":"alexander.s.m@gmail.com","avatar":"https://gravatar.com/avatar/b746bedd5dfb858b4b10909a07c7db58e3c23507abde0931efcf3e5c34f875ba?d=mp&s=160"},"body":"I believe I have found exact place where strlen is used incorrectly\nThis is at diff.c:show_stats\n\nhttps://github.com/git/git/blob/c50926e1f48891e2671e1830dbcd2912a4563450/diff.c#L2623\n\nIt probably should be replaced with one of utf8_width, utf8_strnwidth\nor utf8_strwidth from utf8.c\n\n\nOn Wed, 10 Aug 2022 at 12:40, Torsten Bögershausen wrote:\n>\n> On Tue, Aug 09, 2022 at 10:55:31PM -0700, Junio C Hamano wrote:\n> > Calvin Wan writes:\n> >\n> > > Hi Alexander,\n> > >\n> > > Thank you for the report! I attempted to reproduce with the steps you\n> > > provided, but was unable to do so. What commands would I have to run\n> > > on a clean git repository to reproduce this?\n> >\n> > Sounds like a symptom observable when the width computed by\n> > utf8.c::git_gcwidth(), using the width table imported from\n> > unicode.org, and the width the terminal thinks each of the displayed\n> > character has, do not match (e.g. seen when ambiguous characters are\n> > involved, https://unicode.org/reports/tr11/#Ambiguous).\n> >\n>\n> I am not fully sure about that - I can reproduce it with Latin based\n> file names as well:\n>\n> git log --stat\n> [snip]\n> Arger.txt | 1 +\n> Ärger.txt | 1 +\n> 2 files changed, 2 insertions(+)\n>\n> From this very first experiment I would suspect that we use\n> strlen() somewhere rather then utf8.c::git_gcwidth()\n>\n> More digging needed (but I don't promise anything today)\n"},{"id":"460974","messageId":"20220810095157.wo4jaumtu47qplsb@tb-raspi4","threadId":"58282","inReplyTo":"CA+VDVVUKf48Q9A0hWPnBE+qG_7tBDuXKkdo+wWDU7iC3Wg=oEg@mail.gmail.com","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-10T09:51:58Z","receivedAt":"2022-08-10T09:52:14Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 10, 2022 at 12:56:11PM +0400, Alexander Meshcheryakov wrote:\n\nThanks for digging.\n\n(And please, try to avoid top-posting here in this list)\n\n> I believe I have found exact place where strlen is used incorrectly\n> This is at diff.c:show_stats\n>\n> https://github.com/git/git/blob/c50926e1f48891e2671e1830dbcd2912a4563450/diff.c#L2623\n>\n> It probably should be replaced with one of utf8_width, utf8_strnwidth\n> or utf8_strwidth from utf8.c\n\nThat did not help here. If I understand it right, this function is not at all involved\nin our `git log --stat` ?\n\nI tried this patch (not 100% git-style) and didn't see any print.\n\n\n--- a/diff.c\n+++ b/diff.c\n@@ -2620,7 +2620,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n continue;\n\t\t\t }\n\t\t\t\t\t fill_print_name(file);\n\t\t\t\t\t\t\t- len = strlen(file->print_name);\n\t\t\t\t\t\t\t+ {\n\t\t\t\t\t\t\t+ const char *cp = file->print_name;\n\t\t\t\t\t\t\t+ size_t l = strlen(file->print_name);\n\t\t\t\t\t\t\t+ len = utf8_width(&cp, &l);\n\t\t\t\t\t\t\t+ fprintf(stderr, \"%s/%s:%d file->print_name='%s' len=%lu\\n\",\n\t\t\t\t\t\t\t+ __FILE__, __FUNCTION__, __LINE__,\n\t\t\t\t\t\t\t+ file->print_name, (unsigned long)len);\n\t\t\t\t\t\t\t+ }\n\t\t\t\t\t\t\t if (max_len < len)\n\t\t\t\t\t\t\t\t\t max_len = len;\n\n\nAnd looking here, it seems as we are calculating max_len here.\nStill more digging needed (but I don't promise anything today)\n"},{"id":"460977","messageId":"20220810114151.uhaun5gbknd5btyz@tb-raspi4","threadId":"58282","inReplyTo":"20220810095157.wo4jaumtu47qplsb@tb-raspi4","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-10T11:41:51Z","receivedAt":"2022-08-10T11:42:04Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 10, 2022 at 11:51:58AM +0200, Torsten Bögershausen wrote:\n> On Wed, Aug 10, 2022 at 12:56:11PM +0400, Alexander Meshcheryakov wrote:\n>\n> Thanks for digging.\n>\n> (And please, try to avoid top-posting here in this list)\n>\n> > I believe I have found exact place where strlen is used incorrectly\n> > This is at diff.c:show_stats\n> >\n> > https://github.com/git/git/blob/c50926e1f48891e2671e1830dbcd2912a4563450/diff.c#L2623\n> >\n> > It probably should be replaced with one of utf8_width, utf8_strnwidth\n> > or utf8_strwidth from utf8.c\n>\n\nPlease forget what I wrote earlier - I was running the wrong `git` binary :-(\nSorry for the noise.\n\nI can probably do more testing soonish.\n\n"},{"id":"461001","messageId":"xmqqiln0p01z.fsf@gitster.g","threadId":"58282","inReplyTo":"20220810084017.gnnodcbt5lyibbf6@tb-raspi4","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T15:53:28Z","receivedAt":"2022-08-10T15:56:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> git log --stat\n> [snip]\n> Arger.txt | 1 +\n> Ärger.txt | 1 +\n> 2 files changed, 2 insertions(+)\n>\n> From this very first experiment I would suspect that we use\n> strlen() somewhere rather then utf8.c::git_gcwidth()\n\nYeah, that does sound like the case, and quite honestly, knowing\nthat the diffstat code is way older than unicode-width code, which\nwas added by you in mid 2014, I am not all that surprised if we used\nto use strlen() throughout and we still do by mistake.\n\nThanks for a doze of sanity.\n"},{"id":"461009","messageId":"20220810173554.sl3bxtosnszygs5f@tb-raspi4","threadId":"58282","inReplyTo":"xmqqiln0p01z.fsf@gitster.g","subject":"Re: [BUG] Unicode filenames handling in `git log --stat`","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-10T17:35:54Z","receivedAt":"2022-08-10T17:36:07Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 10, 2022 at 08:53:28AM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> > git log --stat\n> > [snip]\n> > Arger.txt | 1 +\n> > Ärger.txt | 1 +\n> > 2 files changed, 2 insertions(+)\n> >\n> > From this very first experiment I would suspect that we use\n> > strlen() somewhere rather then utf8.c::git_gcwidth()\n>\n> Yeah, that does sound like the case, and quite honestly, knowing\n> that the diffstat code is way older than unicode-width code, which\n> was added by you in mid 2014, I am not all that surprised if we used\n> to use strlen() throughout and we still do by mistake.\n>\n> Thanks for a doze of sanity.\n\nSome 2 updates here:\n- The strlen() needs a replacement.\n It looks as if the following patch helps:\n\n/* somewhere in diff.c */\nstatic size_t screen_utf8_width(const char *start)\n{\n const char *cp = start;\n size_t remain = strlen(start);\n size_t width = 0;\n\n while (remain) {\n int n = utf8_width(&cp, &remain);\n if (n < 0)\n return strlen(start); /* not UTF-8 ? Use strlen() */\n width += n;\n }\n return width;\n}\n\n@@ -2620,7 +2635,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n continue;\n\t\t\t }\n\t\t\t\t\t fill_print_name(file);\n\t\t\t\t\t\t\t- len = strlen(file->print_name);\n\t\t\t\t\t\t\t+ len = screen_utf8_width(file->print_name);\n\t\t\t\t\t\t\t if (max_len < len)\n\t\t\t\t\t\t\t\t\t max_len = len;\n\n@@ -2743,7 +2758,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n * \"scale\" the filename\n\t\t */\n\t\t\t\t len = name_width;\n\t\t\t\t\t\t - name_len = strlen(name);\n\t\t\t\t\t\t + name_len = screen_utf8_width(name);\n\t\t\t\t\t\t if (name_width < name_len) {\n\n\n=====================================\nLet's see if I can make a proper patch out of it.\n\nThe second problem, and I hoped it wasn't, seems to be related to what\nyou had digged out earlier.\n\n>Sounds like a symptom observable when the width computed by\n>utf8.c::git_gcwidth(), using the width table imported from\n>unicode.org, and the width the terminal thinks each of the displayed\n>character has, do not match (e.g. seen when ambiguous characters are\n>involved, https://unicode.org/reports/tr11/#Ambiguous).\n\nThat needs a second patch, probably after some more digging,\nhow unicode is rendedered on the different systems\n"},{"id":"461190","messageId":"20220814133531.16952-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH/RFC 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-08-14T13:35:31Z","receivedAt":"2022-08-14T13:40:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n$ git log --stat\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nA side note: the original report was about cyrillic filenames.\nAfter some investigations it turned out that\na) This is not a problem with \"ambiguous characters\" in unicode\nb) The same problem exist for all unicode code points (so we\n can use Latin based Umlauts for demonstrations below)\n\nThe 'Ä' takes the same space on the screen as the 'A'.\nBut needs one more byte in memory, so the the `git log --stat` output\nfor \"Arger.txt\" (!) gets mis-aligned:\nThe maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\nfor aligment, it needs 9 bytes in memory.\nIf there was a file \"Ö\", it would be correctly aligned by chance,\nbut \"Öhö\" would not.\n\nThe solution is of course, to use utf8_strwidth() instead of strlen()\nwhen dealing with the width on screen.\n\nAnd then there is another problem: code like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n\n(or using the underlying snprintf() function) does not align the\nbuffer to a minimum of len measured in screen-width, but uses the\nmemory count, if name is UTF-8 encoded.\n\nWe could be tempted to wish that snprintf() was UTF-8 aware.\nThat doesn't seem to be the case anywhere (tested on Linux and Mac),\nprobably snprintf() uses the \"bytes in memory\"/strlen() approach to be\ncompatible with older versions and this will never change.\n\nThe choosen solution is to split code in diff.c like this\n\nstrbuf_addf(&out, \"%-*s\", len, name);\n\ninto 2 calls, like this:\n\nstrbuf_addf(&out, \"%s\", name);\nif (len > utf8_strwidth(name))\n strbuf_addchars(&out, ' ', len - utf8_strwidth(name));\n\nReported-by: Alexander Meshcheryakov \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 25 +++++++++++++++++--------\n 1 file changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 974626a621..7fb254c545 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\tfill_print_name(file);\n-\t\tlen = strlen(file->print_name);\n+\t\tlen = utf8_strwidth(file->print_name);\n \t\tif (max_len < len)\n \t\t\tmax_len = len;\n\n@@ -2734,6 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n+\t\tsize_t num_padding_spaces = 0;\n \t\tint name_len;\n\n \t\tif (!file->is_interesting && (added + deleted == 0))\n@@ -2743,7 +2744,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t * \"scale\" the filename\n \t\t */\n \t\tlen = name_width;\n-\t\tname_len = strlen(name);\n+\t\tname_len = utf8_strwidth(name);\n \t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n@@ -2753,10 +2754,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n+\t\tif (len > utf8_strwidth(name))\n+\t\t\tnum_padding_spaces = len - utf8_strwidth(name);\n\n \t\tif (file->is_binary) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addf(&out, \"| %*s\", number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n \t\t\t\tstrbuf_addch(&out, '\\n');\n \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n@@ -2776,8 +2781,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addstr(&out, \"| Unmerged\\n\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\n@@ -2803,8 +2810,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n+\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\tif (num_padding_spaces)\n+\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\tstrbuf_addf(&out, \"| %*\"PRIuMAX\"%s\",\n \t\t\tnumber_width, added + deleted,\n \t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(&out, '+', add, add_c, reset);\n--\n2.34.0\n\n"},{"id":"461201","messageId":"xmqqfshy773n.fsf@gitster.g","threadId":"58282","inReplyTo":"20220814133531.16952-1-tboegi@web.de","subject":"Re: [PATCH/RFC 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-14T23:12:12Z","receivedAt":"2022-08-14T23:12:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> The choosen solution is to split code in diff.c like this\n>\n> strbuf_addf(&out, \"%-*s\", len, name);\n>\n> into 2 calls, like this:\n>\n> strbuf_addf(&out, \"%s\", name);\n> if (len > utf8_strwidth(name))\n> strbuf_addchars(&out, ' ', len - utf8_strwidth(name));\n\nMakes sense. Is utf8_strwidth(name) cheap enough that we can call\nit twice in a row on the same string casually, or should we avoid it\nwith a new variable?\n\nIt might be worth doing a helper function, even?\n\n\tstatic inline strbuf_pad(struct strbuf *out, const char *s, size_t width)\n\t{\n\t\tsize_t w = utf8_strwidth(s);\n\n\t\tstrbuf_addstr(out, s);\n\t\tif (w < width)\n\t\t\tstrbuf_addchars(out, ' ', width - w);\n\t}\n\nOther than that, sounds very sensible.\n\n"},{"id":"461217","messageId":"20220815063441.uxrtdqsggmrqxxl2@tb-raspi4","threadId":"58282","inReplyTo":"xmqqfshy773n.fsf@gitster.g","subject":"Re: [PATCH/RFC 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-15T06:34:41Z","receivedAt":"2022-08-15T06:34:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sun, Aug 14, 2022 at 04:12:12PM -0700, Junio C Hamano wrote:\n> tboegi@web.de writes:\n>\n> > The choosen solution is to split code in diff.c like this\n> >\n> > strbuf_addf(&out, \"%-*s\", len, name);\n> >\n> > into 2 calls, like this:\n> >\n> > strbuf_addf(&out, \"%s\", name);\n> > if (len > utf8_strwidth(name))\n> > strbuf_addchars(&out, ' ', len - utf8_strwidth(name));\n>\n> Makes sense. Is utf8_strwidth(name) cheap enough that we can call\n> it twice in a row on the same string casually, or should we avoid it\n> with a new variable?\n>\n> It might be worth doing a helper function, even?\n>\n> \tstatic inline strbuf_pad(struct strbuf *out, const char *s, size_t width)\n> \t{\n> \t\tsize_t w = utf8_strwidth(s);\n>\n> \t\tstrbuf_addstr(out, s);\n> \t\tif (w < width)\n> \t\t\tstrbuf_addchars(out, ' ', width - w);\n> \t}\n>\n> Other than that, sounds very sensible.\n>\n\nThanks for the review.\n\nActually, the commit message is wrong - after writing it, the code\nwas changed into\n\nif (len > utf8_strwidth(name))\n num_padding_spaces = len - utf8_strwidth(name);\n\nand later\n\nif (num_padding_spaces)\n\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n\n(And having written this, there is probably room for test cases,\nIOW: a V2 will come the next days)\n"},{"id":"461468","messageId":"xmqqedxdnu6d.fsf@gitster.g","threadId":"58282","inReplyTo":"20220815063441.uxrtdqsggmrqxxl2@tb-raspi4","subject":"Re: [PATCH/RFC 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-18T21:00:42Z","receivedAt":"2022-08-18T21:02:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> (And having written this, there is probably room for test cases,\n> IOW: a V2 will come the next days)\n\nYeah, that all sounds sensible.\n\nThanks for working on this.\n"},{"id":"462022","messageId":"20220827085007.20030-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-08-27T08:50:07Z","receivedAt":"2022-08-27T08:50:37Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n$ git log --stat\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nA side note: the original report was about cyrillic filenames.\nAfter some investigations it turned out that\na) This is not a problem with \"ambiguous characters\" in unicode\nb) The same problem exist for all unicode code points (so we\n can use Latin based Umlauts for demonstrations below)\n\nThe 'Ä' takes the same space on the screen as the 'A'.\nBut needs one more byte in memory, so the the `git log --stat` output\nfor \"Arger.txt\" (!) gets mis-aligned:\nThe maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\nfor aligment, it needs 9 bytes in memory.\nIf there was a file \"Ö\", it would be correctly aligned by chance,\nbut \"Öhö\" would not.\n\nThe solution is of course, to use utf8_strwidth() instead of strlen()\nwhen dealing with the width on screen.\n\nAnd then there is another problem: code like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n\n(or using the underlying snprintf() function) does not align the\nbuffer to a minimum of len measured in screen-width, but uses the\nmemory count, if name is UTF-8 encoded.\n\nWe could be tempted to wish that snprintf() was UTF-8 aware.\nThat doesn't seem to be the case anywhere (tested on Linux and Mac),\nprobably snprintf() uses the \"bytes in memory\"/strlen() approach to be\ncompatible with older versions and this will never change.\n\nThe choosen solution is to split code in diff.c like this\n\nstrbuf_addf(&out, \"%-*s\", len, name);\n\ninto something like this:\n\nsize_t num_padding_spaces = 0;\n// [snip]\nif (len > utf8_strwidth(name))\n num_padding_spaces = len - utf8_strwidth(name);\nstrbuf_addf(&out, \"%s\", name);\nif (num_padding_spaces)\n strbuf_addchars(&out, ' ', num_padding_spaces);\n\nTests:\nTwo things need to be tested:\n- The calculation of the maximum width\n- The calculation of num_padding_spaces\n\nThe name \"textfile\" is changed into \"textfilë\", both have a width of 8.\nIf strlen() was used, to get the maximum width, the shorter \"binfile\" would\nhave been mis-aligned:\n binfile | [snip]\n textfilë | [snip]\n\nIf only \"binfile\" would be renamed into \"binfilë\":\n binfilë | [snip]\n textfile | [snip]\n\nIn order to verify that the width is calculated correctly everywhere,\n\"binfile\" is renamed into \"binfïlë\", giving 2 bytes more in strlen()\n\"textfile\" is renamed into \"textfilë\", 1 byte more in strlen(),\nand the updated t4012-diff-binary.sh checks the correct aligment:\n binfïlë | [snip]\n textfilë | [snip]\n\nReported-by: Alexander Meshcheryakov \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 37 +++++++++++++++++++++++--------------\n t/t4012-diff-binary.sh | 14 +++++++-------\n 2 files changed, 30 insertions(+), 21 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 974626a621..cf38e1dc88 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2591,7 +2591,7 @@ void print_stat_summary(FILE *fp, int files,\n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n {\n \tint i, len, add, del, adds = 0, dels = 0;\n-\tuintmax_t max_change = 0, max_len = 0;\n+\tuintmax_t max_change = 0, max_width = 0;\n \tint total_files = data->nr, count;\n \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n \tconst char *reset, *add_c, *del_c;\n@@ -2620,9 +2620,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\tfill_print_name(file);\n-\t\tlen = strlen(file->print_name);\n-\t\tif (max_len < len)\n-\t\t\tmax_len = len;\n+\t\tlen = utf8_strwidth(file->print_name);\n+\t\tif (max_width < len)\n+\t\t\tmax_width = len;\n\n \t\tif (file->is_unmerged) {\n \t\t\t/* \"Unmerged\" is 8 characters */\n@@ -2646,7 +2646,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n\n \t/*\n \t * We have width = stat_width or term_columns() columns total.\n-\t * We want a maximum of min(max_len, stat_name_width) for the name part.\n+\t * We want a maximum of min(max_width, stat_name_width) for the name part.\n \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n \t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n \t * for \" | NNNN \" and one the empty column at the end, altogether\n@@ -2701,8 +2701,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tgraph_width = options->stat_graph_width;\n\n \tname_width = (options->stat_name_width > 0 &&\n-\t\t options->stat_name_width < max_len) ?\n-\t\toptions->stat_name_width : max_len;\n+\t\t options->stat_name_width < max_width) ?\n+\t\toptions->stat_name_width : max_width;\n\n \t/*\n \t * Adjust adjustable widths not to exceed maximum width\n@@ -2734,6 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n+\t\tsize_t num_padding_spaces = 0;\n \t\tint name_len;\n\n \t\tif (!file->is_interesting && (added + deleted == 0))\n@@ -2743,7 +2744,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t * \"scale\" the filename\n \t\t */\n \t\tlen = name_width;\n-\t\tname_len = strlen(name);\n+\t\tname_len = utf8_strwidth(name);\n \t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n@@ -2753,10 +2754,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n+\t\tif (len > utf8_strwidth(name))\n+\t\t\tnum_padding_spaces = len - utf8_strwidth(name);\n\n \t\tif (file->is_binary) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addf(&out, \"| %*s\", number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n \t\t\t\tstrbuf_addch(&out, '\\n');\n \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n@@ -2776,8 +2781,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addstr(&out, \"| Unmerged\\n\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\n@@ -2803,8 +2810,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n+\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\tif (num_padding_spaces)\n+\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\tstrbuf_addf(&out, \"| %*\"PRIuMAX\"%s\",\n \t\t\tnumber_width, added + deleted,\n \t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(&out, '+', add, add_c, reset);\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex c509143c81..2d49de01c8 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -113,20 +113,20 @@ test_expect_success 'diff --no-index with binary creation' '\n '\n\n cat >expect < 1026 bytes\n- textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+ binfïlë | Bin 0 -> 1026 bytes\n+ textfilë | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n\n test_expect_success 'diff --stat with binary files and big change count' '\n-\tprintf \"\\01\\00%1024d\" 1 >binfile &&\n-\tgit add binfile &&\n+\tprintf \"\\01\\00%1024d\" 1 >binfïlë &&\n+\tgit add binfïlë &&\n \ti=0 &&\n \twhile test $i -lt 10000; do\n \t\techo $i &&\n \t\ti=$(($i + 1)) || return 1\n-\tdone >textfile &&\n-\tgit add textfile &&\n-\tgit diff --cached --stat binfile textfile >output &&\n+\tdone >textfilë &&\n+\tgit add textfilë &&\n+\tgit -c core.quotepath=false diff --cached --stat binfïlë textfilë >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n '\n--\n2.34.0\n\n"},{"id":"462023","messageId":"20220827085439.4qqfdggdhnytxxav@tb-raspi4","threadId":"58282","inReplyTo":"20220827085007.20030-1-tboegi@web.de","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-27T08:54:39Z","receivedAt":"2022-08-27T08:54:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Aug 27, 2022 at 10:50:07AM +0200, tboegi@web.de wrote:\n> From: Torsten Bögershausen \n>\n\n\n> b) The same problem exist for all unicode code points (so we\n\nThat should be \"exists\". Let's see if there are more comments,\nbefore sending a new patch.\n"},{"id":"462024","messageId":"CAPig+cQEj6LOW68r7m7pb5LAwZKjvP-Z53f3Sh4Gxy0P3gA3cw@mail.gmail.com","threadId":"58282","inReplyTo":"20220827085439.4qqfdggdhnytxxav@tb-raspi4","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-27T09:50:46Z","receivedAt":"2022-08-27T09:51:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 27, 2022 at 4:58 AM Torsten Bögershausen wrote:\n> On Sat, Aug 27, 2022 at 10:50:07AM +0200, tboegi@web.de wrote:\n> > From: Torsten Bögershausen \n> > b) The same problem exist for all unicode code points (so we\n>\n> That should be \"exists\". Let's see if there are more comments,\n> before sending a new patch.\n\nHere's one:\n\n> The choosen solution is to split code in diff.c like this\n\ns/choosen/chosen/\n"},{"id":"462085","messageId":"0q921n79-sr17-2794-83r0-r59rnqq03pp2@tzk.qr","threadId":"58282","inReplyTo":"20220827085007.20030-1-tboegi@web.de","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-29T12:04:42Z","receivedAt":"2022-08-29T12:35:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sat, 27 Aug 2022, tboegi@web.de wrote:\n\n> From: Torsten Bögershausen \n>\n> When unicode filenames (encoded in UTF-8) are used, the visible width\n> on the screen is not the same as strlen(filename).\n>\n> For example, `git log --stat` may produce an output like this:\n>\n> $ git log --stat\n>\n> [snip the header]\n>\n> Arger.txt | 1 +\n> Ärger.txt | 1 +\n> 2 files changed, 2 insertions(+)\n>\n> A side note: the original report was about cyrillic filenames.\n> After some investigations it turned out that\n> a) This is not a problem with \"ambiguous characters\" in unicode\n> b) The same problem exist for all unicode code points (so we\n> can use Latin based Umlauts for demonstrations below)\n>\n> The 'Ä' takes the same space on the screen as the 'A'.\n> But needs one more byte in memory, so the the `git log --stat` output\n> for \"Arger.txt\" (!) gets mis-aligned:\n> The maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n> 9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\n> for aligment, it needs 9 bytes in memory.\n> If there was a file \"Ö\", it would be correctly aligned by chance,\n> but \"Öhö\" would not.\n>\n> The solution is of course, to use utf8_strwidth() instead of strlen()\n> when dealing with the width on screen.\n>\n> And then there is another problem: code like this\n> strbuf_addf(&out, \"%-*s\", len, name);\n>\n> (or using the underlying snprintf() function) does not align the\n> buffer to a minimum of len measured in screen-width, but uses the\n> memory count, if name is UTF-8 encoded.\n>\n> We could be tempted to wish that snprintf() was UTF-8 aware.\n> That doesn't seem to be the case anywhere (tested on Linux and Mac),\n> probably snprintf() uses the \"bytes in memory\"/strlen() approach to be\n> compatible with older versions and this will never change.\n\nAn interesting read so, far, but...\n\n>\n> The choosen solution is to split code in diff.c like this\n>\n> strbuf_addf(&out, \"%-*s\", len, name);\n>\n> into something like this:\n>\n> size_t num_padding_spaces = 0;\n> // [snip]\n> if (len > utf8_strwidth(name))\n> num_padding_spaces = len - utf8_strwidth(name);\n> strbuf_addf(&out, \"%s\", name);\n> if (num_padding_spaces)\n> strbuf_addchars(&out, ' ', num_padding_spaces);\n\n... this sounds like it would benefit from beinv refactored into a\nseparate function, e.g. `strbuf_add_padded(buf, utf8string)`, both for\nreadability as well as for self-documentation.\n\nAlso, it is unclear to me why we have to evaluate `utf8_strwidth()`\n_twice_ and why we do not assign the result to a variable called `width`\nand then have a conditional like\n\n\tif (width < len) /* pad to `len` columns */\n\t\tstrbuf_addchars(&out, ' ' , len - width);\n\ninstead. That would sound more logical to me.\n\nBesides, since the simple change from `strlen()` to `utf8_strwidth()` is\nso different from changing `strbuf_addf(...)`, I would prefer to see them\nsplit into two patches.\n\n>\n> Tests:\n> Two things need to be tested:\n> - The calculation of the maximum width\n> - The calculation of num_padding_spaces\n>\n> The name \"textfile\" is changed into \"textfilë\", both have a width of 8.\n> If strlen() was used, to get the maximum width, the shorter \"binfile\" would\n> have been mis-aligned:\n> binfile | [snip]\n> textfilë | [snip]\n>\n> If only \"binfile\" would be renamed into \"binfilë\":\n> binfilë | [snip]\n> textfile | [snip]\n>\n> In order to verify that the width is calculated correctly everywhere,\n> \"binfile\" is renamed into \"binfïlë\", giving 2 bytes more in strlen()\n> \"textfile\" is renamed into \"textfilë\", 1 byte more in strlen(),\n> and the updated t4012-diff-binary.sh checks the correct aligment:\n> binfïlë | [snip]\n> textfilë | [snip]\n\nI wonder whether you can change only _one_ name and still verify the\ncorrectness. When you make two changes at the same time, it is always\npossible for one change to \"cancel out\" the other one, and therefore it is\nharder to reason about the correctness of your patch.\n\nBetter keep it simple and change only one instance (personally, I would\nhave changed two letters in the longer one).\n\n>\n> Reported-by: Alexander Meshcheryakov \n> Signed-off-by: Torsten Bögershausen \n> ---\n> diff.c | 37 +++++++++++++++++++++++--------------\n> t/t4012-diff-binary.sh | 14 +++++++-------\n> 2 files changed, 30 insertions(+), 21 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 974626a621..cf38e1dc88 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2591,7 +2591,7 @@ void print_stat_summary(FILE *fp, int files,\n> static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> {\n> \tint i, len, add, del, adds = 0, dels = 0;\n> -\tuintmax_t max_change = 0, max_len = 0;\n> +\tuintmax_t max_change = 0, max_width = 0;\n\nWhy rename `max_len`, but not `len`? I would have expected (and agreed\nwith seeing) `len` to be renamed to `width`, too.\n\n> \tint total_files = data->nr, count;\n> \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n> \tconst char *reset, *add_c, *del_c;\n> @@ -2620,9 +2620,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tfill_print_name(file);\n> -\t\tlen = strlen(file->print_name);\n> -\t\tif (max_len < len)\n> -\t\t\tmax_len = len;\n> +\t\tlen = utf8_strwidth(file->print_name);\n> +\t\tif (max_width < len)\n> +\t\t\tmax_width = len;\n>\n> \t\tif (file->is_unmerged) {\n> \t\t\t/* \"Unmerged\" is 8 characters */\n> @@ -2646,7 +2646,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>\n> \t/*\n> \t * We have width = stat_width or term_columns() columns total.\n> -\t * We want a maximum of min(max_len, stat_name_width) for the name part.\n> +\t * We want a maximum of min(max_width, stat_name_width) for the name part.\n> \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n> \t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n> \t * for \" | NNNN \" and one the empty column at the end, altogether\n> @@ -2701,8 +2701,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\tgraph_width = options->stat_graph_width;\n>\n> \tname_width = (options->stat_name_width > 0 &&\n> -\t\t options->stat_name_width < max_len) ?\n> -\t\toptions->stat_name_width : max_len;\n> +\t\t options->stat_name_width < max_width) ?\n> +\t\toptions->stat_name_width : max_width;\n\nIt is a bit sad that the diff lines regarding the renamed variable drown\nout the actual change (`strlen()` -> `utf8_strwidth()`). But the end\nresult is nicer.\n\nThank you for working on this!\nDscho\n\n>\n> \t/*\n> \t * Adjust adjustable widths not to exceed maximum width\n> @@ -2734,6 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\tchar *name = file->print_name;\n> \t\tuintmax_t added = file->added;\n> \t\tuintmax_t deleted = file->deleted;\n> +\t\tsize_t num_padding_spaces = 0;\n> \t\tint name_len;\n>\n> \t\tif (!file->is_interesting && (added + deleted == 0))\n> @@ -2743,7 +2744,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t * \"scale\" the filename\n> \t\t */\n> \t\tlen = name_width;\n> -\t\tname_len = strlen(name);\n> +\t\tname_len = utf8_strwidth(name);\n> \t\tif (name_width < name_len) {\n> \t\t\tchar *slash;\n> \t\t\tprefix = \"...\";\n> @@ -2753,10 +2754,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tif (slash)\n> \t\t\t\tname = slash;\n> \t\t}\n> +\t\tif (len > utf8_strwidth(name))\n> +\t\t\tnum_padding_spaces = len - utf8_strwidth(name);\n>\n> \t\tif (file->is_binary) {\n> -\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n> +\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\t\tif (num_padding_spaces)\n> +\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\t\tstrbuf_addf(&out, \"| %*s\", number_width, \"Bin\");\n> \t\t\tif (!added && !deleted) {\n> \t\t\t\tstrbuf_addch(&out, '\\n');\n> \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n> @@ -2776,8 +2781,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\telse if (file->is_unmerged) {\n> -\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n> +\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\t\tif (num_padding_spaces)\n> +\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\t\tstrbuf_addstr(&out, \"| Unmerged\\n\");\n> \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n> \t\t\t\t\t out.buf, out.len, 0);\n> \t\t\tstrbuf_reset(&out);\n> @@ -2803,8 +2810,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\t\tadd = total - del;\n> \t\t\t}\n> \t\t}\n> -\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n> +\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\tif (num_padding_spaces)\n> +\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\tstrbuf_addf(&out, \"| %*\"PRIuMAX\"%s\",\n> \t\t\tnumber_width, added + deleted,\n> \t\t\tadded + deleted ? \" \" : \"\");\n> \t\tshow_graph(&out, '+', add, add_c, reset);\n> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\n> index c509143c81..2d49de01c8 100755\n> --- a/t/t4012-diff-binary.sh\n> +++ b/t/t4012-diff-binary.sh\n> @@ -113,20 +113,20 @@ test_expect_success 'diff --no-index with binary creation' '\n> '\n>\n> cat >expect < - binfile | Bin 0 -> 1026 bytes\n> - textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> + binfïlë | Bin 0 -> 1026 bytes\n> + textfilë | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> EOF\n>\n> test_expect_success 'diff --stat with binary files and big change count' '\n> -\tprintf \"\\01\\00%1024d\" 1 >binfile &&\n> -\tgit add binfile &&\n> +\tprintf \"\\01\\00%1024d\" 1 >binfïlë &&\n> +\tgit add binfïlë &&\n> \ti=0 &&\n> \twhile test $i -lt 10000; do\n> \t\techo $i &&\n> \t\ti=$(($i + 1)) || return 1\n> -\tdone >textfile &&\n> -\tgit add textfile &&\n> -\tgit diff --cached --stat binfile textfile >output &&\n> +\tdone >textfilë &&\n> +\tgit add textfilë &&\n> +\tgit -c core.quotepath=false diff --cached --stat binfïlë textfilë >output &&\n> \tgrep \" | \" output >actual &&\n> \ttest_cmp expect actual\n> '\n> --\n> 2.34.0\n>\n>\n"},{"id":"462103","messageId":"20220829175425.cmbwtqpxrq4ppnnk@tb-raspi4","threadId":"58282","inReplyTo":"0q921n79-sr17-2794-83r0-r59rnqq03pp2@tzk.qr","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-08-29T17:54:25Z","receivedAt":"2022-08-29T17:54:33Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Aug 29, 2022 at 02:04:42PM +0200, Johannes Schindelin wrote:\n> Hi Torsten,\n> >\n> > The choosen solution is to split code in diff.c like this\n> >\n> > strbuf_addf(&out, \"%-*s\", len, name);\n> >\n> > into something like this:\n> >\n> > size_t num_padding_spaces = 0;\n> > // [snip]\n> > if (len > utf8_strwidth(name))\n> > num_padding_spaces = len - utf8_strwidth(name);\n> > strbuf_addf(&out, \"%s\", name);\n> > if (num_padding_spaces)\n> > strbuf_addchars(&out, ' ', num_padding_spaces);\n>\n> ... this sounds like it would benefit from beinv refactored into a\n> separate function, e.g. `strbuf_add_padded(buf, utf8string)`, both for\n> readability as well as for self-documentation.\n\nYes, but:\nAll (tm) strbuf() functions use an unsigned size_t, and are not\ntolerant against passing 0 as \"do nothing\".\nA nicer solution (for this patch) could be a change like this:\nInstead of\n\nvoid strbuf_addchars(struct strbuf *sb, int c, size_t n)\n{\n strbuf_grow(sb, n);\n\tmemset(sb->buf + sb->len, c, n);\n\tstrbuf_setlen(sb, sb->len + n);\n}\n\nWe would find:\nvoid strbuf_addchars(struct strbuf *sb, int c, ssize_t n)\n{\n if (n <= 0)\n\t return;\n strbuf_grow(sb, (size_t)n);\n\tmemset(sb->buf + sb->len, c, (size_t)n);\n\tstrbuf_setlen(sb, sb->len + (size_t)n);\n}\n\nI couldn't convince myself to do so.\nSince it is mainly diff.c that needs this adjustment/padding of strings,\nI coulnd't convince myself to write another function in strbuf.c\n\n\n>\n> Also, it is unclear to me why we have to evaluate `utf8_strwidth()`\n> _twice_ and why we do not assign the result to a variable called `width`\n> and then have a conditional like\n>\n> \tif (width < len) /* pad to `len` columns */\n> \t\tstrbuf_addchars(&out, ' ' , len - width);\n>\n> instead. That would sound more logical to me.\n\nThis is caused by the logic in diff.c:\n /*\n * Find the longest filename and max number of changes\n */\n for (i = 0; (i < count) && (i < data->nr); i++) {\n struct diffstat_file *file = data->files[i];\n [snip]\n len = utf8_strwidth(file->print_name);\n if (max_width < len)\n max_width = len;\n// and later\n /*\n * From here name_width is the width of the name area,\n * and graph_width is the width of the graph area.\n * max_change is used to scale graph properly.\n */\n for (i = 0; i < count; i++) {\n /*\n * \"scale\" the filename\n */\n // TB: Which means either shortening it with ...\n // Or padding it, if needed, and here we need\n // another\n name_len = utf8_strwidth(name);\n\n>\n> Besides, since the simple change from `strlen()` to `utf8_strwidth()` is\n> so different from changing `strbuf_addf(...)`, I would prefer to see them\n> split into two patches.\n\nHm, that is a possiblity. Seems to ease the burden for reviewers.\n\n>\n> >\n> > Tests:\n> > Two things need to be tested:\n> > - The calculation of the maximum width\n> > - The calculation of num_padding_spaces\n> >\n> > The name \"textfile\" is changed into \"textfilë\", both have a width of 8.\n> > If strlen() was used, to get the maximum width, the shorter \"binfile\" would\n> > have been mis-aligned:\n> > binfile | [snip]\n> > textfilë | [snip]\n> >\n> > If only \"binfile\" would be renamed into \"binfilë\":\n> > binfilë | [snip]\n> > textfile | [snip]\n> >\n> > In order to verify that the width is calculated correctly everywhere,\n> > \"binfile\" is renamed into \"binfïlë\", giving 2 bytes more in strlen()\n> > \"textfile\" is renamed into \"textfilë\", 1 byte more in strlen(),\n> > and the updated t4012-diff-binary.sh checks the correct aligment:\n> > binfïlë | [snip]\n> > textfilë | [snip]\n>\n> I wonder whether you can change only _one_ name and still verify the\n> correctness. When you make two changes at the same time, it is always\n> possible for one change to \"cancel out\" the other one, and therefore it is\n> harder to reason about the correctness of your patch.\n\nNee, I have a hard time to see how a +/- 1 can \"cancel out\" a +/- 2.\nBut I may improve the commit message, to make that more clear.\n\n>\n> Better keep it simple and change only one instance (personally,\n> I would have changed two letters in the longer one).\n\nThat is certainly doable.\n\n\n>\n> >\n> > Reported-by: Alexander Meshcheryakov \n> > Signed-off-by: Torsten Bögershausen \n> > ---\n> > diff.c | 37 +++++++++++++++++++++++--------------\n> > t/t4012-diff-binary.sh | 14 +++++++-------\n> > 2 files changed, 30 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 974626a621..cf38e1dc88 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -2591,7 +2591,7 @@ void print_stat_summary(FILE *fp, int files,\n> > static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> > {\n> > \tint i, len, add, del, adds = 0, dels = 0;\n> > -\tuintmax_t max_change = 0, max_len = 0;\n> > +\tuintmax_t max_change = 0, max_width = 0;\n>\n> Why rename `max_len`, but not `len`? I would have expected (and agreed\n> with seeing) `len` to be renamed to `width`, too.\n\nThat is a valid point.\nThere is, however, already a variable called \"width\".\nAnd renaming this one into a new one as well ?\n\n>\n> > \tint total_files = data->nr, count;\n> > \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n> > \tconst char *reset, *add_c, *del_c;\n> > @@ -2620,9 +2620,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \t\tfill_print_name(file);\n> > -\t\tlen = strlen(file->print_name);\n> > -\t\tif (max_len < len)\n> > -\t\t\tmax_len = len;\n> > +\t\tlen = utf8_strwidth(file->print_name);\n> > +\t\tif (max_width < len)\n> > +\t\t\tmax_width = len;\n> >\n> > \t\tif (file->is_unmerged) {\n> > \t\t\t/* \"Unmerged\" is 8 characters */\n> > @@ -2646,7 +2646,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> >\n> > \t/*\n> > \t * We have width = stat_width or term_columns() columns total.\n> > -\t * We want a maximum of min(max_len, stat_name_width) for the name part.\n> > +\t * We want a maximum of min(max_width, stat_name_width) for the name part.\n> > \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n> > \t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n> > \t * for \" | NNNN \" and one the empty column at the end, altogether\n> > @@ -2701,8 +2701,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> > \t\tgraph_width = options->stat_graph_width;\n> >\n> > \tname_width = (options->stat_name_width > 0 &&\n> > -\t\t options->stat_name_width < max_len) ?\n> > -\t\toptions->stat_name_width : max_len;\n> > +\t\t options->stat_name_width < max_width) ?\n> > +\t\toptions->stat_name_width : max_width;\n>\n> It is a bit sad that the diff lines regarding the renamed variable drown\n> out the actual change (`strlen()` -> `utf8_strwidth()`). But the end\n> result is nicer.\n>\n> Thank you for working on this!\n> Dscho\n\nThanks so much for the review - let's see if I can make a better patch\nthe next days (better say weeks)\n"},{"id":"462105","messageId":"xmqqwnaqnbfy.fsf@gitster.g","threadId":"58282","inReplyTo":"20220829175425.cmbwtqpxrq4ppnnk@tb-raspi4","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T18:37:05Z","receivedAt":"2022-08-29T18:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> This is caused by the logic in diff.c:\n> /*\n> * Find the longest filename and max number of changes\n> */\n> for (i = 0; (i < count) && (i < data->nr); i++) {\n> struct diffstat_file *file = data->files[i];\n> [snip]\n> len = utf8_strwidth(file->print_name);\n> if (max_width < len)\n> max_width = len;\n> // and later\n> /*\n> * From here name_width is the width of the name area,\n> * and graph_width is the width of the graph area.\n> * max_change is used to scale graph properly.\n> */\n> for (i = 0; i < count; i++) {\n> /*\n> * \"scale\" the filename\n> */\n> // TB: Which means either shortening it with ...\n> // Or padding it, if needed, and here we need\n> // another\n> name_len = utf8_strwidth(name);\n>\n>>\n>> Besides, since the simple change from `strlen()` to `utf8_strwidth()` is\n>> so different from changing `strbuf_addf(...)`, I would prefer to see them\n>> split into two patches.\n>\n> Hm, that is a possiblity. Seems to ease the burden for reviewers.\n\nAnother thing I remembered (this is a comment primarily on the\noriginal I wrote based on 'all world is ASCII' mindset that led to\nthe use of strlen() as a display-width indicator) in the code is\nthat we \"abbreviate\" an overly long pathname and transform renames\nthat originally is in the a/b/c -> a/B/c form into a/{b->B}/c form,\nand IIRC they are all byte based. The latter may be OK because the\ntransformation is limited to '/' boundary, but the former may chomp\na single multi-byte letter in the middle, which would need to be\ncorrected as a part of this change.\n"},{"id":"462508","messageId":"20220902042138.13901-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v3 2/2] diff.c: More changes and tests around utf8_strwidth()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-09-02T04:21:38Z","receivedAt":"2022-09-02T04:21:49Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nThe last commit uses utf8_strwidth() instead of strlen() in diff.c\nand it is time to test the change.\n\nAnd now we detect another problem that is fixed here: code like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n(or using the underlying snprintf() function) does not align the\nbuffer to a minimum of len measured in screen-width, but uses the\nmemory count.\n\nOne could be tempted to wish that snprintf() was UTF-8 aware.\nThat doesn't seem to be the case anywhere (tested on Linux and Mac),\nprobably snprintf() uses the \"bytes in memory\"/strlen() approach to be\ncompatible with older versions and this will never change.\n\nThe chosen solution is to split code in diff.c like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n\ninto something like this:\nsize_t num_padding_spaces = 0;\n// [snip]\nif (len > utf8_strwidth(name))\n num_padding_spaces = len - utf8_strwidth(name);\nstrbuf_addf(&out, \"%s\", name);\nif (num_padding_spaces)\n strbuf_addchars(&out, ' ', num_padding_spaces);\n\nI couldn't convince myself to write a wrapper here that is\n\"easy to read and understandable\" and would fit nicely into the chain of\nstrbuf_addX() calls used in diff.c\n\nTests:\nTwo things need to be tested:\n - The calculation of the maximum width\n - The calculation of num_padding_spaces\n\nThe name \"textfile\" is changed into \"tëxtfilë\", both have a width of 8.\nIf strlen() was used, to get the maximum width, the shorter \"binfile\" would\nhave been mis-aligned:\n binfile | [snip]\n tëxtfilë | [snip]\n\nIf only \"binfile\" would be renamed into \"binfilë\":\n binfilë | [snip]\n textfile | [snip]\n\nIn order to verify that the width is calculated correctly everywhere,\n\"binfile\" is renamed into \"binfilë\", giving 1 bytes more in strlen()\n\"tëxtfile\" is renamed into \"tëxtfilë\", 2 byte more in strlen().\n\nThe updated t4012-diff-binary.sh checks the correct aligment:\n binfilë | [snip]\n tëxtfilë | [snip]\n\nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 33 +++++++++++++++++++++------------\n t/t4012-diff-binary.sh | 14 +++++++-------\n 2 files changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b5df464de5..cf38e1dc88 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2591,7 +2591,7 @@ void print_stat_summary(FILE *fp, int files,\n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n {\n \tint i, len, add, del, adds = 0, dels = 0;\n-\tuintmax_t max_change = 0, max_len = 0;\n+\tuintmax_t max_change = 0, max_width = 0;\n \tint total_files = data->nr, count;\n \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n \tconst char *reset, *add_c, *del_c;\n@@ -2621,8 +2621,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t}\n \t\tfill_print_name(file);\n \t\tlen = utf8_strwidth(file->print_name);\n-\t\tif (max_len < len)\n-\t\t\tmax_len = len;\n+\t\tif (max_width < len)\n+\t\t\tmax_width = len;\n\n \t\tif (file->is_unmerged) {\n \t\t\t/* \"Unmerged\" is 8 characters */\n@@ -2646,7 +2646,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n\n \t/*\n \t * We have width = stat_width or term_columns() columns total.\n-\t * We want a maximum of min(max_len, stat_name_width) for the name part.\n+\t * We want a maximum of min(max_width, stat_name_width) for the name part.\n \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n \t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n \t * for \" | NNNN \" and one the empty column at the end, altogether\n@@ -2701,8 +2701,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tgraph_width = options->stat_graph_width;\n\n \tname_width = (options->stat_name_width > 0 &&\n-\t\t options->stat_name_width < max_len) ?\n-\t\toptions->stat_name_width : max_len;\n+\t\t options->stat_name_width < max_width) ?\n+\t\toptions->stat_name_width : max_width;\n\n \t/*\n \t * Adjust adjustable widths not to exceed maximum width\n@@ -2734,6 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n+\t\tsize_t num_padding_spaces = 0;\n \t\tint name_len;\n\n \t\tif (!file->is_interesting && (added + deleted == 0))\n@@ -2753,10 +2754,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n+\t\tif (len > utf8_strwidth(name))\n+\t\t\tnum_padding_spaces = len - utf8_strwidth(name);\n\n \t\tif (file->is_binary) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addf(&out, \"| %*s\", number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n \t\t\t\tstrbuf_addch(&out, '\\n');\n \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n@@ -2776,8 +2781,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\t\tif (num_padding_spaces)\n+\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\t\tstrbuf_addstr(&out, \"| Unmerged\\n\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\n@@ -2803,8 +2810,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n+\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n+\t\tif (num_padding_spaces)\n+\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n+\t\tstrbuf_addf(&out, \"| %*\"PRIuMAX\"%s\",\n \t\t\tnumber_width, added + deleted,\n \t\t\tadded + deleted ? \" \" : \"\");\n \t\tshow_graph(&out, '+', add, add_c, reset);\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex c509143c81..c64d9d2f40 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -113,20 +113,20 @@ test_expect_success 'diff --no-index with binary creation' '\n '\n\n cat >expect < 1026 bytes\n- textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+ binfilë | Bin 0 -> 1026 bytes\n+ tëxtfilë | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n\n test_expect_success 'diff --stat with binary files and big change count' '\n-\tprintf \"\\01\\00%1024d\" 1 >binfile &&\n-\tgit add binfile &&\n+\tprintf \"\\01\\00%1024d\" 1 >binfilë &&\n+\tgit add binfilë &&\n \ti=0 &&\n \twhile test $i -lt 10000; do\n \t\techo $i &&\n \t\ti=$(($i + 1)) || return 1\n-\tdone >textfile &&\n-\tgit add textfile &&\n-\tgit diff --cached --stat binfile textfile >output &&\n+\tdone >tëxtfilë &&\n+\tgit add tëxtfilë &&\n+\tgit -c core.quotepath=false diff --cached --stat binfilë tëxtfilë >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n '\n--\n2.34.0\n\n"},{"id":"462509","messageId":"20220902042133.13883-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v3 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-09-02T04:21:33Z","receivedAt":"2022-09-02T04:21:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nA side note: the original report was about cyrillic filenames.\nAfter some investigations it turned out that\na) This is not a problem with \"ambiguous characters\" in unicode\nb) The same problem exists for all unicode code points (so we\n can use Latin based Umlauts for demonstrations below)\n\nThe 'Ä' takes the same space on the screen as the 'A'.\nBut needs one more byte in memory, so the the `git log --stat` output\nfor \"Arger.txt\" (!) gets mis-aligned:\nThe maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\nfor aligment, it needs 9 bytes in memory.\nIf there was a file \"Ö\", it would be correctly aligned by chance,\nbut \"Öhö\" would not.\n\nThe solution is of course, to use utf8_strwidth() instead of strlen()\nwhen dealing with the width on screen.\n\nSide note 1:\nNeeded changes for this fix are split into 2 commits:\nThis commit only changes strlen() into utf8_strwidth() in diff.c:\nThe next commit will add tests and further needed changes.\n\nSide note 2:\nJunio C Hamano suspects that there is probably more work to be done,\nin a separate commit:\nCode in diff.c::pprint_rename() that \"abbreviates\" overly long pathnames\nand \"transforms\" renames lines like\n\"a/b/c -> a/B/c\" into the shorter\n\"a/{b->B}/c\" form, and IIRC this is all byte based.\n\nReported-by: Alexander Meshcheryakov \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 974626a621..b5df464de5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\tfill_print_name(file);\n-\t\tlen = strlen(file->print_name);\n+\t\tlen = utf8_strwidth(file->print_name);\n \t\tif (max_len < len)\n \t\t\tmax_len = len;\n\n@@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t * \"scale\" the filename\n \t\t */\n \t\tlen = name_width;\n-\t\tname_len = strlen(name);\n+\t\tname_len = utf8_strwidth(name);\n \t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n--\n2.34.0\n\n"},{"id":"462521","messageId":"9o5o2rqs-r7q4-p22r-0oss-1n09por2n248@tzk.qr","threadId":"58282","inReplyTo":"20220902042133.13883-1-tboegi@web.de","subject":"Re: [PATCH v3 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T09:39:20Z","receivedAt":"2022-09-02T09:39:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Fri, 2 Sep 2022, tboegi@web.de wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 974626a621..b5df464de5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tfill_print_name(file);\n> -\t\tlen = strlen(file->print_name);\n> +\t\tlen = utf8_strwidth(file->print_name);\n\nSo this is no longer a length (in bytes) but a width (in columns).\n\nIn 2/2, a similar change incurs renaming `max_len` to `max_width`.\n\nI would prefer for 1/2 and 2/2 to be on the same page here: either they\nboth rename variables that have `len` in their name but are actually about\na width (in columns), or neither of the patches rename these variables.\n\nThanks,\nDscho\n\n> \t\tif (max_len < len)\n> \t\t\tmax_len = len;\n>\n> @@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t * \"scale\" the filename\n> \t\t */\n> \t\tlen = name_width;\n> -\t\tname_len = strlen(name);\n> +\t\tname_len = utf8_strwidth(name);\n> \t\tif (name_width < name_len) {\n> \t\t\tchar *slash;\n> \t\t\tprefix = \"...\";\n> --\n> 2.34.0\n>\n>\n"},{"id":"462522","messageId":"8p9rs98o-o802-569o-n59r-07orq1690182@tzk.qr","threadId":"58282","inReplyTo":"20220829175425.cmbwtqpxrq4ppnnk@tb-raspi4","subject":"Re: [PATCH v2 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T09:47:00Z","receivedAt":"2022-09-02T09:47:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Mon, 29 Aug 2022, Torsten Bögershausen wrote:\n\n> On Mon, Aug 29, 2022 at 02:04:42PM +0200, Johannes Schindelin wrote:\n> > >\n> > > The choosen solution is to split code in diff.c like this\n> > >\n> > > strbuf_addf(&out, \"%-*s\", len, name);\n> > >\n> > > into something like this:\n> > >\n> > > size_t num_padding_spaces = 0;\n> > > // [snip]\n> > > if (len > utf8_strwidth(name))\n> > > num_padding_spaces = len - utf8_strwidth(name);\n> > > strbuf_addf(&out, \"%s\", name);\n> > > if (num_padding_spaces)\n> > > strbuf_addchars(&out, ' ', num_padding_spaces);\n> >\n> > ... this sounds like it would benefit from beinv refactored into a\n> > separate function, e.g. `strbuf_add_padded(buf, utf8string)`, both for\n> > readability as well as for self-documentation.\n>\n> Yes, but:\n> All (tm) strbuf() functions use an unsigned size_t, and are not\n> tolerant against passing 0 as \"do nothing\".\n\nI am missing something, as this seems not to contradict the idea of\n`strbuf_add_padded()`. Simply provide the desired width as a `size_t`,\ncompare the width of the actual added string, and if it is shorter, pad\nwith spaces. At no stage does this require a signed type, all involved\nvalues are strictly non-negative.\n\n> >\n> > Also, it is unclear to me why we have to evaluate `utf8_strwidth()`\n> > _twice_ and why we do not assign the result to a variable called `width`\n> > and then have a conditional like\n> >\n> > \tif (width < len) /* pad to `len` columns */\n> > \t\tstrbuf_addchars(&out, ' ' , len - width);\n> >\n> > instead. That would sound more logical to me.\n>\n> This is caused by the logic in diff.c:\n> /*\n> * Find the longest filename and max number of changes\n> */\n> for (i = 0; (i < count) && (i < data->nr); i++) {\n> struct diffstat_file *file = data->files[i];\n> [snip]\n> len = utf8_strwidth(file->print_name);\n> if (max_width < len)\n> max_width = len;\n> // and later\n> /*\n> * From here name_width is the width of the name area,\n> * and graph_width is the width of the graph area.\n> * max_change is used to scale graph properly.\n> */\n> for (i = 0; i < count; i++) {\n> /*\n> * \"scale\" the filename\n> */\n> // TB: Which means either shortening it with ...\n> // Or padding it, if needed, and here we need\n> // another\n> name_len = utf8_strwidth(name);\n\nI was referring to this part of the commit message:\n\n\tif (len > utf8_strwidth(name))\n\t\tnum_padding_spaces = len - utf8_strwidth(name);\n\nHere, we evaluate `utf8_strwidth(name)`, compare it to `len`, and if the\nformer was smaller, we evaluate the same function call _again_.\n\nWhat my feedback intended to suggest was to store the result and reuse it:\n\n\tname_width = utf8_strwidth(name);\n\tif (name_width < len)\n\t\tnum_padding_spaces = len - name_width;\n\nCiao,\nDscho\n"},{"id":"462523","messageId":"00059orr-6p52-q0ro-306r-s225561s2912@tzk.qr","threadId":"58282","inReplyTo":"20220902042138.13901-1-tboegi@web.de","subject":"Re: [PATCH v3 2/2] diff.c: More changes and tests around utf8_strwidth()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T10:12:14Z","receivedAt":"2022-09-02T10:13:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Fri, 2 Sep 2022, tboegi@web.de wrote:\n\n> diff --git a/diff.c b/diff.c\n> index b5df464de5..cf38e1dc88 100644\n> --- a/diff.c\n> +++ b/diff.c\n> [...]\n> @@ -2753,10 +2754,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tif (slash)\n> \t\t\t\tname = slash;\n> \t\t}\n> +\t\tif (len > utf8_strwidth(name))\n> +\t\t\tnum_padding_spaces = len - utf8_strwidth(name);\n\nHere, we determine how many spaces are needed for padding. The value is\nlater used in three instances, and from the diff it is not immediately\nobvious that all code paths are covered. I did verify locally that this is\nthe case, though, so all is good.\n\n>\n> \t\tif (file->is_binary) {\n> -\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n\nThis was already a bit wasteful by calling `strbuf_addf()` twice, where\none time would have sufficed. (This applies to the other two code paths\nbelow, too.)\n\n> +\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\t\tif (num_padding_spaces)\n> +\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\t\tstrbuf_addf(&out, \"| %*s\", number_width, \"Bin\");\n\nInstead of fixing this, we now add yet another `strbuf*()` call.\n\nBut this could be done more elegantly, via a single `strbuf_addf()` call:\n\n\t\t\tstrbuf_addf(&out, \"%s%s%*s | %*s\",\n\t\t\t\t prefix, name, num_padding_spaces, \"\",\n\t\t\t\t number_width, \"Bin\");\n\nBy the way, it would flow much better, I think, if we used the\nshort-and-sweet variable name `padding` instead of `num_padding_spaces`.\n\n> \t\t\tif (!added && !deleted) {\n> \t\t\t\tstrbuf_addch(&out, '\\n');\n> \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n> @@ -2776,8 +2781,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\telse if (file->is_unmerged) {\n> -\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n> +\t\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\t\tif (num_padding_spaces)\n> +\t\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\t\tstrbuf_addstr(&out, \"| Unmerged\\n\");\n\nThis can become\n\n\t\t\tstrbuf_addf(&out, \" %s%s%*s | Unmerged\",\n\t\t\t\t prefix, name, padding, \"\");\n\ninstead.\n\n> \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n> \t\t\t\t\t out.buf, out.len, 0);\n> \t\t\tstrbuf_reset(&out);\n> @@ -2803,8 +2810,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\t\tadd = total - del;\n> \t\t\t}\n> \t\t}\n> -\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n> -\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n> +\t\tstrbuf_addf(&out, \" %s%s \", prefix, name);\n> +\t\tif (num_padding_spaces)\n> +\t\t\tstrbuf_addchars(&out, ' ', num_padding_spaces);\n> +\t\tstrbuf_addf(&out, \"| %*\"PRIuMAX\"%s\",\n> \t\t\tnumber_width, added + deleted,\n> \t\t\tadded + deleted ? \" \" : \"\");\n\nAnd this reads better as\n\n\t\tstrbuf_addf(&out, \" %s%s%*s | %*\"PRIuMAX\"%s\",\n\t\t\t prefix, name, padding, \"\",\n\t\t\t number_width, added + deleted,\n\t\t\t added + deleted ? \" \" : \"\");\n\nIf we modify the code in this manner, we avoid repeating a pretty\nunreadable pattern three times, using a much more readable pattern\ninstead.\n\nRandom note: The existing code (not your fault) is hard to follow because\nit calls `show_graph()` for `add` and `del` always, even if their counts\nare zero (in which case `show_graph()` returns early), while the\nseparating space is appended in the otherwise unrelated `strbuf_addf()`\ncall before that, but uses the (unscaled) `added + deleted` as condition\nfor that separator. It would be much easier to follow like this:\n\n\t\tstrbuf_addf(&out, \" %s%s%*s | %*\"PRIuMAX\",\n\t\t\t prefix, name, padding, \"\",\n\t\t\t number_width, added + deleted);\n\n\t\tif (add || del) {\n\t\t\tstrbuf_addch(&out, ' ');\n\t\t\tshow_graph(&out, '+', add, add_c, reset);\n\t\t\tshow_graph(&out, '-', del, del_c, reset);\n\t\t}\n\nBut I consider this #leftoverbits, not something to burden your\ncontribution with.\n\nCiao,\nDscho\n\n> \t\tshow_graph(&out, '+', add, add_c, reset);\n"},{"id":"462573","messageId":"20220903053931.15611-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-09-03T05:39:31Z","receivedAt":"2022-09-03T05:39:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nA side note: the original report was about cyrillic filenames.\nAfter some investigations it turned out that\na) This is not a problem with \"ambiguous characters\" in unicode\nb) The same problem exists for all unicode code points (so we\n can use Latin based Umlauts for demonstrations below)\n\nThe 'Ä' takes the same space on the screen as the 'A'.\nBut needs one more byte in memory, so the the `git log --stat` output\nfor \"Arger.txt\" (!) gets mis-aligned:\nThe maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\nfor aligment, it needs 9 bytes in memory.\nIf there was a file \"Ö\", it would be correctly aligned by chance,\nbut \"Öhö\" would not.\n\nThe solution is of course, to use utf8_strwidth() instead of strlen()\nwhen dealing with the width on screen.\n\nSide note 1:\nNeeded changes for this fix are split into 2 commits:\nThis commit only changes strlen() into utf8_strwidth() in diff.c:\nThe next commit will add tests and further needed changes.\n\nSide note 2:\nJunio C Hamano suspects that there is probably more work to be done,\nin a separate commit:\nCode in diff.c::pprint_rename() that \"abbreviates\" overly long pathnames\nand \"transforms\" renames lines like\n\"a/b/c -> a/B/c\" into the shorter\n\"a/{b->B}/c\" form, and IIRC this is all byte based.\n\nReported-by: Alexander Meshcheryakov \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 974626a621..b5df464de5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\tfill_print_name(file);\n-\t\tlen = strlen(file->print_name);\n+\t\tlen = utf8_strwidth(file->print_name);\n \t\tif (max_len < len)\n \t\t\tmax_len = len;\n\n@@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t * \"scale\" the filename\n \t\t */\n \t\tlen = name_width;\n-\t\tname_len = strlen(name);\n+\t\tname_len = utf8_strwidth(name);\n \t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n--\n2.34.0\n\n"},{"id":"462574","messageId":"20220903053934.15629-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v4 2/2] diff.c: More changes and tests around utf8_strwidth()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-09-03T05:39:34Z","receivedAt":"2022-09-03T05:39:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen(filename).\n\nFor example, `git log --stat` may produce an output like this:\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nThe last commit uses utf8_strwidth() instead of strlen() in diff.c\nand it is time to test the change.\n\nAnd now we detect another problem that is fixed here: code like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n(or using the underlying snprintf() function) does not align the\nbuffer to a minimum of len measured in screen-width, but uses the\nmemory count.\n\nOne could be tempted to wish that snprintf() was UTF-8 aware.\nThat doesn't seem to be the case anywhere (tested on Linux and Mac),\nprobably snprintf() uses the \"bytes in memory\"/strlen() approach to be\ncompatible with older versions and this will never change.\n\nThe basic idea is to change code in diff.c like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n\ninto something like this:\nint padding = len - utf8_strwidth(name);\nif (padding < 0)\n\tpadding = 0;\nstrbuf_addf(&out, \" %s%*s\", name, padding, \"\");\n\nThe real change is slighty bigger, as it, as well, integrates two calls\nof strbuf_addf() into one.\n\nTests:\nTwo things need to be tested:\n - The calculation of the maximum width\n - The calculation of padding\n\nThe name \"textfile\" is changed into \"tëxtfilë\", both have a width of 8.\nIf strlen() was used, to get the maximum width, the shorter \"binfile\" would\nhave been mis-aligned:\n binfile | [snip]\n tëxtfilë | [snip]\n\nIf only \"binfile\" would be renamed into \"binfilë\":\n binfilë | [snip]\n textfile | [snip]\n\nIn order to verify that the width is calculated correctly everywhere,\n\"binfile\" is renamed into \"binfilë\", giving 1 bytes more in strlen()\n\"tëxtfile\" is renamed into \"tëxtfilë\", 2 byte more in strlen().\n\nThe updated t4012-diff-binary.sh checks the correct aligment:\n binfilë | [snip]\n tëxtfilë | [snip]\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 23 ++++++++++++++---------\n t/t4012-diff-binary.sh | 14 +++++++-------\n 2 files changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b5df464de5..35b9da90fe 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2734,7 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n-\t\tint name_len;\n+\t\tint name_len, padding;\n\n \t\tif (!file->is_interesting && (added + deleted == 0))\n \t\t\tcontinue;\n@@ -2753,10 +2753,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n+\t\tpadding = len - utf8_strwidth(name);\n+\t\tif (padding < 0)\n+\t\t\tpadding = 0;\n\n \t\tif (file->is_binary) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n+\t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n+\t\t\t\t prefix, name, padding, \"\",\n+\t\t\t\t number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n \t\t\t\tstrbuf_addch(&out, '\\n');\n \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n@@ -2776,8 +2780,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n+\t\t\t\t prefix, name, padding, \"\",\n+\t\t\t\t number_width, \"Unmerged\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\n@@ -2803,10 +2808,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n-\t\t\tnumber_width, added + deleted,\n-\t\t\tadded + deleted ? \" \" : \"\");\n+\t\tstrbuf_addf(&out, \" %s%s%*s | %*\"PRIuMAX\"%s\",\n+\t\t\t prefix, name, padding, \"\",\n+\t\t\t number_width, added + deleted,\n+\t\t\t added + deleted ? \" \" : \"\");\n \t\tshow_graph(&out, '+', add, add_c, reset);\n \t\tshow_graph(&out, '-', del, del_c, reset);\n \t\tstrbuf_addch(&out, '\\n');\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex c509143c81..c64d9d2f40 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -113,20 +113,20 @@ test_expect_success 'diff --no-index with binary creation' '\n '\n\n cat >expect < 1026 bytes\n- textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+ binfilë | Bin 0 -> 1026 bytes\n+ tëxtfilë | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n\n test_expect_success 'diff --stat with binary files and big change count' '\n-\tprintf \"\\01\\00%1024d\" 1 >binfile &&\n-\tgit add binfile &&\n+\tprintf \"\\01\\00%1024d\" 1 >binfilë &&\n+\tgit add binfilë &&\n \ti=0 &&\n \twhile test $i -lt 10000; do\n \t\techo $i &&\n \t\ti=$(($i + 1)) || return 1\n-\tdone >textfile &&\n-\tgit add textfile &&\n-\tgit diff --cached --stat binfile textfile >output &&\n+\tdone >tëxtfilë &&\n+\tgit add tëxtfilë &&\n+\tgit -c core.quotepath=false diff --cached --stat binfilë tëxtfilë >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n '\n--\n2.34.0\n\n"},{"id":"462622","messageId":"772s47o8-o5r2-0s9p-5681-9ooo9r144p57@tzk.qr","threadId":"58282","inReplyTo":"20220903053934.15629-1-tboegi@web.de","subject":"Re: [PATCH v4 2/2] diff.c: More changes and tests around utf8_strwidth()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-05T10:13:03Z","receivedAt":"2022-09-05T10:15:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nthank you for working on a new iteration!\n\nOn Sat, 3 Sep 2022, tboegi@web.de wrote:\n\n> [...]\n> diff --git a/diff.c b/diff.c\n> index b5df464de5..35b9da90fe 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2734,7 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\tchar *name = file->print_name;\n> \t\tuintmax_t added = file->added;\n> \t\tuintmax_t deleted = file->deleted;\n> -\t\tint name_len;\n> +\t\tint name_len, padding;\n\nI had a look and `len` is also declard as an `int`.\n\n>\n> \t\tif (!file->is_interesting && (added + deleted == 0))\n> \t\t\tcontinue;\n> @@ -2753,10 +2753,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tif (slash)\n> \t\t\t\tname = slash;\n> \t\t}\n> +\t\tpadding = len - utf8_strwidth(name);\n> +\t\tif (padding < 0)\n> +\t\t\tpadding = 0;\n\nI would have had a slight preference for something like this:\n\n\t\tint name_len = utf8_strwidth(name);\n\t\tint padding = name_len < len ? len - name_len : 0;\n\ni.e. avoid the potentially negative difference. (Ideally, I would have\nliked the type to be changed to `size_t`, but that is impractical due to\nthe variables' use in `%.*s` formats.)\n\nBut it is not worth a new iteration on its own, and I am very happy with\nthe current iteration.\n\nThanks!\nDscho\n"},{"id":"462645","messageId":"xmqqv8q1zgzi.fsf@gitster.g","threadId":"58282","inReplyTo":"20220903053931.15611-1-tboegi@web.de","subject":"Re: [PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-05T20:46:57Z","receivedAt":"2022-09-05T20:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n> Subject: Re: [PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1\n\nGiven 2/2 does not share a similar title, \"part1\" sounds somewhat\nstrange. In any case, 'when appropriate,' is probalby best unsaid,\nas it is almost a given. We won't deliberately use something that\nis not appropriate on purpose anyway. Even if we =were to keep that\nword, downcase \"When\".\n\n\n> When unicode filenames (encoded in UTF-8) are used, the visible width\n> on the screen is not the same as strlen(filename).\n>\n> For example, `git log --stat` may produce an output like this:\n>\n> [snip the header]\n>\n> Arger.txt | 1 +\n> Ärger.txt | 1 +\n> 2 files changed, 2 insertions(+)\n>\n> A side note: the original report was about cyrillic filenames.\n> After some investigations it turned out that\n> a) This is not a problem with \"ambiguous characters\" in unicode\n> b) The same problem exists for all unicode code points (so we\n> can use Latin based Umlauts for demonstrations below)\n>\n> The 'Ä' takes the same space on the screen as the 'A'.\n> But needs one more byte in memory, so the the `git log --stat` output\n> for \"Arger.txt\" (!) gets mis-aligned:\n> The maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n> 9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\n> for aligment, it needs 9 bytes in memory.\n> If there was a file \"Ö\", it would be correctly aligned by chance,\n> but \"Öhö\" would not.\n>\n> The solution is of course, to use utf8_strwidth() instead of strlen()\n> when dealing with the width on screen.\n>\n> Side note 1:\n> Needed changes for this fix are split into 2 commits:\n> This commit only changes strlen() into utf8_strwidth() in diff.c:\n> The next commit will add tests and further needed changes.\n\nI am not sure if it makes sense to split them into two. It is hard\nfor us to demonistrate the need for this step if it does not come\nwith its own test.\n\n> Side note 2:\n> Junio C Hamano suspects that there is probably more work to be done,\n> in a separate commit:\n> Code in diff.c::pprint_rename() that \"abbreviates\" overly long pathnames\n> and \"transforms\" renames lines like\n> \"a/b/c -> a/B/c\" into the shorter\n> \"a/{b->B}/c\" form, and IIRC this is all byte based.\n\nI already said that I suspect {b->B} conversion is OK, so the side\nnote is probably more noise than being useful.\n>\n> Reported-by: Alexander Meshcheryakov \n> Signed-off-by: Torsten Bögershausen \n> ---\n> diff.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 974626a621..b5df464de5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tfill_print_name(file);\n> -\t\tlen = strlen(file->print_name);\n> +\t\tlen = utf8_strwidth(file->print_name);\n> \t\tif (max_len < len)\n> \t\t\tmax_len = len;\n>\n> @@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t * \"scale\" the filename\n> \t\t */\n> \t\tlen = name_width;\n> -\t\tname_len = strlen(name);\n> +\t\tname_len = utf8_strwidth(name);\n> \t\tif (name_width < name_len) {\n> \t\t\tchar *slash;\n> \t\t\tprefix = \"...\";\n> --\n> 2.34.0\n"},{"id":"462682","messageId":"20220907043040.idqqivi3jt35jyst@tb-raspi4","threadId":"58282","inReplyTo":"xmqqv8q1zgzi.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-09-07T04:30:40Z","receivedAt":"2022-09-07T04:30:52Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Sep 05, 2022 at 01:46:57PM -0700, Junio C Hamano wrote:\n> tboegi@web.de writes:\n>\n> > From: Torsten Bögershausen \n> > Subject: Re: [PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1\n>\n> Given 2/2 does not share a similar title, \"part1\" sounds somewhat\n> strange. In any case, 'when appropriate,' is probalby best unsaid,\n> as it is almost a given. We won't deliberately use something that\n> is not appropriate on purpose anyway. Even if we =were to keep that\n> word, downcase \"When\".\n\nYes, agreed. In short: I will make a new patch the next weeks,\nin one commit (again). (That can take some days or weeks)\n\nThanks to Dscho for his patience with the strbuf() improvements.\nI think that I tried a \"%*s\" version, but couldn't get that to work.\n\n> > Side note 2:\n> > Junio C Hamano suspects that there is probably more work to be done,\n> > in a separate commit:\n> > Code in diff.c::pprint_rename() that \"abbreviates\" overly long pathnames\n> > and \"transforms\" renames lines like\n> > \"a/b/c -> a/B/c\" into the shorter\n> > \"a/{b->B}/c\" form, and IIRC this is all byte based.\n>\n> I already said that I suspect {b->B} conversion is OK, so the side\n> note is probably more noise than being useful.\n\nOK - the comment can be removed.\n\nI didn't know how to read this comment:\n>...but the former may chomp a single multi-byte letter in the middle,\n> which would need to be corrected as a part of this change.\n\nAfter diffing into the code some more times, I think that we don't\nchomp a single byte out of an UTF-8 sequence.\n"},{"id":"462745","messageId":"xmqqedwnujce.fsf@gitster.g","threadId":"58282","inReplyTo":"20220907043040.idqqivi3jt35jyst@tb-raspi4","subject":"Re: [PATCH v4 1/2] diff.c: When appropriate, use utf8_strwidth(), part1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T18:31:45Z","receivedAt":"2022-09-07T18:31:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>\n> OK - the comment can be removed.\n>\n> I didn't know how to read this comment:\n>>...but the former may chomp a single multi-byte letter in the middle,\n>> which would need to be corrected as a part of this change.\n>\n> After diffing into the code some more times, I think that we don't\n> chomp a single byte out of an UTF-8 sequence.\n\nWhen turning a/b/c vs a/B/c into a/{b->B}/c, two steps are involved.\nTake common prefix and suffix (in this case 'a' and 'c') and turn\n'b' vs 'B' into {b->B} is one step. The other is what to do when\nprefix and suffix are long. After turning aaaaa/b/c vs aaaaa/B/c\ninto aaaaa/{b->B}/c, if the result is overly long, how we shorten\nthe prefix (i.e. aaaaa) and the suffix?\n\nI knew the code that produces {b->B} honored '/' boundary, but I\njust did not remember offhand what diff.c::pprint_rename() did in\nits latter half, specifically, if it just chomped pfx and sfx as a\nsequence of bytes (which would have been wrong) or insisted that the\ncommon sequence search honors '/' boundary (which would be OK, as\nbyte '/' will not appear in the middle of a single multi-byte UTF-8\n\"letter\"). I think iti s doing the latter, so it should be fine.\n\n\n\nThanks.\n"},{"id":"463017","messageId":"20220914151333.3309-1-tboegi@web.de","threadId":"58282","inReplyTo":"CA+VDVVVmi99i6ZY64tg8RkVXDc5gOzQP_SH12zhDKRkUnhWFgw@mail.gmail.com","subject":"[PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2022-09-14T15:13:33Z","receivedAt":"2022-09-14T15:14:07Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen unicode filenames (encoded in UTF-8) are used, the visible width\non the screen is not the same as strlen().\n\nFor example, `git log --stat` may produce an output like this:\n\n[snip the header]\n\n Arger.txt | 1 +\n Ärger.txt | 1 +\n 2 files changed, 2 insertions(+)\n\nA side note: the original report was about cyrillic filenames.\nAfter some investigations it turned out that\na) This is not a problem with \"ambiguous characters\" in unicode\nb) The same problem exists for all unicode code points (so we\n can use Latin based Umlauts for demonstrations below)\n\nThe 'Ä' takes the same space on the screen as the 'A'.\nBut needs one more byte in memory, so the the `git log --stat` output\nfor \"Arger.txt\" (!) gets mis-aligned:\nThe maximum length is derived from \"Ärger.txt\", 10 bytes in memory,\n9 positions on the screen. That is why \"Arger.txt\" gets one extra ' '\nfor aligment, it needs 9 bytes in memory.\nIf there was a file \"Ö\", it would be correctly aligned by chance,\nbut \"Öhö\" would not.\n\nThe solution is of course, to use utf8_strwidth() instead of strlen()\nwhen dealing with the width on screen.\n\nAnd then there is another problem, code like this:\nstrbuf_addf(&out, \"%-*s\", len, name);\n(or using the underlying snprintf() function) does not align the\nbuffer to a minimum of len measured in screen-width, but uses the\nmemory count.\n\nOne could be tempted to wish that snprintf() was UTF-8 aware.\nThat doesn't seem to be the case anywhere (tested on Linux and Mac),\nprobably snprintf() uses the \"bytes in memory\"/strlen() approach to be\ncompatible with older versions and this will never change.\n\nThe basic idea is to change code in diff.c like this\nstrbuf_addf(&out, \"%-*s\", len, name);\n\ninto something like this:\nint padding = len - utf8_strwidth(name);\nif (padding < 0)\n\tpadding = 0;\nstrbuf_addf(&out, \" %s%*s\", name, padding, \"\");\n\nThe real change is slighty bigger, as it, as well, integrates two calls\nof strbuf_addf() into one.\n\nTests:\nTwo things need to be tested:\n - The calculation of the maximum width\n - The calculation of padding\n\nThe name \"textfile\" is changed into \"tëxtfilë\", both have a width of 8.\nIf strlen() was used, to get the maximum width, the shorter \"binfile\" would\nhave been mis-aligned:\n binfile | [snip]\n tëxtfilë | [snip]\n\nIf only \"binfile\" would be renamed into \"binfilë\":\n binfilë | [snip]\n textfile | [snip]\n\nIn order to verify that the width is calculated correctly everywhere,\n\"binfile\" is renamed into \"binfilë\", giving 1 bytes more in strlen()\n\"tëxtfile\" is renamed into \"tëxtfilë\", 2 byte more in strlen().\n\nThe updated t4012-diff-binary.sh checks the correct aligment:\n binfilë | [snip]\n tëxtfilë | [snip]\n\nReported-by: Alexander Meshcheryakov \nHelped-by: Johannes Schindelin \nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 27 ++++++++++++++++-----------\n t/t4012-diff-binary.sh | 14 +++++++-------\n 2 files changed, 23 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 974626a621..35b9da90fe 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\tfill_print_name(file);\n-\t\tlen = strlen(file->print_name);\n+\t\tlen = utf8_strwidth(file->print_name);\n \t\tif (max_len < len)\n \t\t\tmax_len = len;\n\n@@ -2734,7 +2734,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tchar *name = file->print_name;\n \t\tuintmax_t added = file->added;\n \t\tuintmax_t deleted = file->deleted;\n-\t\tint name_len;\n+\t\tint name_len, padding;\n\n \t\tif (!file->is_interesting && (added + deleted == 0))\n \t\t\tcontinue;\n@@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t * \"scale\" the filename\n \t\t */\n \t\tlen = name_width;\n-\t\tname_len = strlen(name);\n+\t\tname_len = utf8_strwidth(name);\n \t\tif (name_width < name_len) {\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n@@ -2753,10 +2753,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tif (slash)\n \t\t\t\tname = slash;\n \t\t}\n+\t\tpadding = len - utf8_strwidth(name);\n+\t\tif (padding < 0)\n+\t\t\tpadding = 0;\n\n \t\tif (file->is_binary) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n+\t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n+\t\t\t\t prefix, name, padding, \"\",\n+\t\t\t\t number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n \t\t\t\tstrbuf_addch(&out, '\\n');\n \t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n@@ -2776,8 +2780,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\tstrbuf_addf(&out, \" %s%s%*s | %*s\",\n+\t\t\t\t prefix, name, padding, \"\",\n+\t\t\t\t number_width, \"Unmerged\");\n \t\t\temit_diff_symbol(options, DIFF_SYMBOL_STATS_LINE,\n \t\t\t\t\t out.buf, out.len, 0);\n \t\t\tstrbuf_reset(&out);\n@@ -2803,10 +2808,10 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addf(&out, \" %s%-*s |\", prefix, len, name);\n-\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n-\t\t\tnumber_width, added + deleted,\n-\t\t\tadded + deleted ? \" \" : \"\");\n+\t\tstrbuf_addf(&out, \" %s%s%*s | %*\"PRIuMAX\"%s\",\n+\t\t\t prefix, name, padding, \"\",\n+\t\t\t number_width, added + deleted,\n+\t\t\t added + deleted ? \" \" : \"\");\n \t\tshow_graph(&out, '+', add, add_c, reset);\n \t\tshow_graph(&out, '-', del, del_c, reset);\n \t\tstrbuf_addch(&out, '\\n');\ndiff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\nindex c509143c81..c64d9d2f40 100755\n--- a/t/t4012-diff-binary.sh\n+++ b/t/t4012-diff-binary.sh\n@@ -113,20 +113,20 @@ test_expect_success 'diff --no-index with binary creation' '\n '\n\n cat >expect < 1026 bytes\n- textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+ binfilë | Bin 0 -> 1026 bytes\n+ tëxtfilë | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n\n test_expect_success 'diff --stat with binary files and big change count' '\n-\tprintf \"\\01\\00%1024d\" 1 >binfile &&\n-\tgit add binfile &&\n+\tprintf \"\\01\\00%1024d\" 1 >binfilë &&\n+\tgit add binfilë &&\n \ti=0 &&\n \twhile test $i -lt 10000; do\n \t\techo $i &&\n \t\ti=$(($i + 1)) || return 1\n-\tdone >textfile &&\n-\tgit add textfile &&\n-\tgit diff --cached --stat binfile textfile >output &&\n+\tdone >tëxtfilë &&\n+\tgit add tëxtfilë &&\n+\tgit -c core.quotepath=false diff --cached --stat binfilë tëxtfilë >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n '\n--\n2.34.0\n\n"},{"id":"463020","messageId":"xmqqpmfx52qj.fsf@gitster.g","threadId":"58282","inReplyTo":"20220914151333.3309-1-tboegi@web.de","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-14T16:40:04Z","receivedAt":"2022-09-14T16:40:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> The basic idea is to change code in diff.c like this\n> strbuf_addf(&out, \"%-*s\", len, name);\n>\n> into something like this:\n> int padding = len - utf8_strwidth(name);\n> if (padding < 0)\n> \tpadding = 0;\n> strbuf_addf(&out, \" %s%*s\", name, padding, \"\");\n> ...\n> Reported-by: Alexander Meshcheryakov \n> Helped-by: Johannes Schindelin \n> Signed-off-by: Torsten Bögershausen \n> ---\n> diff.c | 27 ++++++++++++++++-----------\n> t/t4012-diff-binary.sh | 14 +++++++-------\n> 2 files changed, 23 insertions(+), 18 deletions(-)\n\n> diff --git a/diff.c b/diff.c\n> index 974626a621..35b9da90fe 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2620,7 +2620,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tfill_print_name(file);\n> -\t\tlen = strlen(file->print_name);\n> +\t\tlen = utf8_strwidth(file->print_name);\n> \t\tif (max_len < len)\n> \t\t\tmax_len = len;\n\nThe changes in this patch are isolated to the show_stats() helper\nfunction, and looking at use of \"len\", \"max_len\", and \"name_len\", it\nmay be a good clean-up to make them based on \"width\". A bit of care\nneeds to be taken because the way existing variables are used is a\nbit convoluted at times:\n\n - \"width\" already exists. \"len\" and \"max_len\" are used in an early\n loop to eventually derive \"name_width\".\n\n - \"len\" is later used in the loop for each pathname to hold a copy\n of \"name_width\" that can locally be adjusted to accomodate \"...\"\n abbreviation/munging of the pathname.\n\n - \"name_width\" already exists in addition to \"name_len\". The\n former holds how many display columns a pathname can occupy in the\n diffstat output, while the latter is used in a loop to hold the\n display columns of the pathname each iteration is looking at, to\n see if it is wider than \"name_width\" (in which case there is the\n \"...\" abbreviation that is NOT UTF-8 aware even after this patch)\n or narrower (in which case we'd do the padding). As the existing\n \"name_width\" is how we want to name our variables (i.e. the width\n allocated for names), the \"name_len\", if we were to follow \"len\n misleads us to think it is byte length, so use width instead\",\n would need to become something like \"this_name_width\" (i.e. the\n width of the name of the pathname in this iteration of the loop).\n\nBut I am OK to do WITHOUT any such renaming, and I do not want to\nsee such renaming in the same patch (\"preliminary clean-up\" or\n\"clean-up after the dust settles\" are good, thoguh). Counting\ndisplay columns correctly is more important.\n\nI think I spotted two remaining \"bugs\" that are left unfixed with\nthis patch..\n\nThere is \"stat_width is -1 (auto)\" case, which reads like so:\n\n\tif (options->stat_width == -1)\n\t\twidth = term_columns() - strlen(line_prefix);\n\telse\n\t\twidth = options->stat_width ? options->stat_width : 80;\n\nHere line_prefix eventually comes from the \"git log --graph\" and\nshows the colored graph segments on the same output line as the\ndiffstat.\n\nThis patch is probably not making anything worse, but by leaving it\nstrlen(), it is likely overcounting the width of it. We can\npresumably use utf8_strnwidth() that can optionally be told to be\naware of the ANSI color sequence to count its width correctly to fix\nit.\n\n> @@ -2743,7 +2743,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t * \"scale\" the filename\n> \t\t */\n> \t\tlen = name_width;\n> -\t\tname_len = strlen(name);\n> +\t\tname_len = utf8_strwidth(name);\n> \t\tif (name_width < name_len) {\n> \t\t\tchar *slash;\n> \t\t\tprefix = \"...\";\n\nThe code around here between this and the next hunk needs cleaning up.\n\n\t\tif (name_width < name_len) {\n\t\t\tchar *slash;\n\t\t\tprefix = \"...\";\n\t\t\tlen -= 3;\n\t\t\tname += name_len - len;\n\t\t\tslash = strchr(name, '/');\n\t\t\tif (slash)\n\t\t\t\tname = slash;\n\t\t}\n\nWe found the display columns of the current item \"name_len\" is wider\nthan what we allocated \"name_width\" for the names. We are going to\nchomp as many pathname components from the front as needed, at '/'\nboundary, to turn \"aaaa/bbbb/cccc/dddd.txt\" into \".../cccc/dddd.txt\"\nto make the result fit.\n\nBut the way to ensure that '/' before \"cccc\" is the one we want (as\nopposed to the one before \"bbbb\" or \"dddd\") is initially based on\ncolumns (i.e. because we want \"...\", we first subtract 3 from len\nwhich is a local synonym for name_width and then subtract that from\n\"name_len\", i.e. we ask: \"how many display columns do we have in the\ncurrent pathname that is excess of what we can afford to allocate?\"\nThe intention is to skip that many columns from the beginning of \"name\"\nand start looking for '/' from there.\n\nBut we move \"name\" pointer by that many *bytes*! We end up scanning\nstarting at a middle of a character. What we look for is '/' and when\nwe find it we know the byte is a standalone character, so we do not\nchomp a character in the middle, but it is very likely that we find\na slash that leaves the remaining string still too long, because\nskipping say 2 columns may need skipping 4 bytes, but we only\nskipped the same number of bytes as the number of columns we need to\nskip.\n\nThis is the other remaining bug.\n\nI think this needs to become a loop that loops while the width of\nthe current suffix is still wider than we can afford, discarding one\nleading pathname component at a time at '/', measuring the resulting\nwidth, or something like that. Something along the lines of this\nnot-even-compile-tested sketch:\n\n /* we assume strlen(prefix) == utf8_strwidth(prefix) */\n\twhile (name_width < utf8_strwidth(name) + strlen(prefix)) {\n\t\tchar *slash;\n\t\tif (name[0] == '/')\n\t\t\tname++;\n slash = strchr(name);\n\t\tif (slash)\n\t\t\tname = slash;\n\t\telse\n\t\t\tbreak; /* Give Up */\n\t\tprefix = \"...\";\n\t}\n\n> @@ -2753,10 +2753,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tif (slash)\n> \t\t\t\tname = slash;\n> \t\t}\n> +\t\tpadding = len - utf8_strwidth(name);\n> +\t\tif (padding < 0)\n> +\t\t\tpadding = 0;\n\nHere \"len\" cannot become \"name_length\" because the former has to be\nnarrower than the latter by the display width of \"prefix\"\n(i.e. \"...\"), so while this looks \"strange\", it is correct.\n\nI think the remainder of the patch I did not quote looked quite\nstraight-forward and correct.\n\nThanks for working on this topic.\n"},{"id":"463029","messageId":"xmqqmtb1pcos.fsf@gitster.g","threadId":"58282","inReplyTo":"20220914151333.3309-1-tboegi@web.de","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-15T02:57:07Z","receivedAt":"2022-09-15T02:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n> Subject: Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()\n\nLet's retitle it to \"diff.c: use utf8_strwidth() to count display width\".\n"},{"id":"463657","messageId":"20220926184308.5oaaoopod36igq6i@tb-raspi4","threadId":"58282","inReplyTo":"xmqqpmfx52qj.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-09-26T18:43:08Z","receivedAt":"2022-09-26T18:44:55Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Sep 14, 2022 at 09:40:04AM -0700, Junio C Hamano wrote:\n\n[]\n\n> I think I spotted two remaining \"bugs\" that are left unfixed with\n> this patch..\n>\n> There is \"stat_width is -1 (auto)\" case, which reads like so:\n>\n> \tif (options->stat_width == -1)\n> \t\twidth = term_columns() - strlen(line_prefix);\n> \telse\n> \t\twidth = options->stat_width ? options->stat_width : 80;\n>\n> Here line_prefix eventually comes from the \"git log --graph\" and\n> shows the colored graph segments on the same output line as the\n> diffstat.\n>\n> This patch is probably not making anything worse, but by leaving it\n> strlen(), it is likely overcounting the width of it. We can\n> presumably use utf8_strnwidth() that can optionally be told to be\n> aware of the ANSI color sequence to count its width correctly to fix\n> it.\n\n[]\n> This is the other remaining bug.\n\n[]\n\n> I think the remainder of the patch I did not quote looked quite\n> straight-forward and correct.\n>\n> Thanks for working on this topic.\n\nHow should we proceed here ?\nThis patch fixes one, and only one, reported bug,\nwhich is now verfied by a test case using unicode instead of ASCII.\nFixing additional bugs in diff.c (or anywhere else) had never been\npart of this.\n\nThings that needs more fixing and cleanups had been layed out as the\nresult of a review, that is good.\n\n\"git log --graph\" was mentioned.\nDo we have test cases, that test this ?\nHow easy are they converted into unicode instead of ASCII ?\n\nI am not even sure, if I ever used \"git log --graph\" myself.\nDigging further here, is somewhat out of my scope.\nAt least for the moment.\n\n\n\n\n\n"},{"id":"464540","messageId":"xmqq35bv1gu5.fsf@gitster.g","threadId":"58282","inReplyTo":"20220926184308.5oaaoopod36igq6i@tb-raspi4","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T21:58:26Z","receivedAt":"2022-10-10T21:58:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On Wed, Sep 14, 2022 at 09:40:04AM -0700, Junio C Hamano wrote:\n>\n> []\n>\n>> I think I spotted two remaining \"bugs\" that are left unfixed with\n>> this patch..\n>> ...\n> How should we proceed here ?\n> This patch fixes one, and only one, reported bug,\n\nBut then two more were reported in the message you are responding\nto, and they stem from the same underlying logic bug where byte\ncount and display columns are mixed interchangeably.\n\n> \"git log --graph\" was mentioned.\n> Do we have test cases, that test this ?\n> How easy are they converted into unicode instead of ASCII ?\n\nThe graph stuff pushes your \"start of line\" to the right, making the\navailable screen real estate narrower. I do not think in the\ncurrent code we need to worry about unicode vs ascii (IIRC, we stick\nto ASCII graphics while drawing lines), but we do need to take into\naccount the fact that ANSI COLOR escape sequences have non-zero byte\ncount while occupying zero display columns.\n\nThe other bug about the code that finds which / to use to abbreviate\na long pathname on diffstat lines does involve byte vs column that\ncomes from unicode. From the bug description in the message you are\nresponding to, if we have a directory name whose display columns and\nbyte count are significantly different, the end result by chopping\nwith the current code would end up wider than it should be, which\nsounds like a recipe to cook up a test case to me.\n\n"},{"id":"465298","messageId":"20221020154608.jndql5sio3jyii3z@tb-raspi4","threadId":"58282","inReplyTo":"xmqq35bv1gu5.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-10-20T15:46:09Z","receivedAt":"2022-10-20T15:46:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Oct 10, 2022 at 02:58:26PM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> > On Wed, Sep 14, 2022 at 09:40:04AM -0700, Junio C Hamano wrote:\n> >\n> > []\n> >\n> >> I think I spotted two remaining \"bugs\" that are left unfixed with\n> >> this patch..\n> >> ...\n> > How should we proceed here ?\n> > This patch fixes one, and only one, reported bug,\n>\n> But then two more were reported in the message you are responding\n> to, and they stem from the same underlying logic bug where byte\n> count and display columns are mixed interchangeably.\n>\n> > \"git log --graph\" was mentioned.\n> > Do we have test cases, that test this ?\n> > How easy are they converted into unicode instead of ASCII ?\n>\n> The graph stuff pushes your \"start of line\" to the right, making the\n> available screen real estate narrower. I do not think in the\n> current code we need to worry about unicode vs ascii (IIRC, we stick\n> to ASCII graphics while drawing lines), but we do need to take into\n> account the fact that ANSI COLOR escape sequences have non-zero byte\n> count while occupying zero display columns.\n>\n> The other bug about the code that finds which / to use to abbreviate\n> a long pathname on diffstat lines does involve byte vs column that\n> comes from unicode. From the bug description in the message you are\n> responding to, if we have a directory name whose display columns and\n> byte count are significantly different, the end result by chopping\n> with the current code would end up wider than it should be, which\n> sounds like a recipe to cook up a test case to me.\n>\n\n\nI couldn't find how to trigger this code path.\nThe `git log --graph` help says:\n--graph\n Draw a text-based graphical representation of the commit history\n on the left hand side of the output.\n This may cause extra lines to be printed in between commits,\n in order for the graph history to be drawn properly.\n Cannot be combined with --no-walk.\n\nThere is no indication about filenames or diffs in the\nresultet output.\nIf someone has time and knowledge to cook up a test case,\nthat would help.\n\nFor the moment, I don't have enough spare time to spend on digging\nhow to write this test case, that's the sad part of the story.\nAnd that is probably a good start, or, to be more strict,\nan absolute precondition, if I need to change another single line\nin diff.c\n\nI still haven't understood why the current patch can not move forward\non its own ?\nThere is a bug report, patch, a test case that verifies the fix.\n\nWhat more is needed ?\nTo fix all other bugs/issues/limitations in diff.c ?\nIf yes, they need to go in separate commits anyway, or do I miss\nsomething ?\n\nCan we dampen the expectations a little bit ?\n"},{"id":"465308","messageId":"xmqqy1tas85w.fsf@gitster.g","threadId":"58282","inReplyTo":"20221020154608.jndql5sio3jyii3z@tb-raspi4","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-20T17:43:07Z","receivedAt":"2022-10-20T17:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> What more is needed ?\n> To fix all other bugs/issues/limitations in diff.c ?\n> If yes, they need to go in separate commits anyway, or do I miss\n> something ?\n\nAt least leave some NEEDSWORK comment in the code that is known to\nneed more work, to remind others that the fix in the area of the\ncode is not done, perhaps. Otherwise, much of the effort in the\nreview gets lost.\n\nI offhand recall at least two (please go back to the original thread\nto find the details of them). One that measures the width of\nlong/path/name in bytes to determine where to start chomping in the\ndiffstat filename (because it still mixes display columns and\nbytes), and the other one that measures the width of leading graph\nsegment in bytes without ignoring the ANSI color sequence, which\nshould be using utf8_strnwidth() but is using strlen().\n\nhttps://lore.kernel.org/git/xmqqpmfx52qj.fsf@gitster.g/\n\nThanks.\n"},{"id":"465478","messageId":"20221021151909.z3nejpnnt2wmmkry@tb-raspi4","threadId":"58282","inReplyTo":"xmqqy1tas85w.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-10-21T15:19:09Z","receivedAt":"2022-10-21T15:19:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Oct 20, 2022 at 10:43:07AM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> > What more is needed ?\n> > To fix all other bugs/issues/limitations in diff.c ?\n> > If yes, they need to go in separate commits anyway, or do I miss\n> > something ?\n>\n> At least leave some NEEDSWORK comment in the code that is known to\n> need more work, to remind others that the fix in the area of the\n> code is not done, perhaps. Otherwise, much of the effort in the\n> review gets lost.\n>\n> I offhand recall at least two (please go back to the original thread\n> to find the details of them). One that measures the width of\n> long/path/name in bytes to determine where to start chomping in the\n> diffstat filename (because it still mixes display columns and\n> bytes), and the other one that measures the width of leading graph\n> segment in bytes without ignoring the ANSI color sequence, which\n> should be using utf8_strnwidth() but is using strlen().\n>\n> https://lore.kernel.org/git/xmqqpmfx52qj.fsf@gitster.g/\n>\n> Thanks.\n\nGood, good, good.\nFor the moment I don't have any spare time to spend on Git.\nAll your comments are noted, and I hope to get time to address them later.\nIf you kick out the branch from seen and the whats cooking list,\nthat would be fine with me.\n\n"},{"id":"465519","messageId":"xmqq35bgkfde.fsf@gitster.g","threadId":"58282","inReplyTo":"20221021151909.z3nejpnnt2wmmkry@tb-raspi4","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T21:59:09Z","receivedAt":"2022-10-21T21:59:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> For the moment I don't have any spare time to spend on Git.\n> All your comments are noted, and I hope to get time to address them later.\n> If you kick out the branch from seen and the whats cooking list,\n> that would be fine with me.\n\nI'd rather not waste the efforts so far. I am tempted to queue the\nfollowing on top or squash it in.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] diff: leave NEEDWORK notes in show_stats() function\n\nThe previous step made an attempt to correctly compute display\ncolumns allocated and padded different parts of diffstat output.\nThere are at least two known codepaths in the function that still\nmixes up display widths and byte length that need to be fixed.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 2751cae131..1d222d87b2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2675,6 +2675,11 @@ 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 \telse\n@@ -2750,6 +2755,16 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\tchar *slash;\n \t\t\tprefix = \"...\";\n \t\t\tlen -= 3;\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: (name_len - len) counts the display\n+\t\t\t * width, which would be shorter than the byte\n+\t\t\t * length of the corresponding substring.\n+\t\t\t * Advancing \"name\" by that number of bytes does\n+\t\t\t * *NOT* skip over that many columns, so it is\n+\t\t\t * very likely that chomping the pathname at the\n+\t\t\t * slash we will find starting from \"name\" will\n+\t\t\t * leave the resulting string still too long.\n+\t\t\t */\n \t\t\tname += name_len - len;\n \t\t\tslash = strchr(name, '/');\n \t\t\tif (slash)\n-- \n2.38.1-320-g901e6a2134\n\n"},{"id":"465600","messageId":"20221023200222.o6p7d6qor5sygdgb@tb-raspi4","threadId":"58282","inReplyTo":"xmqq35bgkfde.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] diff.c: When appropriate, use utf8_strwidth()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2022-10-23T20:02:22Z","receivedAt":"2022-10-23T20:07:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Oct 21, 2022 at 02:59:09PM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> > For the moment I don't have any spare time to spend on Git.\n> > All your comments are noted, and I hope to get time to address them later.\n> > If you kick out the branch from seen and the whats cooking list,\n> > that would be fine with me.\n>\n> I'd rather not waste the efforts so far. I am tempted to queue the\n> following on top or squash it in.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] diff: leave NEEDWORK notes in show_stats() function\n>\n> The previous step made an attempt to correctly compute display\n> columns allocated and padded different parts of diffstat output.\n> There are at least two known codepaths in the function that still\n> mixes up display widths and byte length that need to be fixed.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 2751cae131..1d222d87b2 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2675,6 +2675,11 @@ 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> \telse\n> @@ -2750,6 +2755,16 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t\tchar *slash;\n> \t\t\tprefix = \"...\";\n> \t\t\tlen -= 3;\n> +\t\t\t/*\n> +\t\t\t * NEEDSWORK: (name_len - len) counts the display\n> +\t\t\t * width, which would be shorter than the byte\n> +\t\t\t * length of the corresponding substring.\n> +\t\t\t * Advancing \"name\" by that number of bytes does\n> +\t\t\t * *NOT* skip over that many columns, so it is\n> +\t\t\t * very likely that chomping the pathname at the\n> +\t\t\t * slash we will find starting from \"name\" will\n> +\t\t\t * leave the resulting string still too long.\n> +\t\t\t */\n> \t\t\tname += name_len - len;\n> \t\t\tslash = strchr(name, '/');\n> \t\t\tif (slash)\n\n\nThat looks good to me -\nmy preferred version would be a patch on it's own on top.\n"}]}