| Message ID | xmqq4jcooddp.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | advice: omit trailing whitespace | expand |
On Fri, Mar 29, 2024 at 03:57:06PM -0700, Junio C Hamano wrote: > 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. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- Yeah, ACK. This is obviously a good thing. I'll base my other series in this. Thanks. > advice.c | 3 ++- > t/t3200-branch.sh | 4 ++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git c/advice.c w/advice.c > index d19648b7f8..75111191ad 100644 > --- c/advice.c > +++ w/advice.c > @@ -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) > diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh > index d3bbd00b81..ccfa6a720d 100755 > --- c/t/t3200-branch.sh > +++ w/t/t3200-branch.sh > @@ -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. A quick run tells me that this step also needs, I think: --- >8 --- diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index b41a47eb94..03bb4af7d6 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -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
On 2024-03-29 23:57, Junio C Hamano wrote: > 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. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> Looking good to me. Consistency is always good. Reviewed-by: Dragan Simic <dsimic@manjaro.org> > --- > advice.c | 3 ++- > t/t3200-branch.sh | 4 ++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git c/advice.c w/advice.c > index d19648b7f8..75111191ad 100644 > --- c/advice.c > +++ w/advice.c > @@ -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) > diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh > index d3bbd00b81..ccfa6a720d 100755 > --- c/t/t3200-branch.sh > +++ w/t/t3200-branch.sh > @@ -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.
On Sat, Mar 30, 2024 at 12:23:41AM +0100, Rubén Justo wrote: > On Fri, Mar 29, 2024 at 03:57:06PM -0700, Junio C Hamano wrote: > > 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. > > > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > --- > > Yeah, ACK. This is obviously a good thing. I'll base my other series > in this. Thanks. > > > advice.c | 3 ++- > > t/t3200-branch.sh | 4 ++-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git c/advice.c w/advice.c > > index d19648b7f8..75111191ad 100644 > > --- c/advice.c > > +++ w/advice.c > > @@ -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)); 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 --- diff --git a/advice.c b/advice.c index a18bfe776f..5897e62541 100644 --- a/advice.c +++ b/advice.c @@ -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. > > if (*np) > > diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh > > index d3bbd00b81..ccfa6a720d 100755 > > --- c/t/t3200-branch.sh > > +++ w/t/t3200-branch.sh > > @@ -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. > > > A quick run tells me that this step also needs, I think: > > --- >8 --- > > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index b41a47eb94..03bb4af7d6 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -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.
Rubén Justo <rjusto@gmail.com> writes: >> > 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)); > > 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: > - 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.
On Sat, Mar 30, 2024 at 11:43:40PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> > 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)); > > > > 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: > > - 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. 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.
On Sun, Mar 31, 2024 at 09:11:59AM +0200, Rubén Justo wrote: > On Sat, Mar 30, 2024 at 11:43:40PM -0700, Junio C Hamano wrote: > > Rubén Justo <rjusto@gmail.com> writes: > > > > >> > 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)); > > > > > > 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: > > > - 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. > > 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. > > In any case, the patch is OK as it is.
Rubén Justo <rjusto@gmail.com> writes: >> > > and move the "hint" literal to the args, to ease the translation work: >> > > - 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. >> >> 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. 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".
On Sun, Mar 31, 2024 at 03:20:22PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> > > and move the "hint" literal to the args, to ease the translation work: > >> > > - 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. > >> > >> 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. > > 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:". diff --git a/advice.c b/advice.c index a18bfe776f..a625316725 100644 --- a/advice.c +++ b/advice.c @@ -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) ;) > > 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".
diff --git c/advice.c w/advice.c index d19648b7f8..75111191ad 100644 --- c/advice.c +++ w/advice.c @@ -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) diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh index d3bbd00b81..ccfa6a720d 100755 --- c/t/t3200-branch.sh +++ w/t/t3200-branch.sh @@ -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.
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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- advice.c | 3 ++- t/t3200-branch.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)