[PATCH] tests: A new test prereq for testing chmod -w as root

[PATCH] tests: A new test prereq for testing chmod -w as root

[Ævar Arnfjörð Bjarmason]

Some tests depend on not being able to read files after chmod -w. This
doesn't work when running the tests as root.

Change test-lib.sh to test if this works, and if so it sets a new
CHMOD_0000 test prerequisite. The tests that use this previously
failed when run under root.

There was already a test for this in t3600-rm.sh added by Junio C
Hamano in 2283645b85 in 2006. That check now uses the new CHMOD_0000
prerequisite.

There are also two other prerequisites, "POSIXPERM_AND_CHMOD_0000" and
"SYMLINKS_AND_CHMOD_0000". They're being used for tests that failed
and already depended on POSIXPERM or SYMLINKS. They're possibly
redundant, but I don't have access to a system without POSIX
permissions or symlinks so I couldn't test that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0001-init.sh             |    2 +-
 t/t0004-unwritable.sh       |    8 ++++----
 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/t3600-rm.sh               |   16 ----------------
 t/t3700-add.sh              |   10 +++++-----
 t/t7300-clean.sh            |    7 +++----
 t/t7508-status.sh           |    2 +-
 t/test-lib.sh               |   14 ++++++++++++++
 8 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 7c0a698..7c62582 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -301,7 +301,7 @@ test_expect_success 'init notices EEXIST (2)' '
 	)
 '
 
-test_expect_success POSIXPERM 'init notices EPERM' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'init notices EPERM' '
 	rm -fr newdir &&
 	(
 		mkdir newdir &&
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 2342ac5..1299f52 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -15,7 +15,7 @@ test_expect_success setup '
 
 '
 
-test_expect_success POSIXPERM 'write-tree should notice unwritable repository' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'write-tree should notice unwritable repository' '
 
 	(
 		chmod a-w .git/objects .git/objects/?? &&
@@ -27,7 +27,7 @@ test_expect_success POSIXPERM 'write-tree should notice unwritable repository' '
 
 '
 
-test_expect_success POSIXPERM 'commit should notice unwritable repository' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'commit should notice unwritable repository' '
 
 	(
 		chmod a-w .git/objects .git/objects/?? &&
@@ -39,7 +39,7 @@ test_expect_success POSIXPERM 'commit should notice unwritable repository' '
 
 '
 
-test_expect_success POSIXPERM 'update-index should notice unwritable repository' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'update-index should notice unwritable repository' '
 
 	(
 		echo 6O >file &&
@@ -52,7 +52,7 @@ test_expect_success POSIXPERM 'update-index should notice unwritable repository'
 
 '
 
-test_expect_success POSIXPERM 'add should notice unwritable repository' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'add should notice unwritable repository' '
 
 	(
 		echo b >file &&
diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index f19b4a2..ceb0fd4 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '
 
 '
 
-test_expect_success SYMLINKS 'funny symlink in work tree, un-unlink-able' '
+test_expect_success SYMLINKS_AND_CHMOD_0000 'funny symlink in work tree, un-unlink-able' '
 
 	rm -fr a b &&
 	git reset --hard &&
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b514cbb..b26cabd 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -28,22 +28,6 @@ embedded' &&
      git commit -m 'add files with tabs and newlines'
 "
 
-# Determine rm behavior
-# Later we will try removing an unremovable path to make sure
-# git rm barfs, but if the test is run as root that cannot be
-# arranged.
-: >test-file
-chmod a-w .
-rm -f test-file 2>/dev/null
-if test -f test-file
-then
-	test_set_prereq RO_DIR
-else
-	skip_all='skipping removal failure test (perhaps running as root?)'
-fi
-chmod 775 .
-rm -f test-file
-
 test_expect_success \
     'Pre-check that foo exists and is in index before git rm foo' \
     '[ -f foo ] && git ls-files --error-unmatch foo'
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 7d7140d..a0f3e7c 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,7 +179,7 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
-test_expect_success POSIXPERM 'git add should fail atomically upon an unreadable file' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'git add should fail atomically upon an unreadable file' '
 	git reset --hard &&
 	date >foo1 &&
 	date >foo2 &&
@@ -190,7 +190,7 @@ test_expect_success POSIXPERM 'git add should fail atomically upon an unreadable
 
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add --ignore-errors' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'git add --ignore-errors' '
 	git reset --hard &&
 	date >foo1 &&
 	date >foo2 &&
@@ -201,7 +201,7 @@ test_expect_success POSIXPERM 'git add --ignore-errors' '
 
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add (add.ignore-errors)' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'git add (add.ignore-errors)' '
 	git config add.ignore-errors 1 &&
 	git reset --hard &&
 	date >foo1 &&
@@ -212,7 +212,7 @@ test_expect_success POSIXPERM 'git add (add.ignore-errors)' '
 '
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add (add.ignore-errors = false)' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'git add (add.ignore-errors = false)' '
 	git config add.ignore-errors 0 &&
 	git reset --hard &&
 	date >foo1 &&
@@ -223,7 +223,7 @@ test_expect_success POSIXPERM 'git add (add.ignore-errors = false)' '
 '
 rm -f foo2
 
-test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 '--no-ignore-errors overrides config' '
        git config add.ignore-errors 1 &&
        git reset --hard &&
        date >foo1 &&
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 7d8ed68..5533973 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -388,16 +388,15 @@ test_expect_success 'core.excludesfile' '
 
 '
 
-test_expect_success 'removal failure' '
+test_expect_success CHMOD_0000 'removal failure' '
 
 	mkdir foo &&
 	touch foo/bar &&
 	(exec <foo/bar &&
 	 chmod 0 foo &&
-	 test_must_fail git clean -f -d)
-
+	 test_must_fail git clean -f -d &&
+	 chmod 755 foo)
 '
-chmod 755 foo
 
 test_expect_success 'nested git work tree' '
 	rm -fr foo bar &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index a72fe3a..4f85db5 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -793,7 +793,7 @@ test_expect_success 'commit --dry-run submodule summary (--amend)' '
 	test_cmp expect output
 '
 
-test_expect_success POSIXPERM 'status succeeds in a read-only repository' '
+test_expect_success POSIXPERM_AND_CHMOD_0000 'status succeeds in a read-only repository' '
 	(
 		chmod a-w .git &&
 		# make dir1/tracked stat-dirty
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..41052fb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -886,3 +886,17 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON
 # test whether the filesystem supports symbolic links
 ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
 rm -f y
+
+# test whether we can make read-only files
+mkdir hla
+chmod -w hla
+touch hla/gh >/dev/null 2>&1
+if ! test -f hla/gh
+then
+    test_set_prereq CHMOD_0000
+    # A hack around not being able to supply more than one
+    # prerequisite in the test_* functions.
+    test_have_prereq POSIXPERM && test_set_prereq POSIXPERM_AND_CHMOD_0000
+    test_have_prereq SYMLINKS  && test_set_prereq SYMLINKS_AND_CHMOD_0000
+fi
+rm -rf hla
-- 
1.7.1

Re: [PATCH] tests: A new test prereq for testing chmod -w as root

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Some tests depend on not being able to read files after chmod -w. This
> doesn't work when running the tests as root.

Obviously you meant s/read/write/ or "chmod -r" ;-)

We discussed this prerequisite in the past as "SANITY", in the dual sense
that (1) nobody sane should be running tests as root and (2) for root many
normal assumptions programs make do not hold.  If we throw out the former
by saying that it is safe to run tests under fakeroot, we would need
something like this patch to cover the latter.  The patch is a step in the
right direction.

Having said that.

I wonder if we want to be so specific, as your patch does, to single out
"you can write even to a-w file" aspect of rootness, or just want to cover
the rootness more broadly so that other rooty conditions like "if you can
read even an a-r file, then the assumptions the test makes will not hold"
and "if you can kill other's processes, ...ditto..." can also be covered
with a single prerequisite token.

Also I think there was a discussion and proposed patch to support more
than one prerequisite tokens, concatenated with "," or something, like:

    test_expect_success POSIXPERM,SANITY 'notice unwritable repo' '
        ... test that depends on posixperm and not running
        ... as root comes here
    '

so that you don't have to invent permutations of prerequisite tokens.

Thanks.

Re: [PATCH] tests: A new test prereq for testing chmod -w as root

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 4, 2010 at 19:13, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Some tests depend on not being able to read files after chmod -w. This
>> doesn't work when running the tests as root.
>
> Obviously you meant s/read/write/ or "chmod -r" ;-)

Yeah. I'll fix that in a future submission. Pending the question of
whether there's any interest.

> We discussed this prerequisite in the past as "SANITY", in the dual sense
> that (1) nobody sane should be running tests as root and (2) for root many
> normal assumptions programs make do not hold.  If we throw out the former
> by saying that it is safe to run tests under fakeroot, we would need
> something like this patch to cover the latter.  The patch is a step in the
> right direction.

I sometimes run stuff like this as root, although obviously not often
enough to have bumped into this before with Git. But sometimes you
don't care about user seperation, e.g. inside a virtual machine.

When I bumped into this I was writing a script to run in cron that
would build and install pu daily in /usr/local if tests passed. Since
I needed root to install there I was running the whole script as root,
it was easier than giving the script permission to sudo or granularly
adjusting the permissions of /usr/local.

In any case, seemingly randomly breaking if the tests are run as root
is a bad thing which should be fixed.

> I wonder if we want to be so specific, as your patch does, to single out
> "you can write even to a-w file" aspect of rootness, or just want to cover
> the rootness more broadly so that other rooty conditions like "if you can
> read even an a-r file, then the assumptions the test makes will not hold"
> and "if you can kill other's processes, ...ditto..." can also be covered
> with a single prerequisite token.

Well, in practice it was the one and only thing that broke under
root. And it's something that can be compartmentalized easily and
tested for without cheating and checking if UID == 0.

> Also I think there was a discussion and proposed patch to support more
> than one prerequisite tokens, concatenated with "," or something, like:
>
>    test_expect_success POSIXPERM,SANITY 'notice unwritable repo' '
>        ... test that depends on posixperm and not running
>        ... as root comes here
>    '
>
> so that you don't have to invent permutations of prerequisite tokens.

I almost implemented something like that when rolling this series. Do
you happen to have the Message-ID of the thread or some pertinent
search keywords? Maybe I'll resurrect it. We're using that sort of
thing in a couple of places.

I didn't implement it because I was worried about handling the 'FOO &&
BAR' and 'FOO || BAR' cases, and combinations thereof. That's probably
overthinking it though, AND-ing them together seems to be good enough,
and simple to do.

Re: [PATCH] tests: A new test prereq for testing chmod -w as root

[Jonathan Nieder]

Hi Ævar,

Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>> Some tests depend on not being able to read files after chmod -w. This
>> doesn't work when running the tests as root.
>
> Obviously you meant s/read/write/ or "chmod -r" ;-)
> 
> We discussed this prerequisite in the past as "SANITY"
[...]
> Also I think there was a discussion and proposed patch to support more
> than one prerequisite tokens, concatenated with "," or something

Thanks for picking up this topic.  Here’s some related work:

 - Incomplete series using space-separated test prerequisites:
   http://thread.gmane.org/gmane.comp.version-control.git/145427

 - Why that series is incomplete:
   http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=104;bug=579273

 - Comma-separated test prerequisites:
   http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118434

 - Earlier.
   http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385

>     test_expect_success POSIXPERM,SANITY

Last time I looked, IFS=, as used by Hannes looked like a sane
approach.  I noticed too late and did not follow up; sorry about that.

[PATCH v2] tests: A SANITY test prereq for testing if we're root

[Ævar Arnfjörð Bjarmason]

Some tests depend on not being able to write to files after chmod
-w. This doesn't work when running the tests as root.

Change test-lib.sh to test if this works, and if so it sets a new
SANITY test prerequisite. The tests that use this previously failed
when run under root.

There was already a test for this in t3600-rm.sh, added by Junio C
Hamano in 2283645 in 2006. That check now uses the new SANITY
prerequisite.

Some of this was resurrected from the "Tests in Cygwin" thread in May
2009:

    http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Here's a buch improved v2 of this patch. It goes on top of the
just-submitted "tests: Better prerequisite handling & documentation"
series (<1281129565-26124-1-git-send-email-avarab@gmail.com>).

It uses the SANITY prerequisite suggested by Junio in 2009, and uses a
simpler `test -w /' test to check if we're the superuser.

 t/README                    |    5 +++++
 t/t0001-init.sh             |    2 +-
 t/t0004-unwritable.sh       |    8 ++++----
 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/t3600-rm.sh               |   16 ----------------
 t/t3700-add.sh              |   10 +++++-----
 t/t7300-clean.sh            |    7 +++----
 t/t7508-status.sh           |    2 +-
 t/test-lib.sh               |    4 ++++
 9 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/t/README b/t/README
index dc07939..28effb4 100644
--- a/t/README
+++ b/t/README
@@ -523,6 +523,11 @@ use these, and "test_set_prereq" for how to define your own.
    The filesystem we're on supports symbolic links. E.g. a FAT
    filesystem doesn't support these. See 704a3143 for details.
 
+ - SANITY
+
+   Test is not run by root user, and an attempt to write to an
+   unwritable file is expected to fail correctly.
+
 Tips for Writing Tests
 ----------------------
 
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 7c0a698..7a75999 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -301,7 +301,7 @@ test_expect_success 'init notices EEXIST (2)' '
 	)
 '
 
-test_expect_success POSIXPERM 'init notices EPERM' '
+test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
 	rm -fr newdir &&
 	(
 		mkdir newdir &&
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 2342ac5..385b126 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -15,7 +15,7 @@ test_expect_success setup '
 
 '
 
-test_expect_success POSIXPERM 'write-tree should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' '
 
 	(
 		chmod a-w .git/objects .git/objects/?? &&
@@ -27,7 +27,7 @@ test_expect_success POSIXPERM 'write-tree should notice unwritable repository' '
 
 '
 
-test_expect_success POSIXPERM 'commit should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '
 
 	(
 		chmod a-w .git/objects .git/objects/?? &&
@@ -39,7 +39,7 @@ test_expect_success POSIXPERM 'commit should notice unwritable repository' '
 
 '
 
-test_expect_success POSIXPERM 'update-index should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' '
 
 	(
 		echo 6O >file &&
@@ -52,7 +52,7 @@ test_expect_success POSIXPERM 'update-index should notice unwritable repository'
 
 '
 
-test_expect_success POSIXPERM 'add should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '
 
 	(
 		echo b >file &&
diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index f19b4a2..eb8e3d4 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '
 
 '
 
-test_expect_success SYMLINKS 'funny symlink in work tree, un-unlink-able' '
+test_expect_success SYMLINKS,SANITY 'funny symlink in work tree, un-unlink-able' '
 
 	rm -fr a b &&
 	git reset --hard &&
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b514cbb..b26cabd 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -28,22 +28,6 @@ embedded' &&
      git commit -m 'add files with tabs and newlines'
 "
 
-# Determine rm behavior
-# Later we will try removing an unremovable path to make sure
-# git rm barfs, but if the test is run as root that cannot be
-# arranged.
-: >test-file
-chmod a-w .
-rm -f test-file 2>/dev/null
-if test -f test-file
-then
-	test_set_prereq RO_DIR
-else
-	skip_all='skipping removal failure test (perhaps running as root?)'
-fi
-chmod 775 .
-rm -f test-file
-
 test_expect_success \
     'Pre-check that foo exists and is in index before git rm foo' \
     '[ -f foo ] && git ls-files --error-unmatch foo'
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 7d7140d..ec71083 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,7 +179,7 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
-test_expect_success POSIXPERM 'git add should fail atomically upon an unreadable file' '
+test_expect_success POSIXPERM,SANITY 'git add should fail atomically upon an unreadable file' '
 	git reset --hard &&
 	date >foo1 &&
 	date >foo2 &&
@@ -190,7 +190,7 @@ test_expect_success POSIXPERM 'git add should fail atomically upon an unreadable
 
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add --ignore-errors' '
+test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '
 	git reset --hard &&
 	date >foo1 &&
 	date >foo2 &&
@@ -201,7 +201,7 @@ test_expect_success POSIXPERM 'git add --ignore-errors' '
 
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add (add.ignore-errors)' '
+test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors)' '
 	git config add.ignore-errors 1 &&
 	git reset --hard &&
 	date >foo1 &&
@@ -212,7 +212,7 @@ test_expect_success POSIXPERM 'git add (add.ignore-errors)' '
 '
 rm -f foo2
 
-test_expect_success POSIXPERM 'git add (add.ignore-errors = false)' '
+test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors = false)' '
 	git config add.ignore-errors 0 &&
 	git reset --hard &&
 	date >foo1 &&
@@ -223,7 +223,7 @@ test_expect_success POSIXPERM 'git add (add.ignore-errors = false)' '
 '
 rm -f foo2
 
-test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
+test_expect_success POSIXPERM,SANITY '--no-ignore-errors overrides config' '
        git config add.ignore-errors 1 &&
        git reset --hard &&
        date >foo1 &&
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 7d8ed68..fe8abf6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -388,16 +388,15 @@ test_expect_success 'core.excludesfile' '
 
 '
 
-test_expect_success 'removal failure' '
+test_expect_success SANITY 'removal failure' '
 
 	mkdir foo &&
 	touch foo/bar &&
 	(exec <foo/bar &&
 	 chmod 0 foo &&
-	 test_must_fail git clean -f -d)
-
+	 test_must_fail git clean -f -d &&
+	 chmod 755 foo)
 '
-chmod 755 foo
 
 test_expect_success 'nested git work tree' '
 	rm -fr foo bar &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index a72fe3a..ee0e573 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -793,7 +793,7 @@ test_expect_success 'commit --dry-run submodule summary (--amend)' '
 	test_cmp expect output
 '
 
-test_expect_success POSIXPERM 'status succeeds in a read-only repository' '
+test_expect_success POSIXPERM,SANITY 'status succeeds in a read-only repository' '
 	(
 		chmod a-w .git &&
 		# make dir1/tracked stat-dirty
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4e0a1c3..ace67a7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -894,3 +894,7 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON
 # test whether the filesystem supports symbolic links
 ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
 rm -f y
+
+# When the tests are run as root, permission tests will report that
+# things are writable when they shouldn't be.
+test -w / || test_set_prereq SANITY
-- 
1.7.1

Re: [PATCH v2] tests: A SANITY test prereq for testing if we're root

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> + - SANITY
> +
> +   Test is not run by root user, and an attempt to write to an
> +   unwritable file is expected to fail correctly.

As I said in the previous round, I am indeed in favor of having a single
"running as root---code that expects that the normal UNIXy permission
based protection to apply, aka 'running in sane environment', will not
work correctly" prerequisite token, rather than having separate "can I
expect an unwritable file to be unwritable?"  "can I expect an unreadble
file to be unreadable?" bits.  The name of the token _might_ be subject to
debate (I am fine with either SANITY or NOROOT), but the explanation
should mention this is defined to be a bit more broad than "unWRITABLE", I
think.  "test -w /" is a traditional way to approximately check if you are
running as root (technically, it only checks if you are running with
unduly high privilege---your sysadm _could_ have done "chmod 2775 /" and
made it owned by the admin group).

But that is just a nitpick on the wording we could fix if necessary.

Thanks.

Re: [PATCH v2] tests: A SANITY test prereq for testing if we're root

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 9, 2010 at 16:55, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> + - SANITY
>> +
>> +   Test is not run by root user, and an attempt to write to an
>> +   unwritable file is expected to fail correctly.
>
> As I said in the previous round, I am indeed in favor of having a single
> "running as root---code that expects that the normal UNIXy permission
> based protection to apply, aka 'running in sane environment', will not
> work correctly" prerequisite token, rather than having separate "can I
> expect an unwritable file to be unwritable?"  "can I expect an unreadble
> file to be unreadable?" bits.

I probably shouldn't have used your docs as-is, you're right. It could
be explained better.

> The name of the token _might_ be subject to debate (I am fine with
> either SANITY or NOROOT), but the explanation should mention this is
> defined to be a bit more broad than "unWRITABLE", I think.

NOROOT is better I think,.

> "test -w /" is a traditional way to approximately check if you are
> running as root (technically, it only checks if you are running with
> unduly high privilege---your sysadm _could_ have done "chmod 2775 /"
> and made it owned by the admin group).

Initially I wrote it as:

     # test whether the filesystem supports symbolic links
     ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
     rm -f y
    +
    +# test whether we can make read-only files
    +mkdir hla
    +chmod -w hla
    +touch hla/gh >/dev/null 2>&1
    +test -f hla/gh || test_set_prereq SANITY
    +rm -rf hla

But then I saw your old "test -w /" implementation and figured it was
good enough without doing all this work on setup. I can submit another
patch with that fixup if you like, maybe it'll prevent odd failures on
someone's odd system.