Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 23:32, Jeff King  wrote:
> On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:
>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> Heh, that is one of the cases that I found most ugly when I looked into
> this earlier (and in particular, because I think it makes cut-and-paste
> a little harder).
>
> More discussion in:
>
>   
> https://public-inbox.org/git/2017040758.yyfsc3r3spqpi...@sigill.intra.peff.net/
>
> and down-thread.

Thanks for the pointer. I had missed that thread entirely.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 17:50, Duy Nguyen  wrote:
> On Tue, May 29, 2018 at 6:49 AM, Martin Ågren  wrote:
>> On 28 May 2018 at 23:45, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
>> +error:   sub/added
>> +error:   sub/addedtoo
>> +error: Please move or remove them before you switch branches.
>>  Aborting
>>  EOF
>
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

 Also, very subjectively, I'm torn about this. To me, just one
 "error/warning/fatal" at the start of the first paragraph feels much
 better. If we have to somehow mark the second paragraph that "this is
 also part of the error message" then it's probably better to rephrase.
>>
>> Would you feel the same about "hint: "? We already do prefix all the
>> lines there. It seems to we we should probably do the same for "hint: "
>> as for "warning: ", whatever we decide is right.
>
> It may depend on context. Let's look at the commit that introduces
> this "hint:" prefix, 38ef61cfde (advice: Introduce
> error_resolve_conflict - 2011-08-04). The example in the commit
> message shows the hint paragraph sandwiched by an error and a fatal
> one:
>
>   error: 'commit' is not possible because you have unmerged files.
>   hint: Fix them up in the work tree ...
>   hint: ...
>   fatal: Exiting because of an unresolved conflict.
>
> I think in this case (dense paragraphs of different message types) yes
> it might make sense to prefix lines with "hint:". But when there's
> only one type of message like the "error" part quoted at the top, it
> feels too verbose to have error: prefix everywhere.

Hmm, that's interesting. Deciding based on what has already been output
seems feasible, although it sounds like the potential target of infinite
tweaking of some central logic for deciding which way to go. Or, lots of
various places to try and make consistent. I am tempted towards
indenting with spaces in v2, and to leave "hint: " alone as an outlier.
(It always was one. :-/ ) I'll keep your feedback in mind.

Thanks,
Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Jeff King]

On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:

> Martin Ågren  writes:
> 
> > diff --git a/t/t1011-read-tree-sparse-checkout.sh 
> > b/t/t1011-read-tree-sparse-checkout.sh
> > index 0c6f48f302..31b0702e6c 100755
> > --- a/t/t1011-read-tree-sparse-checkout.sh
> > +++ b/t/t1011-read-tree-sparse-checkout.sh
> > @@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
> > worktree' '
> > test_must_fail git checkout top 2>actual &&
> > cat >expected <<\EOF &&
> >  error: The following untracked working tree files would be overwritten by 
> > checkout:
> > -   sub/added
> > -   sub/addedtoo
> > -Please move or remove them before you switch branches.
> > +error: sub/added
> > +error: sub/addedtoo
> > +error: Please move or remove them before you switch branches.
> >  Aborting
> >  EOF
> 
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

Heh, that is one of the cases that I found most ugly when I looked into
this earlier (and in particular, because I think it makes cut-and-paste
a little harder).

More discussion in:

  
https://public-inbox.org/git/2017040758.yyfsc3r3spqpi...@sigill.intra.peff.net/

and down-thread.

-Peff

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Duy Nguyen]

On Tue, May 29, 2018 at 6:49 AM, Martin Ågren  wrote:
> On 28 May 2018 at 23:45, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
> +error:   sub/added
> +error:   sub/addedtoo
> +error: Please move or remove them before you switch branches.
>  Aborting
>  EOF

 This shows the typical effect of this series, which (I subjectively
 think) gives us a more pleasant end-user experience.
>>>
>>> Also, very subjectively, I'm torn about this. To me, just one
>>> "error/warning/fatal" at the start of the first paragraph feels much
>>> better. If we have to somehow mark the second paragraph that "this is
>>> also part of the error message" then it's probably better to rephrase.
>
> Would you feel the same about "hint: "? We already do prefix all the
> lines there. It seems to we we should probably do the same for "hint: "
> as for "warning: ", whatever we decide is right.

It may depend on context. Let's look at the commit that introduces
this "hint:" prefix, 38ef61cfde (advice: Introduce
error_resolve_conflict - 2011-08-04). The example in the commit
message shows the hint paragraph sandwiched by an error and a fatal
one:

  error: 'commit' is not possible because you have unmerged files.
  hint: Fix them up in the work tree ...
  hint: ...
  fatal: Exiting because of an unresolved conflict.

I think in this case (dense paragraphs of different message types) yes
it might make sense to prefix lines with "hint:". But when there's
only one type of message like the "error" part quoted at the top, it
feels too verbose to have error: prefix everywhere.
-- 
Duy

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren  writes:

> About the _("\t")-approach that you mentioned up-thread. It would allow
> a translator to adjust all the indentations for a particular language.
> To be clear, what you mean is _(" " /* 9 spaces */) to align
> nicely with "warning: ", which is the longest English string. Then the
> translator would translate the nine spaces and all of "fatal:   " and
> others to padded strings, all of the same length (not necessarily nine).
> Correct?

I was envisioning that these

error: the first line of an error message
   and the second line indented by 7 places (strlen("error:")+1)
info: the first line of an info message
  and the second line indented by 6 places (strlen("info:")+1)

are produced by

vreportf("error: ", "   " /* 7 spaces */,
 "the first line of an error message\nand the second ...");
vreportf("info: ", "  " /* 6 spaces */,
 "the first line of an info message\nand the second ...");

And if all of these string literals were inside _(), then depending
on how many display columns translated version of "error" and "info"
takes in the target language, these 7-space and 6-space secondary
prefixes would be "translated" differently.

Of course, since your language may translate "error" and "fatal" to
different display columns, the 7-space secondary prefix in this one

vreportf("fatal: ", "   " /* 7 spaces */,
 "the first line of a fatal error message\nand the second ...");

needs to be mapped to a string that is differnt from the 7-space for
"error: ".  I think you would use "contexts" to map the same source
7-space to different translated string when it becomes necessary.

https://www.gnu.org/software/gettext/manual/html_node/Contexts.html

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 07:50, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>>>  - allow callers to align 1st prefix (e.g. "error: ") with the
>>>leading indent for the second and subsequent lines by passing the
>>>second prefix with appropriate display width.
>>
>> I suspect this second prefix would always consist of
>> strlen(first_prefix) spaces? We should be able to construct it on the
>> fly, without any need for manual counting and human mistakes.
>
> I wouldn't be so bold to claim that, given especially that we are
> talking about i18n/l10n where byte count, character count and
> display width are all different even on a terminal with fixed-width
> font.

You are of course correct. I should have my morning tea before typing.

About the _("\t")-approach that you mentioned up-thread. It would allow
a translator to adjust all the indentations for a particular language.
To be clear, what you mean is _(" " /* 9 spaces */) to align
nicely with "warning: ", which is the longest English string. Then the
translator would translate the nine spaces and all of "fatal:   " and
others to padded strings, all of the same length (not necessarily nine).
Correct?

That approach seems a bit shaky, if nothing else because we may one day
similarly want to use nine "translated" spaces in some other context. Or
maybe this is actually i18n-best-practices.

It also means that shorter prefixes are somewhat arbitrarily padded,
just because there exists a longer prefix that some other code path may
want to use. OTOH, if a "warning: " is followed by an "error:   ", both
lines/blocks would have the same indentation, which might perhaps be
(slightly) preferable.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren  writes:

>>  - allow callers to align 1st prefix (e.g. "error: ") with the
>>leading indent for the second and subsequent lines by passing the
>>second prefix with appropriate display width.
>
> I suspect this second prefix would always consist of
> strlen(first_prefix) spaces? We should be able to construct it on the
> fly, without any need for manual counting and human mistakes.

I wouldn't be so bold to claim that, given especially that we are
talking about i18n/l10n where byte count, character count and
display width are all different even on a terminal with fixed-width
font.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 28 May 2018 at 23:45, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
 +error:   sub/added
 +error:   sub/addedtoo
 +error: Please move or remove them before you switch branches.
  Aborting
  EOF
>>>
>>> This shows the typical effect of this series, which (I subjectively
>>> think) gives us a more pleasant end-user experience.
>>
>> Also, very subjectively, I'm torn about this. To me, just one
>> "error/warning/fatal" at the start of the first paragraph feels much
>> better. If we have to somehow mark the second paragraph that "this is
>> also part of the error message" then it's probably better to rephrase.

Would you feel the same about "hint: "? We already do prefix all the
lines there. It seems to we we should probably do the same for "hint: "
as for "warning: ", whatever we decide is right.

> I personally can go either way.  If you prefer less noisy route, we
> could change the function signature of vreportf() to take a prefix
> for the first line and another prefix for the remaining lines and
> pass that through down to the "split and print with prefix" helper.
>
> That way, we can
>
>  - allow callers to align 1st prefix (e.g. "error: ") with the
>leading indent for the second and subsequent lines by passing the
>second prefix with appropriate display width.

I suspect this second prefix would always consist of
strlen(first_prefix) spaces? We should be able to construct it on the
fly, without any need for manual counting and human mistakes.

>
>  - allow translators to grow or shrink number of lines a given
>message takes, and to decide where in the translated string to
>wrap lines.
>
> Even though step 3/3 may become a bit awkward (the second prefix
> would most likely be only whitespace, and you'd need to write
> something silly like _("\t")), we can still keep the alignment if we
> wanted to.

Thanks both for your comments. I'll see what I can come up with.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Duy Nguyen  writes:

>>> +error:   sub/added
>>> +error:   sub/addedtoo
>>> +error: Please move or remove them before you switch branches.
>>>  Aborting
>>>  EOF
>>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> Also, very subjectively, I'm torn about this. To me, just one
> "error/warning/fatal" at the start of the first paragraph feels much
> better. If we have to somehow mark the second paragraph that "this is
> also part of the error message" then it's probably better to rephrase.

I personally can go either way.  If you prefer less noisy route, we
could change the function signature of vreportf() to take a prefix
for the first line and another prefix for the remaining lines and
pass that through down to the "split and print with prefix" helper.

That way, we can

 - allow callers to align 1st prefix (e.g. "error: ") with the
   leading indent for the second and subsequent lines by passing the
   second prefix with appropriate display width.

 - allow translators to grow or shrink number of lines a given
   message takes, and to decide where in the translated string to
   wrap lines.

Even though step 3/3 may become a bit awkward (the second prefix
would most likely be only whitespace, and you'd need to write
something silly like _("\t")), we can still keep the alignment if we
wanted to.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Duy Nguyen]

On Mon, May 28, 2018 at 11:25 AM, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> diff --git a/t/t1011-read-tree-sparse-checkout.sh 
>> b/t/t1011-read-tree-sparse-checkout.sh
>> index 0c6f48f302..31b0702e6c 100755
>> --- a/t/t1011-read-tree-sparse-checkout.sh
>> +++ b/t/t1011-read-tree-sparse-checkout.sh
>> @@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
>> worktree' '
>>   test_must_fail git checkout top 2>actual &&
>>   cat >expected <<\EOF &&
>>  error: The following untracked working tree files would be overwritten by 
>> checkout:
>> - sub/added
>> - sub/addedtoo
>> -Please move or remove them before you switch branches.
>> +error:   sub/added
>> +error:   sub/addedtoo
>> +error: Please move or remove them before you switch branches.
>>  Aborting
>>  EOF
>
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

Also, very subjectively, I'm torn about this. To me, just one
"error/warning/fatal" at the start of the first paragraph feels much
better. If we have to somehow mark the second paragraph that "this is
also part of the error message" then it's probably better to rephrase.
-- 
Duy

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren  writes:

> diff --git a/t/t1011-read-tree-sparse-checkout.sh 
> b/t/t1011-read-tree-sparse-checkout.sh
> index 0c6f48f302..31b0702e6c 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
> worktree' '
>   test_must_fail git checkout top 2>actual &&
>   cat >expected <<\EOF &&
>  error: The following untracked working tree files would be overwritten by 
> checkout:
> - sub/added
> - sub/addedtoo
> -Please move or remove them before you switch branches.
> +error:   sub/added
> +error:   sub/addedtoo
> +error: Please move or remove them before you switch branches.
>  Aborting
>  EOF

This shows the typical effect of this series, which (I subjectively
think) gives us a more pleasant end-user experience.

> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index 4ee009da66..80d35087b7 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -11,7 +11,7 @@ test_did_you_mean ()
>   sq="'" &&
>   cat >expected <<-EOF &&
>   fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
> - Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
> + fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
>   EOF

And this, too.

> diff --git a/usage.c b/usage.c
> index 80f9c1d14b..6a5669922f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -34,7 +34,7 @@ void vreportf(const char *prefix, const char *err, va_list 
> params)
>   if (iscntrl(*p) && *p != '\t' && *p != '\n')
>   *p = '?';
>   }
> - fprintf(stderr, "%s%s\n", prefix, msg);
> + prefix_suffix_lines(stderr, prefix, msg, "");
>  }

[RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

Teach `vreportf()` to prefix all lines with the given prefix, not only
the first line. This matches how "hint: " is being shown, and affects
"error: ", "fatal: ", "usage: ", "warning: " and "BUG: " (as well as any
out-of-tree and future users).

Note that we need to adjust quite a few tests as a result of this
change. All of these changes are because we obviously need to prefix
more lines in various "expect"-files -- except for one instance of a
trailing empty line that disappears with this commit (see t7609). This
is a very minor change, and arguably a good one.

Signed-off-by: Martin Ågren 
---
Looking back at this, I wonder if the opposite approach would be better,
that is, making `advise()` use `vreportf()` after teaching the latter
the multi-line trick.

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 usage.c  |  2 +-
 8 files changed, 51 insertions(+), 52 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..31b0702e6c 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
worktree' '
test_must_fail git checkout top 2>actual &&
cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by 
checkout:
-   sub/added
-   sub/addedtoo
-Please move or remove them before you switch branches.
+error: sub/added
+error: sub/addedtoo
+error: Please move or remove them before you switch branches.
 Aborting
 EOF
test_i18ncmp expected actual
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4ee009da66..80d35087b7 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -11,7 +11,7 @@ test_did_you_mean ()
sq="'" &&
cat >expected <<-EOF &&
fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-   Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+   fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
EOF
test_cmp expected error
 }
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index c4422312f4..39a707c94a 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -16,7 +16,7 @@ test_expect_success 'bogus GIT_INDEX_VERSION issues warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -30,7 +30,7 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -54,7 +54,7 @@ test_expect_success 'out of bounds index.version issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: index.version set, but the value is invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..cd4a10df2d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -771,10 +771,10 @@ test_expect_success 'setup for testing rm messages' '
 test_expect_success 'rm files with different staged content' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-   bar.txt
-   foo.txt
-   (use -f to force removal)
+   error: file and the HEAD:
+   error: bar.txt
+   error: foo.txt
+   error: (use -f to force removal)
EOF
echo content1 >foo.txt &&
echo content1 >bar.txt &&
@@ -785,9 +785,9 @@ test_expect_success 'rm files with different staged 
content' '
 test_expect_success 'rm files with different staged content without hints' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-