{"thread":{"id":"61227","subject":"[PATCH] advice: omit trailing whitespace","startedAt":"2024-03-29T22:57:12Z","lastAt":"2024-04-01T20:50:20Z","messageCount":9,"participants":["Junio C Hamano","Rubén Justo","Dragan Simic"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491864","messageId":"xmqq4jcooddp.fsf@gitster.g","threadId":"61227","inReplyTo":null,"subject":"[PATCH] advice: omit trailing whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T22:57:06Z","receivedAt":"2024-03-29T22:57:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Git tools all consistently encourage users to avoid whitespaces at\nthe end of line by giving them features like \"git diff --check\" and\n\"git am --whitespace=fix\". Make sure that the advice messages we\ngive users avoid trailing whitespaces. We shouldn't be wasting\nvertical screen real estate by adding blank lines in advice messages\nthat are supposed to be concise hints, but as long as we write such\nblank line in our \"hints\", we should do it right.\n\nA test that expects the current behaviour of leaving trailing\nwhitespaces has been adjusted.\n\nSigned-off-by: Junio C Hamano \n---\n advice.c | 3 ++-\n t/t3200-branch.sh | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git c/advice.c w/advice.c\nindex d19648b7f8..75111191ad 100644\n--- c/advice.c\n+++ w/advice.c\n@@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,\n \n \tfor (cp = buf.buf; *cp; cp = np) {\n \t\tnp = strchrnul(cp, '\\n');\n-\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n+\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n+\t\t\t(np == cp) ? \"\" : \" \",\n \t\t\t(int)(np - cp), cp,\n \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n \t\tif (*np)\ndiff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\nindex d3bbd00b81..ccfa6a720d 100755\n--- c/t/t3200-branch.sh\n+++ w/t/t3200-branch.sh\n@@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' '\n \thint: tracking ref '\\''refs/heads/main'\\'':\n \thint: ambi1\n \thint: ambi2\n-\thint: ''\n+\thint:\n \thint: This is typically a configuration error.\n-\thint: ''\n+\thint:\n \thint: To support setting up tracking branches, ensure that\n \thint: different remotes'\\'' fetch refspecs map into different\n \thint: tracking namespaces.\n"},{"id":"491865","messageId":"fc002d62-6efe-42d4-b562-c10d3419fff6@gmail.com","threadId":"61227","inReplyTo":"xmqq4jcooddp.fsf@gitster.g","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T23:23:41Z","receivedAt":"2024-03-29T23:23:47Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 29, 2024 at 03:57:06PM -0700, Junio C Hamano wrote:\n> Git tools all consistently encourage users to avoid whitespaces at\n> the end of line by giving them features like \"git diff --check\" and\n> \"git am --whitespace=fix\". Make sure that the advice messages we\n> give users avoid trailing whitespaces. We shouldn't be wasting\n> vertical screen real estate by adding blank lines in advice messages\n> that are supposed to be concise hints, but as long as we write such\n> blank line in our \"hints\", we should do it right.\n> \n> A test that expects the current behaviour of leaving trailing\n> whitespaces has been adjusted.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n\nYeah, ACK. This is obviously a good thing. I'll base my other series\nin this. Thanks.\n\n> advice.c | 3 ++-\n> t/t3200-branch.sh | 4 ++--\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git c/advice.c w/advice.c\n> index d19648b7f8..75111191ad 100644\n> --- c/advice.c\n> +++ w/advice.c\n> @@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,\n> \n> \tfor (cp = buf.buf; *cp; cp = np) {\n> \t\tnp = strchrnul(cp, '\\n');\n> -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n> +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n> \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> +\t\t\t(np == cp) ? \"\" : \" \",\n> \t\t\t(int)(np - cp), cp,\n> \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> \t\tif (*np)\n> diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\n> index d3bbd00b81..ccfa6a720d 100755\n> --- c/t/t3200-branch.sh\n> +++ w/t/t3200-branch.sh\n> @@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' '\n> \thint: tracking ref '\\''refs/heads/main'\\'':\n> \thint: ambi1\n> \thint: ambi2\n> -\thint: ''\n> +\thint:\n> \thint: This is typically a configuration error.\n> -\thint: ''\n> +\thint:\n> \thint: To support setting up tracking branches, ensure that\n> \thint: different remotes'\\'' fetch refspecs map into different\n> \thint: tracking namespaces.\n\n\nA quick run tells me that this step also needs, I think:\n\n--- >8 ---\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b41a47eb94..03bb4af7d6 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1780,7 +1780,7 @@ test_expect_success 'recursive tagging should give advice' '\n sed -e \"s/|$//\" <<-EOF >expect &&\n hint: You have created a nested tag. The object referred to by your new tag is\n hint: already a tag. If you meant to tag the object that it points to, use:\n- hint: |\n+ hint:\n hint: git tag -f nested annotated-v4.0^{}\n hint: Disable this message with \"git config advice.nestedTag false\"\n EOF\n\n\n"},{"id":"491866","messageId":"50ca23f79bc0588cf6d99a56415eee2d@manjaro.org","threadId":"61227","inReplyTo":"xmqq4jcooddp.fsf@gitster.g","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-29T23:35:22Z","receivedAt":"2024-03-29T23:35:32Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-29 23:57, Junio C Hamano wrote:\n> Git tools all consistently encourage users to avoid whitespaces at\n> the end of line by giving them features like \"git diff --check\" and\n> \"git am --whitespace=fix\". Make sure that the advice messages we\n> give users avoid trailing whitespaces. We shouldn't be wasting\n> vertical screen real estate by adding blank lines in advice messages\n> that are supposed to be concise hints, but as long as we write such\n> blank line in our \"hints\", we should do it right.\n> \n> A test that expects the current behaviour of leaving trailing\n> whitespaces has been adjusted.\n> \n> Signed-off-by: Junio C Hamano \n\nLooking good to me. Consistency is always good.\n\nReviewed-by: Dragan Simic \n\n> ---\n> advice.c | 3 ++-\n> t/t3200-branch.sh | 4 ++--\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git c/advice.c w/advice.c\n> index d19648b7f8..75111191ad 100644\n> --- c/advice.c\n> +++ w/advice.c\n> @@ -105,8 +105,9 @@ static void vadvise(const char *advice, int\n> display_instructions,\n> \n> \tfor (cp = buf.buf; *cp; cp = np) {\n> \t\tnp = strchrnul(cp, '\\n');\n> -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n> +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n> \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> +\t\t\t(np == cp) ? \"\" : \" \",\n> \t\t\t(int)(np - cp), cp,\n> \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> \t\tif (*np)\n> diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\n> index d3bbd00b81..ccfa6a720d 100755\n> --- c/t/t3200-branch.sh\n> +++ w/t/t3200-branch.sh\n> @@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and \n> advise' '\n> \thint: tracking ref '\\''refs/heads/main'\\'':\n> \thint: ambi1\n> \thint: ambi2\n> -\thint: ''\n> +\thint:\n> \thint: This is typically a configuration error.\n> -\thint: ''\n> +\thint:\n> \thint: To support setting up tracking branches, ensure that\n> \thint: different remotes'\\'' fetch refspecs map into different\n> \thint: tracking namespaces.\n"},{"id":"491915","messageId":"4c8da56e-974b-474e-aefe-1ced5ee69327@gmail.com","threadId":"61227","inReplyTo":"fc002d62-6efe-42d4-b562-c10d3419fff6@gmail.com","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-31T06:17:30Z","receivedAt":"2024-03-31T06:17:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Mar 30, 2024 at 12:23:41AM +0100, Rubén Justo wrote:\n> On Fri, Mar 29, 2024 at 03:57:06PM -0700, Junio C Hamano wrote:\n> > Git tools all consistently encourage users to avoid whitespaces at\n> > the end of line by giving them features like \"git diff --check\" and\n> > \"git am --whitespace=fix\". Make sure that the advice messages we\n> > give users avoid trailing whitespaces. We shouldn't be wasting\n> > vertical screen real estate by adding blank lines in advice messages\n> > that are supposed to be concise hints, but as long as we write such\n> > blank line in our \"hints\", we should do it right.\n> > \n> > A test that expects the current behaviour of leaving trailing\n> > whitespaces has been adjusted.\n> > \n> > Signed-off-by: Junio C Hamano \n> > ---\n> \n> Yeah, ACK. This is obviously a good thing. I'll base my other series\n> in this. Thanks.\n> \n> > advice.c | 3 ++-\n> > t/t3200-branch.sh | 4 ++--\n> > 2 files changed, 4 insertions(+), 3 deletions(-)\n> > \n> > diff --git c/advice.c w/advice.c\n> > index d19648b7f8..75111191ad 100644\n> > --- c/advice.c\n> > +++ w/advice.c\n> > @@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,\n> > \n> > \tfor (cp = buf.buf; *cp; cp = np) {\n> > \t\tnp = strchrnul(cp, '\\n');\n> > -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n> > +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n> > \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> > +\t\t\t(np == cp) ? \"\" : \" \",\n> > \t\t\t(int)(np - cp), cp,\n> > \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n\nThinking again on this I wonder, while we're here, if we could go further\nand move the \"hint\" literal to the args, to ease the translation work:\n\n--- >8 ---\ndiff --git a/advice.c b/advice.c\nindex a18bfe776f..5897e62541 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -104,8 +104,9 @@ static void vadvise(const char *advice, int display_instructions,\n\n for (cp = buf.buf; *cp; cp = np) {\n np = strchrnul(cp, '\\n');\n- fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n+ fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n advise_get_color(ADVICE_COLOR_HINT),\n+ _(\"hint\"),\n (np == cp) ? \"\" : \" \",\n (int)(np - cp), cp,\n advise_get_color(ADVICE_COLOR_RESET));\n--- 8< ---\n\nOf course, this is completely optional.\n\n> > \t\tif (*np)\n> > diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh\n> > index d3bbd00b81..ccfa6a720d 100755\n> > --- c/t/t3200-branch.sh\n> > +++ w/t/t3200-branch.sh\n> > @@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' '\n> > \thint: tracking ref '\\''refs/heads/main'\\'':\n> > \thint: ambi1\n> > \thint: ambi2\n> > -\thint: ''\n> > +\thint:\n> > \thint: This is typically a configuration error.\n> > -\thint: ''\n> > +\thint:\n> > \thint: To support setting up tracking branches, ensure that\n> > \thint: different remotes'\\'' fetch refspecs map into different\n> > \thint: tracking namespaces.\n> \n> \n> A quick run tells me that this step also needs, I think:\n> \n> --- >8 ---\n> \n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index b41a47eb94..03bb4af7d6 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1780,7 +1780,7 @@ test_expect_success 'recursive tagging should give advice' '\n> sed -e \"s/|$//\" <<-EOF >expect &&\n> hint: You have created a nested tag. The object referred to by your new tag is\n> hint: already a tag. If you meant to tag the object that it points to, use:\n> - hint: |\n> + hint:\n> hint: git tag -f nested annotated-v4.0^{}\n> hint: Disable this message with \"git config advice.nestedTag false\"\n> EOF\n> \n> \n\nYour queued version already fixed this. So, nothing to worry about.\n"},{"id":"491916","messageId":"xmqqcyraj3z7.fsf@gitster.g","threadId":"61227","inReplyTo":"4c8da56e-974b-474e-aefe-1ced5ee69327@gmail.com","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-31T06:43:40Z","receivedAt":"2024-03-31T06:43:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> > \tfor (cp = buf.buf; *cp; cp = np) {\n>> > \t\tnp = strchrnul(cp, '\\n');\n>> > -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n>> > +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n>> > \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n>> > +\t\t\t(np == cp) ? \"\" : \" \",\n>> > \t\t\t(int)(np - cp), cp,\n>> > \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n>\n> Thinking again on this I wonder, while we're here, if we could go further\n> and move the \"hint\" literal to the args, to ease the translation work:\n> - fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n> + fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n> advise_get_color(ADVICE_COLOR_HINT),\n> + _(\"hint\"),\n> (np == cp) ? \"\" : \" \",\n> (int)(np - cp), cp,\n> advise_get_color(ADVICE_COLOR_RESET));\n\nIt is not guaranteed that any and all languages want to use a colon\nimmediately after translation of \"hint\"; the current message string\nwith or without my patch allows translators adjust that part to the\ntarget language, but your version will force them to always use only\na colon there. Is that an improvement? I somehow do not think so.\n\n"},{"id":"491920","messageId":"0f43cd7b-a366-4ec2-9577-93708e21f2ff@gmail.com","threadId":"61227","inReplyTo":"xmqqcyraj3z7.fsf@gitster.g","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-31T07:11:59Z","receivedAt":"2024-03-31T07:12:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Mar 30, 2024 at 11:43:40PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> >> > \tfor (cp = buf.buf; *cp; cp = np) {\n> >> > \t\tnp = strchrnul(cp, '\\n');\n> >> > -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n> >> > +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n> >> > \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> >> > +\t\t\t(np == cp) ? \"\" : \" \",\n> >> > \t\t\t(int)(np - cp), cp,\n> >> > \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> >\n> > Thinking again on this I wonder, while we're here, if we could go further\n> > and move the \"hint\" literal to the args, to ease the translation work:\n> > - fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n> > + fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n> > advise_get_color(ADVICE_COLOR_HINT),\n> > + _(\"hint\"),\n> > (np == cp) ? \"\" : \" \",\n> > (int)(np - cp), cp,\n> > advise_get_color(ADVICE_COLOR_RESET));\n> \n> It is not guaranteed that any and all languages want to use a colon\n> immediately after translation of \"hint\"; the current message string\n> with or without my patch allows translators adjust that part to the\n> target language, but your version will force them to always use only\n> a colon there. Is that an improvement? I somehow do not think so.\n\nI was just thinking if leaving the format open to the translation is a\nsane option. Maybe we can move the colon to the literal in the args,\ntoo.\n\nIn any case, the patch is OK as it is.\n"},{"id":"491923","messageId":"3e35b731-333e-4b46-9c0c-089b5c47fe99@gmail.com","threadId":"61227","inReplyTo":"0f43cd7b-a366-4ec2-9577-93708e21f2ff@gmail.com","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-31T08:11:08Z","receivedAt":"2024-03-31T08:11:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sun, Mar 31, 2024 at 09:11:59AM +0200, Rubén Justo wrote:\n> On Sat, Mar 30, 2024 at 11:43:40PM -0700, Junio C Hamano wrote:\n> > Rubén Justo writes:\n> > \n> > >> > \tfor (cp = buf.buf; *cp; cp = np) {\n> > >> > \t\tnp = strchrnul(cp, '\\n');\n> > >> > -\t\tfprintf(stderr,\t_(\"%shint: %.*s%s\\n\"),\n> > >> > +\t\tfprintf(stderr,\t_(\"%shint:%s%.*s%s\\n\"),\n> > >> > \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> > >> > +\t\t\t(np == cp) ? \"\" : \" \",\n> > >> > \t\t\t(int)(np - cp), cp,\n> > >> > \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> > >\n> > > Thinking again on this I wonder, while we're here, if we could go further\n> > > and move the \"hint\" literal to the args, to ease the translation work:\n> > > - fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n> > > + fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n> > > advise_get_color(ADVICE_COLOR_HINT),\n> > > + _(\"hint\"),\n> > > (np == cp) ? \"\" : \" \",\n> > > (int)(np - cp), cp,\n> > > advise_get_color(ADVICE_COLOR_RESET));\n> > \n> > It is not guaranteed that any and all languages want to use a colon\n> > immediately after translation of \"hint\"; the current message string\n> > with or without my patch allows translators adjust that part to the\n> > target language, but your version will force them to always use only\n> > a colon there. Is that an improvement? I somehow do not think so.\n> \n> I was just thinking if leaving the format open to the translation is a\n> sane option. Maybe we can move the colon to the literal in the args,\n> too.\n\nJust for the record, zh_CN (Chinese) and zh_TW (Traditional Chinese)\ndo not use ':' on its translation, but '：'\n\nSo, if we go the way I proposed we'll need to move the ':' too. I\nstill think it's an improvement. But, optional to this series.\n\n> \n> In any case, the patch is OK as it is.\n"},{"id":"491931","messageId":"xmqqo7auf3h5.fsf@gitster.g","threadId":"61227","inReplyTo":"3e35b731-333e-4b46-9c0c-089b5c47fe99@gmail.com","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-31T22:20:22Z","receivedAt":"2024-03-31T22:20:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> > > and move the \"hint\" literal to the args, to ease the translation work:\n>> > > - fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n>> > > + fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n>> > > advise_get_color(ADVICE_COLOR_HINT),\n>> > > + _(\"hint\"),\n>> > > (np == cp) ? \"\" : \" \",\n>> > > (int)(np - cp), cp,\n>> > > advise_get_color(ADVICE_COLOR_RESET));\n>> > \n>> > It is not guaranteed that any and all languages want to use a colon\n>> > immediately after translation of \"hint\"; the current message string\n>> > with or without my patch allows translators adjust that part to the\n>> > target language, but your version will force them to always use only\n>> > a colon there. Is that an improvement? I somehow do not think so.\n>> \n>> I was just thinking if leaving the format open to the translation is a\n>> sane option. Maybe we can move the colon to the literal in the args,\n>> too.\n>\n> Just for the record, zh_CN (Chinese) and zh_TW (Traditional Chinese)\n> do not use ':' on its translation, but '：'\n>\n> So, if we go the way I proposed we'll need to move the ':' too. I\n> still think it's an improvement. But, optional to this series.\n\nIt is making it even worse.\n\nGiving larger unit to work with to translators is usually a better\nfor i18n than chopping a single logical message into multiple pieces\nand paste them together in the code, as your untranslated\nformat-string (e.g., \"%s%s:%s%.*s%s\\n\" we see above) will force the\nword order in the final output.\n\nI think the patch at the beginning of the thread is more than\nserviceable, but if we wanted to improve on it, we should go in the\nopposite direction, e.g.\n\n\tif (np == cp)\n\t\tfprintf(stderr, _(\"%shint:%s\\n\"),\n\t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n\t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n\telse\n\t\tfprintf(stderr, _(\"%shint: %.*s%s\\n\"),\n\t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n\t\t\t(int)(np - cp), cp,\n\t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n\nto give translators flexibility to choose what kind of space to use\n(including \"none\") after \"hint:\".\n\nI am not going to do that, though, until/unless somebody complains\nand says \"there is no inter-word spaces and it is more customary not\nto have a space after the translation of 'hint:' in my language\".\n"},{"id":"491983","messageId":"b71785c5-b93e-41aa-86c1-d05703365240@gmail.com","threadId":"61227","inReplyTo":"xmqqo7auf3h5.fsf@gitster.g","subject":"Re: [PATCH] advice: omit trailing whitespace","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-01T20:50:14Z","receivedAt":"2024-04-01T20:50:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sun, Mar 31, 2024 at 03:20:22PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> >> > > and move the \"hint\" literal to the args, to ease the translation work:\n> >> > > - fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n> >> > > + fprintf(stderr, \"%s%s:%s%.*s%s\\n\",\n> >> > > advise_get_color(ADVICE_COLOR_HINT),\n> >> > > + _(\"hint\"),\n> >> > > (np == cp) ? \"\" : \" \",\n> >> > > (int)(np - cp), cp,\n> >> > > advise_get_color(ADVICE_COLOR_RESET));\n> >> > \n> >> > It is not guaranteed that any and all languages want to use a colon\n> >> > immediately after translation of \"hint\"; the current message string\n> >> > with or without my patch allows translators adjust that part to the\n> >> > target language, but your version will force them to always use only\n> >> > a colon there. Is that an improvement? I somehow do not think so.\n> >> \n> >> I was just thinking if leaving the format open to the translation is a\n> >> sane option. Maybe we can move the colon to the literal in the args,\n> >> too.\n> >\n> > Just for the record, zh_CN (Chinese) and zh_TW (Traditional Chinese)\n> > do not use ':' on its translation, but '：'\n> >\n> > So, if we go the way I proposed we'll need to move the ':' too. I\n> > still think it's an improvement. But, optional to this series.\n> \n> It is making it even worse.\n> \n> Giving larger unit to work with to translators is usually a better\n> for i18n than chopping a single logical message into multiple pieces\n> and paste them together in the code, as your untranslated\n> format-string (e.g., \"%s%s:%s%.*s%s\\n\" we see above) will force the\n> word order in the final output.\n> \n> I think the patch at the beginning of the thread is more than\n> serviceable, but if we wanted to improve on it, we should go in the\n> opposite direction, e.g.\n> \n> \tif (np == cp)\n> \t\tfprintf(stderr, _(\"%shint:%s\\n\"),\n> \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> \telse\n> \t\tfprintf(stderr, _(\"%shint: %.*s%s\\n\"),\n> \t\t\tadvise_get_color(ADVICE_COLOR_HINT),\n> \t\t\t(int)(np - cp), cp,\n> \t\t\tadvise_get_color(ADVICE_COLOR_RESET));\n> \n> to give translators flexibility to choose what kind of space to use\n> (including \"none\") after \"hint:\".\n\ndiff --git a/advice.c b/advice.c\nindex a18bfe776f..a625316725 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -104,9 +104,9 @@ static void vadvise(const char *advice, int display_instructions,\n\n for (cp = buf.buf; *cp; cp = np) {\n np = strchrnul(cp, '\\n');\n- fprintf(stderr, _(\"%shint:%s%.*s%s\\n\"),\n+ fprintf(stderr, \"%s%s%s%.*s%s\\n\",\n advise_get_color(ADVICE_COLOR_HINT),\n- (np == cp) ? \"\" : \" \",\n+ (np == cp) ? _(\"hint:\") : _(\"hint: \"),\n (int)(np - cp), cp,\n advise_get_color(ADVICE_COLOR_RESET));\n if (*np)\n\n;)\n\n> \n> I am not going to do that, though, until/unless somebody complains\n> and says \"there is no inter-word spaces and it is more customary not\n> to have a space after the translation of 'hint:' in my language\".\n"}]}