diff mbox series

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

Message ID 5b18bd1852d673ab5c62a67f873987d74294cd70.1649863951.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Updates to the safe.directory config option | expand

Commit Message

Derrick Stolee April 13, 2022, 3:32 p.m. UTC 
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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 setup.c                   |  3 ++-
 t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100755 t/t0033-safe-directory.sh

Comments

Junio C Hamano April 13, 2022, 4:24 p.m. UTC | #1 
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

OK.

> -	if (is_path_owned_by_current_user(path))
> +	if (is_path_owned_by_current_user(path) &&
> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>  		return 1;

Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
real logic kicks in, I wonder?

> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> new file mode 100755
> index 00000000000..9380ff3d017
> --- /dev/null
> +++ b/t/t0033-safe-directory.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='verify safe.directory checks'
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> +
> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +...
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&
> +	git status
> +'

Just double checking, as I know you are much closer to the affected
platform than I'd ever be ;-) but is the use of $(pwd) safe and
correct here?

I always get confused between $(pwd) and $PWD, which does not make
any difference on platforms I have access to, but makes difference
to hurt Windows users.

> +test_expect_success 'safe.directory matches, but is reset' '
> +	git config --global --add safe.directory "" &&
> +	expect_rejected_dir
> +'
> +
> +test_done

Thanks.  This step should apply to maint-2.30 cleanly, I would
think.
Derrick Stolee April 13, 2022, 4:29 p.m. UTC | #2 
On 4/13/2022 12:24 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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.
> 
> OK.
> 
>> -	if (is_path_owned_by_current_user(path))
>> +	if (is_path_owned_by_current_user(path) &&
>> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>>  		return 1;
> 
> Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
> real logic kicks in, I wonder?

Either order would work. I bet that checking the environment is
faster than checking the disk, so swapping the order would be prudent
here.
 
>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>> new file mode 100755
>> index 00000000000..9380ff3d017
>> --- /dev/null
>> +++ b/t/t0033-safe-directory.sh
>> @@ -0,0 +1,34 @@
>> +#!/bin/sh
>> +
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
>> +
>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}
>> +...
>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
>> +	git status
>> +'
> 
> Just double checking, as I know you are much closer to the affected
> platform than I'd ever be ;-) but is the use of $(pwd) safe and
> correct here?
> 
> I always get confused between $(pwd) and $PWD, which does not make
> any difference on platforms I have access to, but makes difference
> to hurt Windows users.

These tests pass CI on Windows. I've had issues before using $PWD,
thinking back to tests in t7900-maintenance.sh that use $(pwd)
instead.

Thanks,
-Stolee
Ævar Arnfjörð Bjarmason April 13, 2022, 7:16 p.m. UTC | #3 
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"?

These tests also pass with SANITIZE=leak, so please add
TEST_PASSES_SANITIZE_LEAK=true at the top.

> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +
> +test_expect_success 'safe.directory is not set' '
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory does not match' '
> +	git config --global safe.directory bogus &&
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&

nit: $PWD instead of $(pwd)
Junio C Hamano April 13, 2022, 7:46 p.m. UTC | #4 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

(just this part)

> These tests also pass with SANITIZE=leak, so please add
> TEST_PASSES_SANITIZE_LEAK=true at the top.

Derrick, please ignore the above.  It is totally outside the scope
of these patches, and they are meant to be applied on top of the
2.30 maintenance track, where TEST_PASSES_SANITIZE_LEAK=true was
irrelevant.

I do not mind adding such after the dust settles on top of 'master'
(or possibly 'maint'), but not as part of these "let's fix the screw
up in 2.30.3 and its friends" effort.

Thanks.
Derrick Stolee April 13, 2022, 7:52 p.m. UTC | #5 
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
diff mbox series

Patch

diff --git a/setup.c b/setup.c
index c8f67bfed52..f54f449008a 100644
--- a/setup.c
+++ b/setup.c
@@ -1119,7 +1119,8 @@  static int ensure_valid_ownership(const char *path)
 {
 	struct safe_directory_data data = { .path = path };
 
-	if (is_path_owned_by_current_user(path))
+	if (is_path_owned_by_current_user(path) &&
+	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
 		return 1;
 
 	read_very_early_config(safe_directory_cb, &data);
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
new file mode 100755
index 00000000000..9380ff3d017
--- /dev/null
+++ b/t/t0033-safe-directory.sh
@@ -0,0 +1,34 @@ 
+#!/bin/sh
+
+test_description='verify safe.directory checks'
+
+. ./test-lib.sh
+
+GIT_TEST_ASSUME_DIFFERENT_OWNER=1
+export GIT_TEST_ASSUME_DIFFERENT_OWNER
+
+expect_rejected_dir () {
+	test_must_fail git status 2>err &&
+	grep "safe.directory" err
+}
+
+test_expect_success 'safe.directory is not set' '
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory does not match' '
+	git config --global safe.directory bogus &&
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory matches' '
+	git config --global --add safe.directory "$(pwd)" &&
+	git status
+'
+
+test_expect_success 'safe.directory matches, but is reset' '
+	git config --global --add safe.directory "" &&
+	expect_rejected_dir
+'
+
+test_done