Re: [PATCH 1/3] t0033: add tests for safe.directory

[Derrick Stolee <derrickstolee@github.com>]

On 4/13/2022 3:16 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> It is difficult to change the ownership on a directory in our test
>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
>> variable to trick Git into thinking we are in a differently-owned
>> directory. This allows us to test that the config is parsed correctly.
> 
> I think this is a good trade-off, but FWIW I'd think we could test also
> without the git_env_bool() by having the test depend on !NOT_ROOT, then
> check the owner of t/test-lib.sh, and chown to that user (i.e. the
> "real" user).
> 
> But that's all sorts of more fragile than just this test variable..
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> 
> Instead of this "export" perhaps just add it in front of the "git
> status"?

If the only runs were in this helper below, then yes.

>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}

Later patches add more success cases that run 'git status'
as its verification that the match works. I didn't think it
was good to have this environment variable set for each of
those invocations.

This script has one purpose, and this environment variable
is required to make any of the checks work. Setting it
globally seems the best way to do that.

>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
> 
> nit: $PWD instead of $(pwd)

Historically, $PWD doesn't work properly across platforms,
so I have used $(pwd) consistently across many contributions.

Thanks,
-Stolee