[PATCH] advice: omit trailing whitespace

[PATCH] advice: omit trailing whitespace

[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.

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(-)

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.

Re: [PATCH] advice: omit trailing whitespace

[Rubén Justo]

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

Re: [PATCH] advice: omit trailing whitespace

[Rubén Justo]

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.

Re: [PATCH] advice: omit trailing whitespace

[Junio C Hamano]

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.

Re: [PATCH] advice: omit trailing whitespace

[Rubén Justo]

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.

Re: [PATCH] advice: omit trailing whitespace

[Rubén Justo]

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.

Re: [PATCH] advice: omit trailing whitespace

[Junio C Hamano]

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".

Re: [PATCH] advice: omit trailing whitespace

[Rubén Justo]

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".

Re: [PATCH] advice: omit trailing whitespace

[Dragan Simic]

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.