Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first
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
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
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
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
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
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
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
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
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
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
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
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: - bar.txt -