Re: [PATCH] Support for filesystem watching (inotify)

[Eli Zaretskii]

> From: Rüdiger Sonderfeld <address@hidden>
> Date: Sat, 4 Jun 2011 00:34:15 +0200
> 
> I wrote a patch to support watching for file system events. It currently only 
> works with inotify (Linux) but it could be extended to support kqueue 
> (*BSD, OS X) or Win32. But I wanted to get some feedback first. Watching for 
> filesystem events would be very useful for, e.g., dired or magit's status 
> view.

Thanks.  A few comments below.

> +#include <setjmp.h>

Why do you need this header?

> +// Assoc list of files being watched.

We don't use C++-style comments, because we don't require C9x compiler
to build Emacs.

> +static Lisp_Object data;

The name "data" is too generic.  Use something more specific and
descriptive, like inotify_watch_alist.

> +static void 
> +inotify_callback(int fd, void *_, int for_read) {

What's that _ in the argument list?

Also, the style of braces is not according to GNU coding standards.

> +  if(ioctl(fd, FIONREAD,  &to_read) == -1)
> +    report_file_error("ioctl(2) on inotify", Qnil);

Error messages that are shown to the user should not be cryptic.
Please use some text that would make sense to a user who is not an
expert on inotify.  I would think something like this would be
appropriate:

  File watching feature (inotify) is not available

> +  char *const buffer = xmalloc(to_read);
> +  ssize_t const n = read(fd, buffer, to_read);

Can't declare locals in the middle of code: this isn't C++.

> +  if(n < 0)
> +    report_file_error("read from inotify", Qnil);

The text of this message also needs work, to make it understandable by
mere mortals.

> +  size_t i = 0;

C++/C9x again.

> +  while(i < (size_t)n)

I think this cast, in addition to being ugly, is also unneeded,
because you already verified above that n is positive.  So `i' can be
ssize_t as well, and the need for the cast is gone.

> +          call[1] = Fcar(Fcdr(callback));       /* path */

GNU coding standards frown on using "path" for anything except
PATH-style lists of directories separated by colons.  Use "file name"
instead.

> +          /* TODO check how to handle UTF-8 in file names:
> +             make_string_from_bytes NCHARS vs. NBYTES */

Not make_string_from_bytes, but DECODE_FILE.

> +          size_t const len = strlen(ev->name);

C9x again.

> +          /* if a directory is watched name contains the name
> +             of the file that was changed */

Comments should begin with a capital letter and end with a period and
2 blanks.

> +          if(ev->mask & IN_IGNORED)
> +            {
> +              /* Event was removed automatically: Drop it from data list. */
> +              message("File-watch: \"%s\" will be ignored", SSDATA(call[1]));
> +              data = Fdelete(callback, data);
> +            }

Do we really need this message?  Consider outputting it only to the
*Messages* buffer.

> +          if(ev->mask & IN_Q_OVERFLOW)
> +            message1("File watch: Inotify Queue Overflow!");

Same here.

> +  free(buffer);

xfree, not free.

> +Watching a directory is not recursive. CALLBACK receives the events as a list
> +with each list element being a list containing information about an event. 
> The
> +first element is a flag symbol. If a directory is watched the second element 
> is
> +the name of the file that changed. If a file is moved from or to the 
> directory
> +the second element is either :from or :to and the third element is the file
> +name.

Please leave two blanks after a period that ends a sentence.  Also, I
think it's best to show the forms of the non-primitive arguments to
CALLBACK, rather than trying to describe them: a picture is worth a
thousand words.  Finally, the lines are too long.

>     A fourth element contains a numeric identifier (cookie) that can be used
> +to identify matching move operations if a file is moved inside the directory.

This part is extremely unclear.  How about an example?

> +      add_read_fd(inotifyfd, &inotify_callback, &data);

I don't see any code that does the corresponding delete_read_fd.

> +  uint32_t mask = 0;

Why not `unsigned'?  uint32_t is not part of C90, so it's less
portable.

> +  int watchdesc = inotify_add_watch(inotifyfd, SSDATA(args[0]), mask);

Declaration in the middle of code.

More importantly, you cannot pass to inotify_add_watch a string in its
internal Emacs representation.  You need to run it through ENCODE_FILE
first.

> +  if(watchdesc == -1) {
> +    report_file_error("Watching file", Qnil);

Again, this error message text is not helpful.  It should also mention
the file's name.

> +  (Lisp_Object path)
> +{
> +  if(inotifyfd == uninitialized)
> +    return Qnil;
> +  CHECK_STRING(path);
> +
> +  Lisp_Object x = data;
> +  Lisp_Object cell, path_data;

`path' and `path_data' again go against the GNU coding standards wrt
using "path".

> +  while(!NILP(x))
> +    {
> +      cell = Fcar(x);
> +      x = Fcdr(x);
> +      path_data = Fcdr(cell);

??? Isn't `data' an alist?  If so, why not use Fassq etc.?

> +      if(!NILP(path_data) && STRINGP(Fcar(path_data))
> +         && Fstring_equal(Fcar(path_data), path)
> +         && NUMBERP(Fcar(cell)))
> +        {
> +          int const magicno = XINT(Fcar(cell));
> +          data = Fdelete(cell, data);
> +          if(inotify_rm_watch(inotifyfd, magicno) == -1)
> +            report_file_error("Unwatch path", Qnil);
> +          return Qt;
> +        }
> +    }

I think this should call delete_read_fd when `data' becomes empty.