Hi Peff,
On Thu, 2 Mar 2017, Jeff King wrote:
> 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!
> > +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