Re: [PATCH] Added basic file system watching support.

[Stefan Monnier]

>> I'm not very familiar with inotify and other file-system watching
>> facilities, so just as a guideline for others's reviews: I'm OK with
>> installing in 24.3 a non-complete version of the code, and I'm OK to
>> install before the freeze a code that needs more testing, *but* only if
>> those missing functionalities and testing won't require a redesign of
>> the API.
> It could require a minor redesign of the API.

I know of "minor changes" (where backward compatibility is easy to
preserve) and "redesigns" but I don't know what a "minor redesign" would
look like.

> At least I want to gather some  experience using the API first.
> And maybe porting it to other systems will  require API adjustments.
> Both BSD's and Windows' filesystem watch APIs are  not as powerful
> as inotify.

[ I'm probably rehashing something already discussed at the time.  ]
If the API is sufficiently general that it can be reused for other
systems, that's great.

If there's a good chance this won't work without breaking compatibility,
maybe a better option is to provide a low-level API that maps very
closely to inotify and then an Elisp layer on top which abstracts away
differences between different systems.  In that case we can install the
inotify support right away while we're still experimenting with the
higher-level abstraction.

>> I understand that NAME refers to the added/removed file, but I'm
>> wondering why we don't also add FILENAME (the name of the directory) to
>> the event.  If inotify doesn't provide it, we can provide it
>> ourselves, right?
>> Or is it rarely/never needed?
>> Or is it because that directory may change name without us knowing?
> The name might change.

OK, makes sense.

>> I can't remember enough of the context, but objects that are "unique"
>> are plentiful.  A cons cell would do, regardless of its value, for
>> example (now, as to whether it's better than a number, that depends on
>> what it's use for, e.g. whether you can put the cons's fields to good
>> use).
> Currently the only risk is that cookie is bigger than
> MOST_POSITIVE_FIXNUM and  then gets mapped to a value that is already
> taken by another cookie.  This is only a problem on 32bit systems
> without the use of wide-int because cookie is uint32_t.

Ah, I remember what this is now.  Rather than `cookie', I'd rather name
it `identifier' or `identity'.  So IIUC Emacs's code doesn't never needs
to pass this cookie back to the inotify code, right?  So the only issue
with the current "make_number (ev->cookie)" is that it might incorrectly
lead to matching two events that are really unrelated, in case they have
the same lower 30bits.  I don't know how important those last 2bits are
in practice.  But if they're unlikely to be important in practice, then
I guess the current solution might be acceptable.

OTOH we should stipulate that COOKIES should be compared with `equal',
not `eq', so we could later use something like Fcons (make_number
(ev->cookie / 65536), make_number (ev->cookie % 65536)).

> There is a similar problem with the watch descriptor.

This is slightly different in that here, the descriptor is later passed
back to inotify, so it's indispensable that it be preserved correctly.
Also, descriptors aren't usually compared for equality: it might be OK
to have two watch-descriptors that refer to the same watch object but
which aren't `eq'.

The docstring of file-watch doesn't really make it clear what file-watch
returns (it only says it indirectly with "The ID argument is the id returned by
file-watch") so that should be fixed.
Also this ID should be called WD for "watch descriptor", WH for "watch
handle", or FW for "file-watcher".

> Inotify uses an incremental system for watch descriptors and should
> be run out of descriptors by the time MOST_POSITIVE_FIXNUM is
> reached.  But there is no guarantee for it.  Therefore something like
> SAVE_VALUE should probably be used.

If watch descriptors are documented to be "small integers", like file
descriptors, it might be OK to use fixnums, but it's a bit ugly.

I think the cleaner option is to define a new object type for it.
It could be either a new Lisp_Misc type, so you can make them print as
something like "#<file-watcher NNN>" (take a look at "enum
Lisp_Misc_Type" and "union Lisp_Misc" in src/lisp.h for starters; adding
a new type will require adding corresponding branches to the switch
statements in alloc.c and in print.c).


        Stefan