Re: [PATCH v2 1/9] t7006: replace dubious test

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 3, 2017, 11:10 UTC
Message-ID: <alpine.DEB.2.20.1703031208430.3767@virtualbox>
In-Reply-To: <20170303033623.lfi7jhul6psbdde6@sigill.intra.peff.net>

Hi Peff,

On Thu, 2 Mar 2017, Jeff King wrote:

Show 17 quoted lines

> On Fri, Mar 03, 2017 at 03:04:02AM +0100, Johannes Schindelin wrote:
> 
> > The intention of that test case, however, was to verify that the
> > setup_git_directory() function has not run, because it changes global
> > state such as the current working directory.
> > 
> > To keep that spirit, but fix the incorrect assumption, this patch
> > replaces that test case by a new one that verifies that the pager is
> > run in the subdirectory, i.e. that the current working directory has
> > not been changed at the time the pager is configured and launched,
> > even if the `rev-parse` command requires a .git/ directory and *will*
> > change the working directory.
> 
> This is a great solution to the question I had in v1 ("how do we know
> when setup_git_directory() is run?"). It not only checks the
> internal-code case that we care about, but it also shows how real users
> would get bit if we do the wrong thing.

Thanks!

Show 12 quoted lines

> > +test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
> > +	sane_unset GIT_PAGER &&
> > +	test_config core.pager "cat >cwd-retained" &&
> > +	(
> > +		cd sub &&
> > +		rm -f cwd-retained &&
> > +		test_terminal git -p rev-parse HEAD &&
> > +		test -e cwd-retained
> > +	)
> > +'
> 
> Does this actually need TTY and test_terminal?

Sadly, yes. If git.c sees a --paginate or -p, it sets use_pager to 1 which is later used as indicator that setup_pager() should be run. And the first line of that function reads:

	const char *pager = git_pager(isatty(1));

i.e. it does *not* turn on the pager unconditionally.

> (Also a minor nit: we usually prefer test_path_is_file to "test -e"
> these days).

Thanks, fixed.

Ciao, Dscho