Re: [PATCH] t: send verbose test-helper output to fd 4

From: Jeff King <peff@peff.net>
Date: Mar 3, 2018, 06:56 UTC
Message-ID: <20180303065648.GA17312@sigill.intra.peff.net>
In-Reply-To: <CAM0VKjmtHEGmDi8x_AC8JoNj7rtcanSJX1n0b-f1CEX04Xn8CA@mail.gmail.com>

On Wed, Feb 28, 2018 at 03:30:29AM +0100, SZEDER Gábor wrote:

Show 15 quoted lines

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

Show 17 quoted lines

> >  # 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).

Show 5 quoted lines

> 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