From: phillip.wood123@gmail.com
To: "Rubén Justo" <rjusto@gmail.com>, "Git List" <git@vger.kernel.org>
Cc: Phillip Wood <phillip.wood@dunelm.org.uk>,
Patrick Steinhardt <ps@pks.im>,
Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr
Date: Tue, 30 Apr 2024 15:47:04 +0100 [thread overview]
Message-ID: <27fbb12a-f2d2-459c-a27b-519f69242105@gmail.com> (raw)
In-Reply-To: <10905ab3-bb3c-4669-9177-84c8e6759616@gmail.com>
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' '
next prev parent reply other threads:[~2024-04-30 14:47 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 19:00 [PATCH] add-patch: response to invalid option Rubén Justo
2024-04-16 5:51 ` Patrick Steinhardt
2024-04-16 19:11 ` Rubén Justo
2024-04-16 9:41 ` phillip.wood123
2024-04-16 19:24 ` Rubén Justo
2024-04-17 9:37 ` phillip.wood123
2024-04-17 15:05 ` Junio C Hamano
2024-04-18 15:11 ` phillip.wood123
2024-04-16 10:26 ` Junio C Hamano
2024-04-16 13:56 ` Phillip Wood
2024-04-16 15:22 ` Junio C Hamano
2024-04-16 15:46 ` Phillip Wood
2024-04-16 16:10 ` Junio C Hamano
2024-04-16 19:31 ` Rubén Justo
2024-04-20 11:08 ` [PATCH v2] add-patch: response to invalid command Rubén Justo
2024-04-20 17:53 ` Junio C Hamano
2024-04-21 9:51 ` [PATCH v3] add-patch: response to unknown command Rubén Justo
2024-04-21 13:18 ` phillip.wood123
2024-04-21 19:37 ` Rubén Justo
2024-04-21 21:52 ` [PATCH v4] " Rubén Justo
2024-04-22 15:50 ` Junio C Hamano
2024-04-24 10:15 ` phillip.wood123
2024-04-24 15:35 ` Junio C Hamano
2024-04-29 9:48 ` Phillip Wood
2024-04-29 16:09 ` Junio C Hamano
2024-04-25 1:44 ` Jeff King
2024-04-25 2:15 ` Eric Sunshine
2024-04-25 20:23 ` Junio C Hamano
2024-04-25 21:00 ` Eric Sunshine
2024-04-25 21:13 ` Junio C Hamano
2024-04-25 21:05 ` Junio C Hamano
2024-04-25 22:09 ` Rubén Justo
2024-04-25 22:16 ` Junio C Hamano
2024-04-25 23:46 ` Rubén Justo
2024-04-26 5:39 ` Junio C Hamano
2024-04-26 16:26 ` Junio C Hamano
2024-04-26 20:21 ` Jeff King
2024-04-25 3:04 ` Rubén Justo
2024-04-26 20:23 ` Jeff King
2024-04-26 20:41 ` Rubén Justo
2024-04-25 8:53 ` phillip.wood123
2024-04-29 18:35 ` [PATCH v5 0/2] " Rubén Justo
2024-04-29 18:37 ` [PATCH v5 1/2] add-patch: do not show UI messages on stderr Rubén Justo
2024-04-29 19:24 ` Junio C Hamano
2024-04-30 10:52 ` Jeff King
2024-04-30 16:35 ` Rubén Justo
2024-04-30 17:17 ` Junio C Hamano
2024-04-30 17:11 ` Junio C Hamano
2024-04-30 14:47 ` phillip.wood123 [this message]
2024-04-30 16:38 ` Rubén Justo
2024-05-01 15:39 ` phillip.wood123
2024-05-01 16:14 ` Junio C Hamano
2024-05-01 21:13 ` Rubén Justo
2024-05-02 16:38 ` Junio C Hamano
2024-04-29 18:37 ` [PATCH v5 2/2] add-patch: response to unknown command Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27fbb12a-f2d2-459c-a27b-519f69242105@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
--cc=ps@pks.im \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).