On Sat, Apr 29, 2023 at 07:34:48PM +0200, SZEDER Gábor wrote:
> On Thu, Apr 27, 2023 at 01:13:08PM +0200, Patrick Steinhardt wrote:
> > We're about to introduce a new porcelain mode for the output of
> > git-fetch(1). As part of that we'll be introducing a set of new tests
> > that only relate to the output of this command.
> > 
> > Split out tests that exercise the output format of git-fetch(1) so that
> > it becomes easier to verify this functionality as a standalone unit. As
> > the tests assume that the default branch is called "main" we set up the
> > corresponding GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable
> > accordingly.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t5510-fetch.sh        | 53 ----------------------------------
> >  t/t5574-fetch-output.sh | 63 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+), 53 deletions(-)
> >  create mode 100755 t/t5574-fetch-output.sh
> 
> 
> > diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> > new file mode 100755
> > index 0000000000..f91b654d38
> > --- /dev/null
> > +++ b/t/t5574-fetch-output.sh
> > @@ -0,0 +1,63 @@
> > +#!/bin/sh
> > +
> > +test_description='git fetch output format'
> > +
> > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'fetch aligned output' '
> > +	git clone . full-output &&
> > +	test_commit looooooooooooong-tag &&
> > +	(
> > +		cd full-output &&
> > +		git -c fetch.output=full fetch origin >actual 2>&1 &&
> 
> Why is that 2>&1 redirection used here?
> If the output the test case is interested in goes to the command's
> standard output, then it's unnecessary.  However, if it goes to
> standard error, then why is standard output redirected as well?
> 
> I understand that this patch just moves existing test cases around
> as-is, so this is not something you introduced, but I point it out
> here, because later patches of this series add several new test cases
> following this anti-pattern.
> 
> Since this series creates a new test script, perhaps this might be the
> right time to clean this up.

I feel like this patch series is big enough on its own already, so for
now I'd like to refrain from touching up the old tests. But you've got a
good point here, and I'll make sure that any new tests don't followw the
same pattern.

That being said, I think we should keep on testing both stdout and
stderr. The tests here are explicitly about the output format, so it
does make sense to verify that both of them match what we expect. We
should treat them as separate things though.

Patrick