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

[Thien-Thi Nguyen]

() Rüdiger Sonderfeld <address@hidden>
() Sat, 4 Jun 2011 19:13:08 +0200

   +DEFUN ("file-watch", Ffile_watch, Sfile_watch, 3, MANY, 0,
   +       doc: /* Watch a file or directory.
   +
   +file-watch watches the file or directory given in FILENAME.  If a change 
occurs
   +CALLBACK is called with FILENAME as first argument and a list of changes as 
second
   +argument.  FLAGS can be
   +
   +:modify -- notify when a file is modified or created.
   +
   +:move -- notify when a file/directory is moved.
   +
   +:attrib -- notify when attributes change.
   +
   +:delete -- notify when a file/directory is deleted.
   +
   +:all -- notify for all of the above.

I think FLAGS should be symbols, not keywords.  Furthermore, "flags"
is too generic; maybe something like "aspect" or even "what" would be
(slightly) better.  Instead of ‘:all’ (or ‘all’), consider using ‘t’.

Also, it's cool if the first line names all the args.  How about:
"Arrange to call FUNC if ASPECT of FILENAME changes."
?

   +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.  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.
   +
   +Example:
   +(file-watch "foo" #'(lambda (file event) (message "%s %s" file event)) :all)
   +
   +Use `file-unwatch' to stop watching.
   +
   +usage: (file-watch FILENAME CALLBACK &rest FLAGS) */)
   +  (size_t nargs, Lisp_Object *args)

For upward-compatability, it is better to not use rest-args for these
aspects.  You should specify one arg ASPECT, document that it can be
"either a symbol, one of: ..., or a list of of these symbols".

Style preference: I'd prefer CALLBACK to be last so that long lambda
expressions need not be (visually) scanned to determine the aspects watched.
Consider:

(file-watch "foo" t (lambda (name event)
                      ;; LONG
                      ;; COMPLICATED
                      ;; DRAWN-OUT
                      ;; COMPUTATION
                      ))

vs

(file-watch "foo" (lambda (name event)
                    ;; LONG
                    ;; COMPLICATED
                    ;; DRAWN-OUT
                    ;; COMPUTATION
                    )
            t)

No big deal, just sympathy for the lone t.

   +  CHECK_STRING(args[0]);

Please insert a space between a function (or macro) and its arg list.
Generally, (info "(standards) Formatting").

   +      else /* TODO: should this be an error? */

Yes.

   +  /* TODO: check if file is already in the watch_list.  */

Moreover, you need to decide what to do given, for example:
 (progn (file-watch "foo" 'modify 'notify-modify)
        (file-watch "foo" 'move 'notify-move))
Is this OK, is this an error, is this "close to" an error?
I think a simple "file is already in" check is insufficient.

   +  watch_list = Fcons(Fcons(make_number(watchdesc), Flist(2, args)), 
watch_list);

You can use ‘acons’: (acons K V ALIST) ≡ (cons (cons K V) ALIST).