From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Git mailing list <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] t: send verbose test-helper output to fd 4
Date: Sat, 3 Mar 2018 01:56:49 -0500 [thread overview]
Message-ID: <20180303065648.GA17312@sigill.intra.peff.net> (raw)
In-Reply-To: <CAM0VKjmtHEGmDi8x_AC8JoNj7rtcanSJX1n0b-f1CEX04Xn8CA@mail.gmail.com>
On Wed, Feb 28, 2018 at 03:30:29AM +0100, SZEDER Gábor wrote:
> > - echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
> > + echo >&4 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
> > return 1
> > }
>
> The parts above changing the fds used for error messages in
> 'test_must_fail' and 'test_expect_code' will become unnecessary if we
> take my patch from
>
> https://public-inbox.org/git/20180225134015.26964-1-szeder.dev@gmail.com/
>
> because that patch also ensures that error messages from this function
> will go to fd 4 even if the stderr of the functions is redirected in the
> test. Though, arguably, your patch makes it more readily visible that
> those error messages go to fd 4, so maybe it's still worth having.
Yeah, I could take it or leave it if we follow that other route.
> > # not output anything when they fail.
> > verbose () {
> > "$@" && return 0
> > - echo >&2 "command failed: $(git rev-parse --sq-quote "$@")"
> > + echo >&4 "command failed: $(git rev-parse --sq-quote "$@")"
> > return 1
> > }
>
> I'm not so sure about these changes to 'test_18ngrep' and 'verbose'. It
> seems that they are the result of a simple s/>&2/>&4/ on
> 'test-lib-functions.sh'? The problem described in the commit message
> doesn't really applies to them, because their _only_ output to stderr
> are the error messages intended to the user, so no sane test would
> redirect and check their stderr. (OK, technically the command run by
> 'verbose' could output anything to stderr, but 'verbose' was meant for
> commands failing silently in the first place; that's why my patch linked
> above left 'verbose' unchanged.)
Yes, I agree it's not likely to help anybody. I did it mostly for
consistency. I.e., to say "and we are meaning to send this directly to
the user". It actually might make sense to wrap that concept in a helper
function. Arguably "say" should do this redirection automatically (we do
redirect it elsewhere, but mostly to try to accomplish the same thing).
> Also note that there are a lot of other test helper functions that
> output something primarily intended to the user on failure (e.g.
> 'test_path_is_*' and the like), though those messages go stdout instead
> of stderr. Perhaps the messages of those functions should go to stderr
> as well (or even fd 4)?
Yeah, I just missed those. I agree they should redirect to fd 4, too.
I'm trying to clear my inbox before traveling, so I probably won't dig
into this further for a while. If you think this is the right direction,
do you want to add more ">&4" redirects to helpers as part of your
series?
-Peff
prev parent reply other threads:[~2018-03-03 6:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-22 6:48 [PATCH] t: send verbose test-helper output to fd 4 Jeff King
2018-02-22 20:18 ` Junio C Hamano
2018-02-28 2:30 ` SZEDER Gábor
2018-03-03 6:56 ` Jeff King [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180303065648.GA17312@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=szeder.dev@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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).