Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Apr 29, 2022, 19:06 UTC
Message-ID: <20220429190625.GA1626@szeder.dev>
In-Reply-To: <xmqq8rrndb6h.fsf@gitster.g>

On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:

Show 54 quoted lines

> Derrick Stolee <derrickstolee@github.com> writes:
> 
> > On 4/27/2022 4:37 PM, Junio C Hamano wrote:
> >> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >> 
> >>> According to the documentation 'safe.directory' "is only respected
> >>> when specified in a system or global config, not when it is specified
> >>> in a repository config or via the command line option -c
> >>> safe.directory=<path>".
> >>>
> >>> Add tests to check that 'safe.directory' in the repository config or
> >>> on the command line is indeed ignored.
> >>>
> >>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> >>> ---
> >>>  t/t0033-safe-directory.sh | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> >>> index 6f9680e8b0..82dac0eb93 100755
> >>> --- a/t/t0033-safe-directory.sh
> >>> +++ b/t/t0033-safe-directory.sh
> >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
> >>>  	expect_rejected_dir
> >>>  '
> >>>  
> >>> +test_expect_success 'ignoring safe.directory on the command line' '
> >>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
> >>> +	grep "unsafe repository" err
> >>> +'
> >>> +
> >>> +test_expect_success 'ignoring safe.directory in repo config' '
> >>> +	(
> >>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> >>> +		git config safe.directory "$(pwd)"
> >>> +	) &&
> >>> +	expect_rejected_dir
> >>> +'
> >> 
> >> I am debating myself if we want to remove the in-repository
> >> safe.directory configuration setting after this test piece is done,
> >> with test_when_finished.  We just made sure, with this test, that
> >> having the variable does not affect anything, so the subsequent
> >> tests should not care hence it is probably OK.  On the other hand,
> >> to make sure those settings they make (e.g. setting it globally is
> >> what the next test we can see below does) is what affects their
> >> outcome, it removes one more thing we need to worry about if we
> >> clean after ourselves.  I dunno.
> >
> > test_config would do the same, right? I think it automatically
> > does the test_when_finished for us.
> 
> I thought it (specifically, anything depends on test_when_finished)
> has trouble working well from inside a subprocess?

Yeah, test_config doesn't work in a subshell, because modifying the variable holding the cleanup commands won't be visible after leaving the subshell, and the protection added in 0968f12a99 (test-lib-functions: detect test_when_finished in subshell, 2015-09-05) will kick in. And we do need a subshell to set the config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git config' would refuse to touch the config file.

I think something like

  test_when_finished "(
        unset GIT_TEST_ASSUME_DIFFERENT_USER &&
        git config --unset safe.directory
        )"

would work, though.

I considered cleaning up the config file, but all subsequent tests leave behind config settings for later tests as well (in the global config file, though).