[PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[Tomi Ollila]

The find option syntax `-perm +111` is deprecated gnu find feature.
The replacement `( -perm -100 -o -perm -10 -o -perm 1 )` should also
work outside of the GNU domain.
---
 test/basic | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/basic b/test/basic
index 1b2a7d2..64eb7d7 100755
--- a/test/basic
+++ b/test/basic
@@ -53,7 +53,8 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
 test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
 eval $(sed -n -e '/^TESTS="$/,/^"$/p' $TEST_DIRECTORY/notmuch-test)
 tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
-available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
+available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f \
+   '(' -perm -100 -o -perm -10 -o -perm -1 ')' \
     ! -name aggregate-results.sh	\
     ! -name arg-test			\
     ! -name hex-xcode			\
-- 
1.8.0

Re: [PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[Austin Clements]

LGTM.  Though, I wonder, why not *just* -perm -100?  That isn't quite
a correct test of whether the user can execute it: e.g., if the file
is owned by some other user and a group the current user isn't in,
then -perm -1 is the correct test, though unless the file has some
unusual permissions, -perm -100 is likely to pass anyway.  But the
test you have (and the test that was there before) isn't quite correct
either: if the file is owned by the current user and has some crazy
permission like 0611, the user won't be able to execute it, even
though someone else could.

It's too bad "-executable" is a GNU extension.

Quoth Tomi Ollila on Jun 08 at 12:37 am:
> The find option syntax `-perm +111` is deprecated gnu find feature.
> The replacement `( -perm -100 -o -perm -10 -o -perm 1 )` should also
> work outside of the GNU domain.
> ---
>  test/basic | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/test/basic b/test/basic
> index 1b2a7d2..64eb7d7 100755
> --- a/test/basic
> +++ b/test/basic
> @@ -53,7 +53,8 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
>  test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
>  eval $(sed -n -e '/^TESTS="$/,/^"$/p' $TEST_DIRECTORY/notmuch-test)
>  tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
> -available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
> +available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f \
> +   '(' -perm -100 -o -perm -10 -o -perm -1 ')' \
>      ! -name aggregate-results.sh	\
>      ! -name arg-test			\
>      ! -name hex-xcode			\

Re: [PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[Tomi Ollila]

On Mon, Jun 10 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> LGTM.  Though, I wonder, why not *just* -perm -100?  That isn't quite
> a correct test of whether the user can execute it: e.g., if the file
> is owned by some other user and a group the current user isn't in,
> then -perm -1 is the correct test, though unless the file has some
> unusual permissions, -perm -100 is likely to pass anyway.  But the
> test you have (and the test that was there before) isn't quite correct
> either: if the file is owned by the current user and has some crazy
> permission like 0611, the user won't be able to execute it, even
> though someone else could.

While giving considerable amount of thought for such an insignificant
issue I came to realize this:

The purpose of the '-perm ...' part in that expression is not to check
whether the file is executable by the user but just to reduce the set
of files the whole expression returns without need to "blacklist" more
files that are already blacklisted with '! -name ...' subexpressions
("Makefile", ".gitignore" and so on).

With +111, /ppp and their portable alternative
( -perm -100 -or -perm -10 -or -perm 1 ) the implicit reduction this
part does is smaller than with -100.

The returned list is then compared with ${TESTS} and if there is no
exact match then this particular test fails.

Whatever this test result is, the execution of any file in ${TESTS}
will fail with "permission denied" if it is not executable by
the user running the tests.

I think that as we're doing this "shortcut" instead of full file
blacklisting, this should reduce the output less rather than
more and therefore use the version provided in this patch
instead of changing +111 to -100.

(In the future I'd like to see that we had some convention to name
the test scripts and either do comparison to that list or that
convention also dictates order and this test could be removed. There
are a few alternatives that we could think of...).

> It's too bad "-executable" is a GNU extension.

I'd have uses for that :D

Tomi

> Quoth Tomi Ollila on Jun 08 at 12:37 am:
>> The find option syntax `-perm +111` is deprecated gnu find feature.
>> The replacement `( -perm -100 -o -perm -10 -o -perm 1 )` should also
>> work outside of the GNU domain.
>> ---
>>  test/basic | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/basic b/test/basic
>> index 1b2a7d2..64eb7d7 100755
>> --- a/test/basic
>> +++ b/test/basic
>> @@ -53,7 +53,8 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
>>  test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
>>  eval $(sed -n -e '/^TESTS="$/,/^"$/p' $TEST_DIRECTORY/notmuch-test)
>>  tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
>> -available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
>> +available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f \
>> +   '(' -perm -100 -o -perm -10 -o -perm -1 ')' \
>>      ! -name aggregate-results.sh    \
>>      ! -name arg-test                        \
>>      ! -name hex-xcode                       \
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[Austin Clements]

Quoth Tomi Ollila on Jun 13 at 10:47 pm:
> On Mon, Jun 10 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> > LGTM.  Though, I wonder, why not *just* -perm -100?  That isn't quite
> > a correct test of whether the user can execute it: e.g., if the file
> > is owned by some other user and a group the current user isn't in,
> > then -perm -1 is the correct test, though unless the file has some
> > unusual permissions, -perm -100 is likely to pass anyway.  But the
> > test you have (and the test that was there before) isn't quite correct
> > either: if the file is owned by the current user and has some crazy
> > permission like 0611, the user won't be able to execute it, even
> > though someone else could.
> 
> While giving considerable amount of thought for such an insignificant
> issue I came to realize this:
> 
> The purpose of the '-perm ...' part in that expression is not to check
> whether the file is executable by the user but just to reduce the set
> of files the whole expression returns without need to "blacklist" more
> files that are already blacklisted with '! -name ...' subexpressions
> ("Makefile", ".gitignore" and so on).
> 
> With +111, /ppp and their portable alternative
> ( -perm -100 -or -perm -10 -or -perm 1 ) the implicit reduction this
> part does is smaller than with -100.
> 
> The returned list is then compared with ${TESTS} and if there is no
> exact match then this particular test fails.
> 
> Whatever this test result is, the execution of any file in ${TESTS}
> will fail with "permission denied" if it is not executable by
> the user running the tests.
> 
> I think that as we're doing this "shortcut" instead of full file
> blacklisting, this should reduce the output less rather than
> more and therefore use the version provided in this patch
> instead of changing +111 to -100.
> 
> (In the future I'd like to see that we had some convention to name
> the test scripts and either do comparison to that list or that
> convention also dictates order and this test could be removed. There
> are a few alternatives that we could think of...).

Okay.

(I completely agree that the right solution here is switching to a
naming convention and eliminating the hand-made list of tests.)

Re: [PATCH 1/1] test/basic: replaced find -perm +111 with portable alternative

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> The find option syntax `-perm +111` is deprecated gnu find feature.
> The replacement `( -perm -100 -o -perm -10 -o -perm 1 )` should also
> work outside of the GNU domain.

pushed, 

d