Re: [PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Mattias Andrée]

On Wed, 13 Apr 2016 16:16:54 +0100
Stephane Chazelas  wrote:

> 2016-04-07 10:34:56 +0200, Mattias Andrée:
> []
> > +  if (!dir)
> > +{
> > +  error (0, errno, "%s", dirname);
> > +  test_exit (TEST_FAILURE);
> > +}  
> 
> Note that it means it makes it the first operator that
> would actually cause "test" to output something.
> 
> A test for non-empty without diagnostic output, like for
> [ -s ] may be preferable.

-s is of course already in use, but what is wrong with

  2> /dev/null

> 
> For most of the current operators, test returns true if
> the test can be performed and the test succeeds and
> returns false otherwise without an error message.
> 
> For instance, [ -e /secret/file ] would return false if
> "file" exists but you don't have search access to
> "/secret" and you don't get an error.

I don't believe there is anything wrong with that, but
of you want to be pedantic you should fail if you get EIO.
-s may also want to return success if you get EOVERFLOW.

> 
> 
> > +
> > +  while (errno = 0, (de = readdir (dir)))  
> 
> That also means that would be the first operator that has
> a side effect: update the access time of the directory.

Does that matter? If it does, we could use fdopendir instead
and open it with O_NOATIME. But that would make the solution
both more complex and less portable.

> 
> Note that GNU find has a "-empty" predicate.
> 
> Zsh has a "F" (for "full") glob qualifier. *(F) expands
> to the non-empty non-hidden directories in the current
> directory.
> 
> [ -F dir ], (for test for non-empty) could be an
> alternative. It doesn't seem to be taken by any of the
> "test" implementation I've checked.
> 

Sure, but -E may feel more natural, and we hardly need
both. We are much more familiar with thinking of an
empty directory as a separate file type. Additionally,
calling something, that contains at least one item is,
“full” is a misnomer.


pgp2uetPLYPWO.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Stephane Chazelas]

2016-04-07 10:34:56 +0200, Mattias Andrée:
[]
> +  if (!dir)
> +{
> +  error (0, errno, "%s", dirname);
> +  test_exit (TEST_FAILURE);
> +}

Note that it means it makes it the first operator that would
actually cause "test" to output something.

A test for non-empty without diagnostic output, like for [ -s ]
may be preferable.

For most of the current operators, test returns true if the test
can be performed and the test succeeds and returns false
otherwise without an error message.

For instance, [ -e /secret/file ] would return false if "file"
exists but you don't have search access to "/secret" and you
don't get an error.


> +
> +  while (errno = 0, (de = readdir (dir)))

That also means that would be the first operator that has a side
effect: update the access time of the directory.

Note that GNU find has a "-empty" predicate.

Zsh has a "F" (for "full") glob qualifier. *(F) expands to the
non-empty non-hidden directories in the current directory.

[ -F dir ], (for test for non-empty) could be an alternative. It
doesn't seem to be taken by any of the "test" implementation
I've checked.

-- 
Stephane

Re: [PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Mattias Andrée]

On Thu, 7 Apr 2016 11:47:36 +0200
Bernhard Voelker  wrote:

> On 04/07/2016 10:34 AM, Mattias Andrée wrote:
> > diff --git a/src/test.c b/src/test.c
> > index 8ac7467..eb9c43a 100644
> > --- a/src/test.c
> > +++ b/src/test.c
> > @@ -27,6 +27,7 @@
> >  #endif
> >  
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -179,6 +180,39 @@ get_mtime (char const *filename,
> > struct timespec *mtime) return ok;
> >  }
> >  
> > +/* Return true iff DIR is empty. DIR must be a
> > directory.  */ +static bool
> > +empty_p (char const *dirname)
> > +{
> > +  DIR *dir;
> > +  struct dirent *de;
> > +
> > +  dir = opendir (dirname);
> > +  if (!dir)
> > +{
> > +  error (0, errno, "%s", dirname);
> > +  test_exit (TEST_FAILURE);
> > +}
> > +
> > +  while (errno = 0, (de = readdir (dir)))
> > +{
> > +  if (de->d_name[0] == '.')
> > +if (de->d_name[1 + (de->d_name[1] == '.')] ==
> > '\0')
> > +  continue;
> > +  closedir (dir);
> > +  return 0;
> > +}
> > +
> > +  if (errno)
> > +{
> > +  error (0, errno, "%s", dirname);
> > +  closedir (dir);
> > +  test_exit (TEST_FAILURE);
> > +}
> > +  closedir (dir);
> > +  return 1;
> > +}
> > +
> >  /* Return true if S is one of the test command's
> > binary operators.  */ static bool
> >  binop (char const *s)
> > @@ -457,6 +491,12 @@ unary_operator (void)
> >return (stat (argv[pos - 1], _buf) == 0
> >&& S_ISDIR (stat_buf.st_mode));
> >  
> > +case 'E':  /* File is an
> > empty directory? */
> > +  unary_advance ();
> > +  return (stat (argv[pos - 1], _buf) == 0
> > +  && S_ISDIR (stat_buf.st_mode)
> > +  && empty_p (argv[pos - 1]));
> > +  
> 
> While I find this feature quite useful - although I never
> needed such a test yet -, I see the following problems:
> 
> * While the other file type based tests only use
> stat/lstat, this one additionally needs
> opendir/readdir/closedir, i.e., the permissions on the
> directory could slip into the game:
> 
>   $ mkdir /dev/shm/dir
> 
>   $ chmod = /dev/shm/dir
> 
>   $ src/test -E /dev/shm/dir && echo OK || echo FAIL
>   src/test: /dev/shm/dir: Permission denied
>   FAIL
> 
>   $ src/test -d /dev/shm/dir && echo OK || echo FAIL
>   OK

I don't think this is a problem. For reliable
script you need to check whether the test
failed or was successful. That is, in the case
of test, you have to consider whether test
exited with value 0, 1, or >1.

> 
> * The above stat/opendir is racy:
> 
>   $ while test -d /dev/shm/dir ; do \
>   chmod = /dev/shm/dir; \
>   chmod =0700 /dev/shm/dir; \
> done &
> 
>   $ for f in $(seq 100); do \
>   src/test -E /dev/shm/dir 2>/dev/null \
> && echo OK; \
> done \
>   | grep -c OK
>   49
> 
> Avoiding stat at all, and relying on opendir may be a
> better way.

Perhaps it is better to check errno != ENOTDIR when
opendir fails, but the race itself is not important.

> 
> However, as Eric pointed out, coreutils' test and \[ are
> not in use very often, so the discussion and
> implementation should IMO start for the builtins of the
> major shells first.

Yes, the shells that implement test needs to be updated.
But until then, it can use the stand-alone by simply
running `env test` instead of `test`. You really should
be doing that anyway unless you need one of the operators
that are only implemented in the shell, the shells'
implementations are not that good.

> 
> Have a nice day,
> Berny



pgpV2PHJMju4h.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Bernhard Voelker]

On 04/07/2016 10:34 AM, Mattias Andrée wrote:
> diff --git a/src/test.c b/src/test.c
> index 8ac7467..eb9c43a 100644
> --- a/src/test.c
> +++ b/src/test.c
> @@ -27,6 +27,7 @@
>  #endif
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -179,6 +180,39 @@ get_mtime (char const *filename, struct timespec *mtime)
>return ok;
>  }
>  
> +/* Return true iff DIR is empty. DIR must be a directory.  */
> +static bool
> +empty_p (char const *dirname)
> +{
> +  DIR *dir;
> +  struct dirent *de;
> +
> +  dir = opendir (dirname);
> +  if (!dir)
> +{
> +  error (0, errno, "%s", dirname);
> +  test_exit (TEST_FAILURE);
> +}
> +
> +  while (errno = 0, (de = readdir (dir)))
> +{
> +  if (de->d_name[0] == '.')
> +if (de->d_name[1 + (de->d_name[1] == '.')] == '\0')
> +  continue;
> +  closedir (dir);
> +  return 0;
> +}
> +
> +  if (errno)
> +{
> +  error (0, errno, "%s", dirname);
> +  closedir (dir);
> +  test_exit (TEST_FAILURE);
> +}
> +  closedir (dir);
> +  return 1;
> +}
> +
>  /* Return true if S is one of the test command's binary operators.  */
>  static bool
>  binop (char const *s)
> @@ -457,6 +491,12 @@ unary_operator (void)
>return (stat (argv[pos - 1], _buf) == 0
>&& S_ISDIR (stat_buf.st_mode));
>  
> +case 'E':/* File is an empty directory? */
> +  unary_advance ();
> +  return (stat (argv[pos - 1], _buf) == 0
> +  && S_ISDIR (stat_buf.st_mode)
> +  && empty_p (argv[pos - 1]));
> +

While I find this feature quite useful - although I never needed such a
test yet -, I see the following problems:

* While the other file type based tests only use stat/lstat, this
one additionally needs opendir/readdir/closedir, i.e., the permissions
on the directory could slip into the game:

  $ mkdir /dev/shm/dir

  $ chmod = /dev/shm/dir

  $ src/test -E /dev/shm/dir && echo OK || echo FAIL
  src/test: /dev/shm/dir: Permission denied
  FAIL

  $ src/test -d /dev/shm/dir && echo OK || echo FAIL
  OK

* The above stat/opendir is racy:

  $ while test -d /dev/shm/dir ; do \
  chmod = /dev/shm/dir; \
  chmod =0700 /dev/shm/dir; \
done &

  $ for f in $(seq 100); do \
  src/test -E /dev/shm/dir 2>/dev/null \
&& echo OK; \
done \
  | grep -c OK
  49

Avoiding stat at all, and relying on opendir may be a better way.

However, as Eric pointed out, coreutils' test and \[ are not in use very
often, so the discussion and implementation should IMO start for the
builtins of the major shells first.

Have a nice day,
Berny

[PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Mattias Andrée]

---
 NEWS   |  5 +
 doc/coreutils.texi |  7 ++-
 src/test.c | 44 +++-
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index dd3ee9c..3445b17 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,11 @@ GNU coreutils NEWS-*- 
outline -*-
The 2008 edition of POSIX dropped the requirement that arguments
like '+2' must be treated as file names.
 
+** New features
+
+   test now have the -E flag which tests that a file exists and is
+   an empty directory.
+
 
 * Noteworthy changes in release 8.25 (2016-01-20) [stable]
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 45706bd..e399986 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12420,7 +12420,7 @@ Exit status:
 @end display
 
 @menu
-* File type tests:: -[bcdfhLpSt]
+* File type tests:: -[bcEdfhLpSt]
 * Access permission tests:: -[gkruwxOG]
 * File characteristic tests::   -e -s -nt -ot -ef
 * String tests::-z -n = == !=
@@ -12449,6 +12449,11 @@ True if @var{file} exists and is a block special 
device.
 @cindex character special check
 True if @var{file} exists and is a character special device.
 
+@item -E @var{file}
+@opindex -E
+@cindex empty directory check
+True if @var{file} exists, is a directory, and is empty.
+
 @item -d @var{file}
 @opindex -d
 @cindex directory check
diff --git a/src/test.c b/src/test.c
index 8ac7467..eb9c43a 100644
--- a/src/test.c
+++ b/src/test.c
@@ -27,6 +27,7 @@
 #endif
 
 #include 
+#include 
 #include 
 #include 
 
@@ -179,6 +180,39 @@ get_mtime (char const *filename, struct timespec *mtime)
   return ok;
 }
 
+/* Return true iff DIR is empty. DIR must be a directory.  */
+static bool
+empty_p (char const *dirname)
+{
+  DIR *dir;
+  struct dirent *de;
+
+  dir = opendir (dirname);
+  if (!dir)
+{
+  error (0, errno, "%s", dirname);
+  test_exit (TEST_FAILURE);
+}
+
+  while (errno = 0, (de = readdir (dir)))
+{
+  if (de->d_name[0] == '.')
+if (de->d_name[1 + (de->d_name[1] == '.')] == '\0')
+  continue;
+  closedir (dir);
+  return 0;
+}
+
+  if (errno)
+{
+  error (0, errno, "%s", dirname);
+  closedir (dir);
+  test_exit (TEST_FAILURE);
+}
+  closedir (dir);
+  return 1;
+}
+
 /* Return true if S is one of the test command's binary operators.  */
 static bool
 binop (char const *s)
@@ -457,6 +491,12 @@ unary_operator (void)
   return (stat (argv[pos - 1], _buf) == 0
   && S_ISDIR (stat_buf.st_mode));
 
+case 'E':  /* File is an empty directory? */
+  unary_advance ();
+  return (stat (argv[pos - 1], _buf) == 0
+  && S_ISDIR (stat_buf.st_mode)
+  && empty_p (argv[pos - 1]));
+
 case 's':  /* File has something in it? */
   unary_advance ();
   return (stat (argv[pos - 1], _buf) == 0
@@ -590,7 +630,8 @@ test_unop (char const *op)
 case 'f': case 'g': case 'h': case 'k': case 'n':
 case 'o': case 'p': case 'r': case 's': case 't':
 case 'u': case 'w': case 'x': case 'z':
-case 'G': case 'L': case 'O': case 'S': case 'N':
+case 'E': case 'G': case 'L': case 'O': case 'S':
+case 'N':
   return true;
 default:
   return false;
@@ -760,6 +801,7 @@ EXPRESSION is true or false and sets exit status.  It is 
one of:\n\
   -c FILE FILE exists and is character special\n\
   -d FILE FILE exists and is a directory\n\
   -e FILE FILE exists\n\
+  -E FILE FILE exists and is an empty directory\n\
 "), stdout);
   fputs (_("\
   -f FILE FILE exists and is a regular file\n\
-- 
2.8.0