| Message ID | 10905ab3-bb3c-4669-9177-84c8e6759616@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 9d225b025d96d37e8c914646c54cfa9fb5f52b80 |
| Headers | show |
| Series | add-patch: response to unknown command | expand |
Rubén Justo <rjusto@gmail.com> writes: > diff --git a/add-patch.c b/add-patch.c > index 0997d4af73..fc0eed4fd4 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) > va_list args; > > va_start(args, fmt); > - fputs(s->s.error_color, stderr); > - vfprintf(stderr, fmt, args); > - fputs(s->s.reset_color, stderr); > - fputc('\n', stderr); > + fputs(s->s.error_color, stdout); > + vprintf(fmt, args); > + puts(s->s.reset_color); I like the attention of the detail here ;-).
On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > diff --git a/add-patch.c b/add-patch.c > > index 0997d4af73..fc0eed4fd4 100644 > > --- a/add-patch.c > > +++ b/add-patch.c > > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) > > va_list args; > > > > va_start(args, fmt); > > - fputs(s->s.error_color, stderr); > > - vfprintf(stderr, fmt, args); > > - fputs(s->s.reset_color, stderr); > > - fputc('\n', stderr); > > + fputs(s->s.error_color, stdout); > > + vprintf(fmt, args); > > + puts(s->s.reset_color); > > I like the attention of the detail here ;-). Indeed. I had to read this several times to wonder why it was not a mistake to leave the first fputs() but use vprintf() and puts() for the other two (for those just reading, the answer is that puts() prints an extra newline, so we can only use it at the end). Which IMHO really just points to how inconsistent the stdio interfaces are, but there is nothing we can do about that. ;) I am tempted to suggest that it borders on too-clever, and writing out "stdout" everywhere with vfprintf() and fputs() would be more obvious. But in a little self-contained function like this I don't know that it matters much. -Peff
Hi Rubén On 29/04/2024 19:37, Rubén Justo wrote: > There is no need to show some UI messages on stderr, and yet doing so > may produce some undesirable results, such as messages appearing in an > unexpected order. > > Let's use stdout for all UI messages, and adjusts the tests accordingly. The test changes in "warn add.interactive.useBultin" show that we still print a warning to stderr, I don't think that particular case is worth worrying about though. > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > add-patch.c | 13 ++++++------- > t/t3701-add-interactive.sh | 12 ++++++------ > 2 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/add-patch.c b/add-patch.c > index 0997d4af73..fc0eed4fd4 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) > va_list args; > > va_start(args, fmt); > - fputs(s->s.error_color, stderr); > - vfprintf(stderr, fmt, args); > - fputs(s->s.reset_color, stderr); > - fputc('\n', stderr); > + fputs(s->s.error_color, stdout); > + vprintf(fmt, args); > + puts(s->s.reset_color); > va_end(args); > } This looks like a good change > @@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff, > err(s, _("Nothing was applied.\n")); > } else > /* As a last resort, show the diff to the user */ > - fwrite(diff->buf, diff->len, 1, stderr); > + fwrite(diff->buf, diff->len, 1, stdout); This seems reasonable as we'd print the "Nothing was applied" error message above to stdout now. Anything "git apply" writes to stderr should be flushed when it exits before we print these messages. > > return 0; > } > @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > break; > > if (s.file_diff_nr == 0) > - fprintf(stderr, _("No changes.\n")); > + err(&s, _("No changes.")); > else if (binary_count == s.file_diff_nr) > - fprintf(stderr, _("Only binary files changed.\n")); > + err(&s, _("Only binary files changed.")); These two mean we'll now color these messages which we didn't do before. I think if we hit this code we don't print anything else (apart from the warning about add.interactive.useBuiltin being removed) so it probably does not matter whether we use stdout or stderr here and I don't have a strong opinion either way. Best Wishes Phillip > add_p_state_clear(&s); > return 0; > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 04d8333373..c5531520cb 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' ' > cat >expect <<-\EOF && > warning: the add.interactive.useBuiltin setting has been removed! > See its entry in '\''git help config'\'' for details. > - No changes. > EOF > + echo "No changes." >expect.out && > > for v in = =true =false > do > git -c "add.interactive.useBuiltin$v" add -p >out 2>actual && > - test_must_be_empty out && > + test_cmp expect.out out && > test_cmp expect actual || return 1 > done > ' > @@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' ' > > test_expect_success 'correct message when there is nothing to do' ' > git reset --hard && > - git add -p 2>err && > - test_grep "No changes" err && > + git add -p >out && > + test_grep "No changes" out && > printf "\\0123" >binary && > git add binary && > printf "\\0abc" >binary && > - git add -p 2>err && > - test_grep "Only binary files changed" err > + git add -p >out && > + test_grep "Only binary files changed" out > ' > > test_expect_success 'setup again' '
On Tue, Apr 30, 2024 at 06:52:56AM -0400, Jeff King wrote: > On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote: > > > Rubén Justo <rjusto@gmail.com> writes: > > > > > diff --git a/add-patch.c b/add-patch.c > > > index 0997d4af73..fc0eed4fd4 100644 > > > --- a/add-patch.c > > > +++ b/add-patch.c > > > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) > > > va_list args; > > > > > > va_start(args, fmt); > > > - fputs(s->s.error_color, stderr); > > > - vfprintf(stderr, fmt, args); > > > - fputs(s->s.reset_color, stderr); > > > - fputc('\n', stderr); > > > + fputs(s->s.error_color, stdout); > > > + vprintf(fmt, args); > > > + puts(s->s.reset_color); > > > > I like the attention of the detail here ;-). > > Indeed. I had to read this several times to wonder why it was not a > mistake to leave the first fputs() but use vprintf() and puts() for the > other two (for those just reading, the answer is that puts() prints an > extra newline, so we can only use it at the end). I did not know the details (or had happily forgotten them) but Junio ignoring my comments in [1] intrigued me :-). A simple test would have been quick, but "man puts" was quicker; my comments were not correct. Just to be clear, and to extend your comment, in case any reader is interested, this: vfprintf(stdout, fmt, args); can be written as follows: vprintf(fmt, args); And this: fputs(s->s.reset_color, stdout); fputc('\n', stdout); can be shortened to: puts(s->s.reset_color); The difference in vfprintf and vprintf is quite obvious IMHO, but not so obvious with puts. The documentation says: SYNOPSIS int puts(const char *s); DESCRIPTION puts() writes the string s and a trailing newline to stdout. [1] - https://lore.kernel.org/git/4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com/T/#m644a682364212729a2b21c052ef744039c26f972
On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote: > > @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > > break; > > if (s.file_diff_nr == 0) > > - fprintf(stderr, _("No changes.\n")); > > + err(&s, _("No changes.")); > > else if (binary_count == s.file_diff_nr) > > - fprintf(stderr, _("Only binary files changed.\n")); > > + err(&s, _("Only binary files changed.")); > > These two mean we'll now color these messages which we didn't do before. I > think if we hit this code we don't print anything else (apart from the > warning about add.interactive.useBuiltin being removed) so it probably does > not matter whether we use stdout or stderr here and I don't have a strong > opinion either way. Can we consider those messages not part of the UI? IIUC, if we hit that code we haven't entered in the interactive UI. Maybe we should: diff --git a/add-patch.c b/add-patch.c index c28ad380ed..b11a435738 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, break; if (s.file_diff_nr == 0) - err(&s, _("No changes.")); + error(_("no changes")); else if (binary_count == s.file_diff_nr) - err(&s, _("Only binary files changed.")); + error(_("only binary files changed")); add_p_state_clear(&s); return 0; Or, simply leave them untouched in this series. > > Best Wishes > > Phillip Thanks.
Jeff King <peff@peff.net> writes: > I am tempted to suggest that it borders on too-clever, and writing out > "stdout" everywhere with vfprintf() and fputs() would be more obvious. > But in a little self-contained function like this I don't know that it > matters much. I share your preference (and that was the way I wrote "it would look like this" version earlier) but I agree that this is too small to matter. Thanks.
Rubén Justo <rjusto@gmail.com> writes: >> Indeed. I had to read this several times to wonder why it was not a >> mistake to leave the first fputs() but use vprintf() and puts() for the >> other two (for those just reading, the answer is that puts() prints an >> extra newline, so we can only use it at the end). > > I did not know the details (or had happily forgotten them) but Junio > ignoring my comments in [1] intrigued me :-). A simple test would have > been quick, but "man puts" was quicker; my comments were not correct. I thought you were suggesting to fold fputs()+putchar('\n') into puts(), which is a change that does not break. I didn't even know that you were suggesting to make more changes that would break the output. After all, you said only "can be less explicit" without saying which use of "stdout" you meant to make "less explicit" ;-).
Hi Rubén On 30/04/2024 17:38, Rubén Justo wrote: > On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote: > >>> @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, >>> break; >>> if (s.file_diff_nr == 0) >>> - fprintf(stderr, _("No changes.\n")); >>> + err(&s, _("No changes.")); >>> else if (binary_count == s.file_diff_nr) >>> - fprintf(stderr, _("Only binary files changed.\n")); >>> + err(&s, _("Only binary files changed.")); >> >> These two mean we'll now color these messages which we didn't do before. I >> think if we hit this code we don't print anything else (apart from the >> warning about add.interactive.useBuiltin being removed) so it probably does >> not matter whether we use stdout or stderr here and I don't have a strong >> opinion either way. > > Can we consider those messages not part of the UI? IIUC, if we hit that > code we haven't entered in the interactive UI. I thikn so > Maybe we should: > > diff --git a/add-patch.c b/add-patch.c > index c28ad380ed..b11a435738 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > break; > > if (s.file_diff_nr == 0) > - err(&s, _("No changes.")); > + error(_("no changes")); > else if (binary_count == s.file_diff_nr) > - err(&s, _("Only binary files changed.")); > + error(_("only binary files changed")); > > add_p_state_clear(&s); > return 0; > > Or, simply leave them untouched in this series. Either option sounds good to me Best Wishes Phillip
phillip.wood123@gmail.com writes: >>>> - fprintf(stderr, _("No changes.\n")); >>>> + err(&s, _("No changes.")); >>>> else if (binary_count == s.file_diff_nr) >>>> - fprintf(stderr, _("Only binary files changed.\n")); >>>> + err(&s, _("Only binary files changed.")); >>> >>> These two mean we'll now color these messages which we didn't do before. I > ... > I think so > ... >> - err(&s, _("Only binary files changed.")); >> + error(_("only binary files changed")); >> add_p_state_clear(&s); >> return 0; >> Or, simply leave them untouched in this series. > > Either option sounds good to me We are returning "success" to the caller, so using error() here is a bit strong. Judging from how other messages emitted with err() in this program is meant to help the end user, they are all to tell the user why their input caused no actual change, and showing these two messages the same way as these other messages would be the best for consistency. So I'm inclined to say that what was posted is good. If it paints these two messages in the same color as others, that is even getter.
On Wed, May 01, 2024 at 09:14:37AM -0700, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > > >>>> - fprintf(stderr, _("No changes.\n")); > >>>> + err(&s, _("No changes.")); > >>>> else if (binary_count == s.file_diff_nr) > >>>> - fprintf(stderr, _("Only binary files changed.\n")); > >>>> + err(&s, _("Only binary files changed.")); > >>> > >>> These two mean we'll now color these messages which we didn't do before. I > > ... > > I think so > > ... > >> - err(&s, _("Only binary files changed.")); > >> + error(_("only binary files changed")); > >> add_p_state_clear(&s); > >> return 0; > >> Or, simply leave them untouched in this series. > > > > Either option sounds good to me > > We are returning "success" to the caller, so using error() here is a > bit strong. Judging from how other messages emitted with err() in > this program is meant to help the end user, they are all to tell the > user why their input caused no actual change, and showing these two > messages the same way as these other messages would be the best for > consistency. > > So I'm inclined to say that what was posted is good. If it paints > these two messages in the same color as others, that is even getter. Having: $ printf "\0" >binary $ git add -N binary Displaying the message in RED does not seem as a bad change: $ git add -i staged unstaged path 1: +0/-0 binary binary *** Commands *** 1: status 2: update 3: revert 4: add untracked 5: patch 6: diff 7: quit 8: help What now> p <RED>Only binary files changed.<RED> However, here could be disturbing: $ git add -p <RED>Only binary files changed.<RED> And the "No changes." message is going to have even more audience, I think. Perhaps this seems sensible: Range-diff against v5: 2: b0f042f4ff ! 1: 94c3f80041 add-patch: do not show UI messages on stderr @@ add-patch.c: int run_add_p(struct repository *r, enum add_p_mode mode, if (s.file_diff_nr == 0) - fprintf(stderr, _("No changes.\n")); -+ err(&s, _("No changes.")); ++ fprintf(stdout, _("No changes.\n")); else if (binary_count == s.file_diff_nr) - fprintf(stderr, _("Only binary files changed.\n")); -+ err(&s, _("Only binary files changed.")); ++ fprintf(stdout, _("Only binary files changed.\n")); add_p_state_clear(&s); return 0; 1: 4a0eb1337f = 2: abaf904e8c add-patch: response to unknown command But I'm not sure and I do not want to send a new iteration unless it is necessary. I was already happy with previous versions ;-).
Rubén Justo <rjusto@gmail.com> writes: > But I'm not sure and I do not want to send a new iteration unless it is > necessary. I was already happy with previous versions ;-). Let's call it done-well-enough and merge it down. Thanks.
diff --git a/add-patch.c b/add-patch.c index 0997d4af73..fc0eed4fd4 100644 --- a/add-patch.c +++ b/add-patch.c @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) va_list args; va_start(args, fmt); - fputs(s->s.error_color, stderr); - vfprintf(stderr, fmt, args); - fputs(s->s.reset_color, stderr); - fputc('\n', stderr); + fputs(s->s.error_color, stdout); + vprintf(fmt, args); + puts(s->s.reset_color); va_end(args); } @@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff, err(s, _("Nothing was applied.\n")); } else /* As a last resort, show the diff to the user */ - fwrite(diff->buf, diff->len, 1, stderr); + fwrite(diff->buf, diff->len, 1, stdout); return 0; } @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, break; if (s.file_diff_nr == 0) - fprintf(stderr, _("No changes.\n")); + err(&s, _("No changes.")); else if (binary_count == s.file_diff_nr) - fprintf(stderr, _("Only binary files changed.\n")); + err(&s, _("Only binary files changed.")); add_p_state_clear(&s); return 0; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 04d8333373..c5531520cb 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' ' cat >expect <<-\EOF && warning: the add.interactive.useBuiltin setting has been removed! See its entry in '\''git help config'\'' for details. - No changes. EOF + echo "No changes." >expect.out && for v in = =true =false do git -c "add.interactive.useBuiltin$v" add -p >out 2>actual && - test_must_be_empty out && + test_cmp expect.out out && test_cmp expect actual || return 1 done ' @@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' ' test_expect_success 'correct message when there is nothing to do' ' git reset --hard && - git add -p 2>err && - test_grep "No changes" err && + git add -p >out && + test_grep "No changes" out && printf "\\0123" >binary && git add binary && printf "\\0abc" >binary && - git add -p 2>err && - test_grep "Only binary files changed" err + git add -p >out && + test_grep "Only binary files changed" out ' test_expect_success 'setup again' '
There is no need to show some UI messages on stderr, and yet doing so may produce some undesirable results, such as messages appearing in an unexpected order. Let's use stdout for all UI messages, and adjusts the tests accordingly. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- add-patch.c | 13 ++++++------- t/t3701-add-interactive.sh | 12 ++++++------ 2 files changed, 12 insertions(+), 13 deletions(-)