Re: [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 20, 2017, 17:35 UTC
Message-ID: <alpine.DEB.2.21.1.1712201833420.406@MININT-6BKU6QN.europe.corp.microsoft.com>
In-Reply-To: <xmqqpo7av5au.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 19 Dec 2017, Junio C Hamano wrote:

Show 31 quoted lines

> Alex Vandiver <alexmv@dropbox.com> writes:
> 
> > These were mistakenly left in when the test was introduced, in
> > 1487372d3 ("fsmonitor: store fsmonitor bitmap before splitting index",
> > 2017-11-09)
> >
> > Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> > ---
> >  t/t7519-status-fsmonitor.sh | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> > index eb2d13bbc..19b2a0a0f 100755
> > --- a/t/t7519-status-fsmonitor.sh
> > +++ b/t/t7519-status-fsmonitor.sh
> > @@ -307,9 +307,7 @@ test_expect_success 'splitting the index results in the same state' '
> >  	dirty_repo &&
> >  	git update-index --fsmonitor  &&
> >  	git ls-files -f >expect &&
> > -	test-dump-fsmonitor >&2 && echo &&
> >  	git update-index --fsmonitor --split-index &&
> > -	test-dump-fsmonitor >&2 && echo &&
> >  	git ls-files -f >actual &&
> >  	test_cmp expect actual
> >  '
> 
> Hmph, by default the standard output and standard error streams are
> not shown in the test output, and it would help while debugging test
> failures, so I am not sure if this is a good change.  With the
> previous step [4/6], we can lose the "echo", of course, and I do not
> think we need >&2 redirection there, either.

I think you got it backwards. Sure, this helps debugging, but it hurts runtime of the entire test suite (which I might have happened to mention a couple of times takes way too long on Windows, thanks to our choice of test "framework").

And in the bigger picture, I think that it is very, very easy to insert those debugging statements when something breaks (we have to do that with other tests, anyways).

So I am in favor of this patch, and disagree with your assessment, Junio.

Ciao, Dscho