From: Jeff King <peff@peff.net>
Date: Fri, 03 Mar 2017 03:36:23 GMT
Subject: Re: [PATCH v2 1/9] t7006: replace dubious test
Message-ID: <20170303033623.lfi7jhul6psbdde6@sigill.intra.peff.net>
In-Reply-To: <8f79df34e63a37c052437dd022269b4a6c495108.1488506615.git.johannes.schindelin@gmx.de>

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.

> +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? You replace the pager
with something that doesn't care about the terminal, and "-p" should
unconditionally turn it on.

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

-Peff