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....@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
