advice: omit trailing whitespace
PATCH 1/1 advice: omit trailing whitespace
Git tools all consistently encourage users to avoid whitespaces at the end of line by giving them features like "git diff --check" and "git am --whitespace=fix". Make sure that the advice messages we give users avoid trailing whitespaces. We shouldn't be wasting vertical screen real estate by adding blank lines in advice messages that are supposed to be concise hints, but as long as we write such blank line in our "hints", we should do it right.
A test that expects the current behaviour of leaving trailing whitespaces has been adjusted.
--- advice.c | 3 ++- t/t3200-branch.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
advice.c +2 -1
@@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));if (*np)
t/t3200-branch.sh +2 -2
@@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' ' hint: tracking ref '\''refs/heads/main'\'': hint: ambi1 hint: ambi2 - hint: '' + hint: hint: This is typically a configuration error. - hint: '' + hint: hint: To support setting up tracking branches, ensure that hint: different remotes'\'' fetch refspecs map into different hint: tracking namespaces.
Quoted 8 lines · Junio C Hamano
Git tools all consistently encourage users to avoid whitespaces at the end of line by giving them features like "git diff --check" and "git am --whitespace=fix". Make sure that the advice messages we give users avoid trailing whitespaces. We shouldn't be wasting vertical screen real estate by adding blank lines in advice messages that are supposed to be concise hints, but as long as we write such blank line in our "hints", we should do it right.
Quoted 5 lines
A test that expects the current behaviour of leaving trailing whitespaces has been adjusted.
Signed-off-by Junio C Hamano <[email protected]>---
Yeah, ACK. This is obviously a good thing. I'll base my other series in this. Thanks.
Quoted 25 lines
advice.c | 3 ++- t/t3200-branch.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)advice.c +2
-1@@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));if (*np)t/t3200-branch.sh +0
-0@@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' ' hint: tracking ref '\''refs/heads/main'\'':
t/t3200-branch.sh test_expect_success 'avoid ambiguous track and advise' ' · line 1162
Quoted 10 lines
t/t3200-branch.sh +2
-2hint: ambi1 hint: ambi2 - hint: '' + hint: hint: This is typically a configuration error. - hint: '' + hint: hint: To support setting up tracking branches, ensure that hint: different remotes'\'' fetch refspecs map into different hint: tracking namespaces.
A quick run tells me that this step also needs, I think:
--- >8 ---
t/t7004-tag.sh +1 -1
@@ -1780,7 +1780,7 @@ test_expect_success 'recursive tagging should give advice' ' sed -e "s/|$//" <<-EOF >expect && hint: You have created a nested tag. The object referred to by your new tag is hint: already a tag. If you meant to tag the object that it points to, use: - hint: | + hint: hint: git tag -f nested annotated-v4.0^{} hint: Disable this message with "git config advice.nestedTag false" EOF
Quoted 28 lines · Rubén Justo
Quoted 8 lines · Junio C Hamano
Git tools all consistently encourage users to avoid whitespaces at the end of line by giving them features like "git diff --check" and "git am --whitespace=fix". Make sure that the advice messages we give users avoid trailing whitespaces. We shouldn't be wasting vertical screen real estate by adding blank lines in advice messages that are supposed to be concise hints, but as long as we write such blank line in our "hints", we should do it right.
Quoted 5 lines
A test that expects the current behaviour of leaving trailing whitespaces has been adjusted.
Signed-off-by Junio C Hamano <[email protected]>---
Yeah, ACK. This is obviously a good thing. I'll base my other series in this. Thanks.
Quoted 10 lines
advice.c | 3 ++- t/t3200-branch.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)advice.c +0
-0@@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions,
Quoted 8 lines
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 8 lines
advice.c +2
-1for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));
Thinking again on this I wonder, while we're here, if we could go further and move the "hint" literal to the args, to ease the translation work:
--- >8 ---
advice.c +2 -1
@@ -104,8 +104,9 @@ static void vadvise(const char *advice, int display_instructions,for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s:%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), +_ ("hint" ), (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));
--- 8< ---
Of course, this is completely optional.
Quoted 30 lines
Quoted 7 lines
advice.c +0
-0if (*np)t/t3200-branch.sh +0
-0@@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track and advise' ' hint: tracking ref '\''refs/heads/main'\'':t/t3200-branch.sh test_expect_success 'avoid ambiguous track and advise' ' · line 1162
Quoted 10 lines
t/t3200-branch.sh +2
-2hint: ambi1 hint: ambi2 - hint: '' + hint: hint: This is typically a configuration error. - hint: '' + hint: hint: To support setting up tracking branches, ensure that hint: different remotes'\'' fetch refspecs map into different hint: tracking namespaces.A quick run tells me that this step also needs, I think:
--- >8 ---
t/t7004-tag.sh +0
-0
t/t7004-tag.sh test_expect_success 'recursive tagging should give advice' ' · line 1786
Quoted 8 lines
t/t7004-tag.sh +1
-1@@ -1780,7 +1780,7 @@ test_expect_success 'recursive tagging should give advice' ' sed -e "s/|$//" <<-EOF >expect && hint: You have created a nested tag. The object referred to by your new tag is hint: already a tag. If you meant to tag the object that it points to, use: - hint: | + hint: hint: git tag -f nested annotated-v4.0^{} hint: Disable this message with "git config advice.nestedTag false" EOF
Your queued version already fixed this. So, nothing to worry about.
Quoted 11 lines · Rubén Justo
Quoted 8 lines
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 8 lines
advice.c +2
-1for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));Thinking again on this I wonder, while we're here, if we could go further and move the "hint" literal to the args, to ease the translation work:
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 7 lines
advice.c +2
-1-fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s:%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), +_ ("hint" ), (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));
It is not guaranteed that any and all languages want to use a colon immediately after translation of "hint"; the current message string with or without my patch allows translators adjust that part to the target language, but your version will force them to always use only a colon there. Is that an improvement? I somehow do not think so.
Quoted 21 lines · Junio C Hamano
Quoted 11 lines · Rubén Justo
Quoted 8 lines
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 8 lines
advice.c +2
-1for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));Thinking again on this I wonder, while we're here, if we could go further and move the "hint" literal to the args, to ease the translation work:
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 7 lines
advice.c +2
-1-fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s:%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), +_ ("hint" ), (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));
Quoted 5 lines
It is not guaranteed that any and all languages want to use a colon immediately after translation of "hint"; the current message string with or without my patch allows translators adjust that part to the target language, but your version will force them to always use only a colon there. Is that an improvement? I somehow do not think so.
I was just thinking if leaving the format open to the translation is a sane option. Maybe we can move the colon to the literal in the args, too.
In any case, the patch is OK as it is.
Quoted 28 lines · Rubén Justo
Quoted 21 lines · Junio C Hamano
Quoted 11 lines · Rubén Justo
Quoted 8 lines
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 8 lines
advice.c +2
-1for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));Thinking again on this I wonder, while we're here, if we could go further and move the "hint" literal to the args, to ease the translation work:
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 7 lines
advice.c +2
-1-fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s:%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), +_ ("hint" ), (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));Quoted 5 lines
It is not guaranteed that any and all languages want to use a colon immediately after translation of "hint"; the current message string with or without my patch allows translators adjust that part to the target language, but your version will force them to always use only a colon there. Is that an improvement? I somehow do not think so.
Quoted 3 lines
I was just thinking if leaving the format open to the translation is a sane option. Maybe we can move the colon to the literal in the args, too.
Just for the record, zh_CN (Chinese) and zh_TW (Traditional Chinese) do not use ':' on its translation, but '：'
So, if we go the way I proposed we'll need to move the ':' too. I still think it's an improvement. But, optional to this series.
Quoted 2 lines
In any case, the patch is OK as it is.
Quoted 19 lines · Rubén Justo
Quoted 15 lines
Quoted 9 lines
advice.c static void vadvise(const char *advice, int display_instructions, · line 112
Quoted 8 lines
and move the "hint" literal to the args, to ease the translation work:
advice.c +2
-1-fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s:%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), +_ ("hint" ), (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));Quoted 5 lines
It is not guaranteed that any and all languages want to use a colon immediately after translation of "hint"; the current message string with or without my patch allows translators adjust that part to the target language, but your version will force them to always use only a colon there. Is that an improvement? I somehow do not think so.
Quoted 3 lines
I was just thinking if leaving the format open to the translation is a sane option. Maybe we can move the colon to the literal in the args, too.
Quoted 5 lines
Just for the record,
zh_CN(Chinese) andzh_TW(Traditional Chinese) do not use ':' on its translation, but '：'So, if we go the way I proposed we'll need to move the ':' too. I still think it's an improvement. But, optional to this series.
It is making it even worse.
Giving larger unit to work with to translators is usually a better for i18n than chopping a single logical message into multiple pieces and paste them together in the code, as your untranslated format-string (e.g., "%s%s:%s%.*s%s\n" we see above) will force the word order in the final output.
I think the patch at the beginning of the thread is more than serviceable, but if we wanted to improve on it, we should go in the opposite direction, e.g.
if (np == cp)
fprintf(stderr, _("%shint:%s\n"),
advise_get_color(ADVICE_COLOR_HINT),
advise_get_color(ADVICE_COLOR_RESET));
else
fprintf(stderr, _("%shint: %.*s%s\n"),
advise_get_color(ADVICE_COLOR_HINT),
(int)(np - cp), cp,
advise_get_color(ADVICE_COLOR_RESET));to give translators flexibility to choose what kind of space to use (including "none") after "hint:".
I am not going to do that, though, until/unless somebody complains and says "there is no inter-word spaces and it is more customary not to have a space after the translation of 'hint:' in my language".
Quoted 49 lines · Junio C Hamano
Quoted 19 lines · Rubén Justo
Quoted 15 lines
Quoted 9 lines
Review
Quoted 8 lines
and move the "hint" literal to the args, to ease the translation work:
(quoted diff) +2
-1- fprintf(stderr, _("%shint:%s%.*s%s\n"), + fprintf(stderr, "%s%s:%s%.*s%s\n", advise_get_color(ADVICE_COLOR_HINT), + _("hint"), (np == cp) ? "" : " ", (int)(np - cp), cp, advise_get_color(ADVICE_COLOR_RESET));Quoted 5 lines
It is not guaranteed that any and all languages want to use a colon immediately after translation of "hint"; the current message string with or without my patch allows translators adjust that part to the target language, but your version will force them to always use only a colon there. Is that an improvement? I somehow do not think so.
Quoted 3 lines
I was just thinking if leaving the format open to the translation is a sane option. Maybe we can move the colon to the literal in the args, too.
Quoted 5 lines
Just for the record,
zh_CN(Chinese) andzh_TW(Traditional Chinese) do not use ':' on its translation, but '：'So, if we go the way I proposed we'll need to move the ':' too. I still think it's an improvement. But, optional to this series.
It is making it even worse.
Giving larger unit to work with to translators is usually a better for i18n than chopping a single logical message into multiple pieces and paste them together in the code, as your untranslated format-string (e.g., "%s%s:%s%.*s%s\n" we see above) will force the word order in the final output.
I think the patch at the beginning of the thread is more than serviceable, but if we wanted to improve on it, we should go in the opposite direction, e.g.
if (np == cp) fprintf(stderr, _("%shint:%s\n"), advise_get_color(ADVICE_COLOR_HINT), advise_get_color(ADVICE_COLOR_RESET)); else fprintf(stderr, _("%shint: %.*s%s\n"), advise_get_color(ADVICE_COLOR_HINT), (int)(np - cp), cp, advise_get_color(ADVICE_COLOR_RESET));
Quoted 2 lines
to give translators flexibility to choose what kind of space to use (including "none") after "hint:".
advice.c +2 -2
@@ -104,9 +104,9 @@ static void vadvise(const char *advice, int display_instructions,for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint:%s%.*s%s\n" ), +fprintf (stderr,"%s%s%s%.*s%s\n" ,advise_get_color (ADVICE_COLOR_HINT ), - (np == cp) ?"" :" " , + (np == cp) ?_ ("hint:" ) :_ ("hint: " ), (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));if (*np)
;)
Quoted 4 lines
I am not going to do that, though, until/unless somebody complains and says "there is no inter-word spaces and it is more customary not to have a space after the translation of 'hint:' in my language".
Quoted 8 lines · Junio C Hamano
Git tools all consistently encourage users to avoid whitespaces at the end of line by giving them features like "git diff --check" and "git am --whitespace=fix". Make sure that the advice messages we give users avoid trailing whitespaces. We shouldn't be wasting vertical screen real estate by adding blank lines in advice messages that are supposed to be concise hints, but as long as we write such blank line in our "hints", we should do it right.
Quoted 4 lines
A test that expects the current behaviour of leaving trailing whitespaces has been adjusted.
Signed-off-by Junio C Hamano <[email protected]>
Looking good to me. Consistency is always good.
Quoted 38 lines
--- advice.c | 3 ++- t/t3200-branch.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)advice.c +2
-1@@ -105,8 +105,9 @@ static void vadvise(const char *advice, int display_instructions, for (cp = buf.buf; *cp; cp = np) { np =strchrnul (cp,'\n' ); -fprintf (stderr,_ ("%shint: %.*s%s\n" ), +fprintf (stderr,_ ("%shint:%s%.*s%s\n" ),advise_get_color (ADVICE_COLOR_HINT ), + (np == cp) ?"" :" " , (int )(np - cp), cp,advise_get_color (ADVICE_COLOR_RESET ));advice.c +0
-0if (*np)t/t3200-branch.sh +0
-0@@ -1154,9 +1154,9 @@ test_expect_success 'avoid ambiguous track andadvise' '
t/t3200-branch.sh +2
-2hint: tracking ref '\''refs/heads/main'\'': hint: ambi1 hint: ambi2 - hint: '' + hint: hint: This is typically a configuration error. - hint: '' + hint: hint: To support setting up tracking branches, ensure that hint: different remotes'\'' fetch refspecs map into different hint: tracking namespaces.