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@xxxxxxxxx/ > > 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
