Re: empty-directory predicate, native implementation

[Michael Albinus]

Arthur Miller <arthur.miller@live.com> writes:

Hi Arthur,

> Please check that handlers stuff. I am not sure how to test it, and am
> not sure I get those correct; especially tramp-crypt.

See comments below.

> I am using a regular expression from Dired+ by Drew in two places. I
> have mention it the comment in ert tests, but don't know how to mention
> it in the example in manual. Maybe remove example, or maybe it can stay
> without creds?

You don't need that regexp from Drew. We have the constant
directory-files-no-dot-files-regexp for that purpose.

> I also discovered that I wasn't covered with FIXNUM all the way; thought
> it was unsigned int so -1 would be converted into ffffffff, which
> would just yeild all results. Seems like fixnum is not what I thought so I
> have added test for <=0 which return nil.

What's wrong using FIXNATP and XFIXNAT everywhere?

> Ert test are mostly foxused on new argument. I haven't found something
> related to dired in test/src so I have created dired-tests.el. If it
> shoudl be in some other place I can rename it.

test/src/dired-tests.el is perfect. Maybe you add a comment, that just
the tests for COUNT are added.

> diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
> index 3b8b4fb3a9..2f15176e79 100644
> --- a/doc/lispref/files.texi
> +++ b/doc/lispref/files.texi
>
> +If @var{count} is non-@code{nil}, the function will return first
> +@var{count} number of files, or all files, whichever occurs
> +first. @var{count} has to be a natural number (> 0). You can use this
> +function to short circuit evaluation in case you are just interested to
> +find if a directory is empty or not (request one file and tell it to
> +ignore dot-files).

In Emacs documentation, we use two spaces after an end-of-sentence
period. Furthermore, pls say "file name" instead of "file"; this is what
we return.

> +@example
> +@group
> +  (null (directory-files directory-name nil
> +  "^\\([^.]\\|\\.\\([^.]\\|\\..\\)\\).*" t 1))

Please format properly. It shall be

(null
 (directory-files directory-name
   nil directory-files-no-dot-files-regexp t 1))

> diff --git a/etc/NEWS b/etc/NEWS
> index 1838b6b38a..25c54d3dfe 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> +** 'diirectory-files' function can now take an additional count parameter

Pls fix the spelling error. Finish the header line with a period. Use
capital letters COUNT.

> +This option makes directory-files return COUNT first files in

It is not an option, but rather an optional parameter. Quote
directory-files. Use two spaces. Use "file names". Fix "vill".

> The option is useful for checking if
> +directory is empty since it will check at most 3 files when COUNT = 1.

As it stands it is not understandable, without mentioning the respective
regexp. Better, you keep this sentence out.

> diff --git a/lisp/net/ange-ftp.el b/lisp/net/ange-ftp.el
> index 0cb8d7cb83..335a07914c 100644
> --- a/lisp/net/ange-ftp.el
> +++ b/lisp/net/ange-ftp.el
> +(defun ange-ftp-directory-files (directory &optional full match nosort
> +                                        count)

Could be in  the same line.

> @@ -3444,18 +3444,19 @@ ange-ftp-directory-files
>               (setq files
>                     (cons (if full (concat directory f) f) files))))
>         (nreverse files)))
> -    (apply 'ange-ftp-real-directory-files directory full match v19-args)))
> +    (apply 'ange-ftp-real-directory-files directory full match nosort 
> count)))

That is the fallback. But I don't see an implementation of
COUNT. (Should be in the "while tail" loop).

>  (defun ange-ftp-directory-files-and-attributes
> -  (directory &optional full match nosort id-format)
> +  (directory &optional full match nosort attrs id-format count)

What it attrs good for?

> -       (ange-ftp-directory-files directory full match nosort))
> +       (ange-ftp-directory-files directory full match nosort attrs
> +                                 id_format count))

Remove attrs. Use id-format.

>      (ange-ftp-real-directory-files-and-attributes
> -     directory full match nosort id-format)))
> +     directory full match nosort attrs id-format count)))

Remove attrs.

> --- a/lisp/net/tramp-adb.el
> +++ b/lisp/net/tramp-adb.el
> @@ -312,7 +312,7 @@ tramp-adb-handle-directory-files-and-attributes
>        (copy-tree
>         (with-tramp-file-property
>          v localname (format "directory-files-and-attributes-%s-%s-%s-%s"
> -                            full match id-format nosort)
> +                            full match id-format nosort count)

If you want to add count to the property name, you must adapt the format
string.

I miss the implementation of count.

> diff --git a/lisp/net/tramp-crypt.el b/lisp/net/tramp-crypt.el
> index 3e96daa7b1..bda3735db4 100644
> --- a/lisp/net/tramp-crypt.el
> +++ b/lisp/net/tramp-crypt.el
> +;; This function does not seem to pass match and nosort into
> +;; directory-files at all; is that intentional or bug?

ATM: yes. Leave it as it is, we could change later on. Add FIXME in the comment.

> diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
> index 3701bfc22c..787eead807 100644
> --- a/lisp/net/tramp-rclone.el
> +++ b/lisp/net/tramp-rclone.el
> +;; This function did not pass nosort arguemnt into directory-files
> +;; not sure if intentional or bug

You can remove the comment, because you did it right.

> diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
> index 15eab0a4de..7a969979ac 100644
> +;; what about perl & sta -> need to fix list to count?

That's correct. Mark the comment as FIXME.

> diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
> index 1b6af2a2e3..62135f514d 100644
> --- a/lisp/net/tramp-smb.el
> +++ b/lisp/net/tramp-smb.el
> +  (let ((result nil)
> +        (numres 0))

numres is not used. The rest I need to test later; I'm currently just
dry reading.

> diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
> index 6d44ad23ad..a99af70196 100644
> --- a/lisp/net/tramp.el
> +++ b/lisp/net/tramp.el
> +(defun tramp-handle-directory-files (directory &optional full match
> +                                               nosort count)

This fits into one line (80 chars). I'm missiong an implementation for count.

> diff --git a/src/dired.c b/src/dired.c
> index 1584b6acf0..1fc8dd27fa 100644
> --- a/src/dired.c
> +++ b/src/dired.c
> @@ -17,7 +17,6 @@
> +  if (FIXNUMP(return_count))
> +    {
> +      last = XFIXNUM (return_count);
> +      if (last <= 0)
> +        return Qnil;
> +    }

This is superfluous. Remove it.

> +If COUNT is  non-nil, the function will return max of COUNT and length
> + files, where length is number of files in the directory. COUNT has to
> + be a natural number > 0. */)

So you haven't changed the docstring?

>  DEFUN ("directory-files-and-attributes", Fdirectory_files_and_attributes,
> -       Sdirectory_files_and_attributes, 1, 5, 0,
> -       doc: /* Return a list of names of files and their attributes in 
> DIRECTORY.
> +       Sdirectory_files_and_attributes, 1, 6, 0, doc
> +       : /* Return a list of names of files and their attributes in 
> DIRECTORY.
>  Value is a list of the form:
>
> -  ((FILE1 . FILE1-ATTRS) (FILE2 . FILE2-ATTRS) ...)
> +((FILE1 . FILE1-ATTRS) (FILE2 . FILE2-ATTRS) ...)

...

Still the indentation doesn't fit with the original.

> diff --git a/test/src/dired-tests.el b/test/src/dired-tests.el
> new file mode 100644
> index 0000000000..3b739e59cc
> --- /dev/null
> +++ b/test/src/dired-tests.el
> +;; Copyright (C) 2015-2020 Free Software Foundation, Inc.

This shall be just 2020.

> +        ;; nodots expression from dired+ by Drew A.
> +        (nodots "^\\([^.]\\|\\.\\([^.]\\|\\..\\)\\).*"))

Not needed. Use directory-files-no-dot-files-regexp.

> +    (message name)

That's OK for debugging purposes. Finally, the tests shall be silent.

> +    (make-directory name)

Tests shall not kepp garbage. Please delete the directory at the end of
the test, preferred as unwindorm of unwind-protect.

> +(ert-deftest directory-files-and-attributes-tests ()

Same comments apply.

> Best regards

Best regards, Michael.