Patch to add file monitoring

[Gero Treuner]

Hi all,

I felt that it would be nice when mutt instantly notifies about new
mail. Think about mails like "There is on piece of cake left!".

Monitoring files is kind of "state of the art" nowadays, but
unfortunately not portable. So my goal was to limit interference with
existing functionality, and only give a trigger when a relevant file
changed, followed by the established logic for checking mailboxes doing
exactly the same as before.
As I am using linux, "my" mechanism is inotify,

This approach could be implemented pretty good by replacing getch() by
poll() as central point of waiting for input. Although at the heart of
mutt, it is less invasive as it sounds.
The implementation in (new) monitor.c is very specific to inotify, but
was coded having addition of more interfaces in mind, structured that
for top-level functions switches/ifdefs pointing other to stuff can be
done.


Using

When the attached patch is applied, configure checks whether inotify is
available for compilation (supported by glibc on Linux) and if possible
uses it (currently no extra --enable option).

The CONTEXT and BUFFY folders are equipped with monitors. Right now
the lifetime is limited to files existing on start of mutt. Creating
specified mailboxes later is not recognized, and replacing files ends
with monitors for these removed.

Failures in the inotify area are hidden to the user. With -d2 they
appear in the debugfile, -d3 adds regular events and -d5 the bits from
the events.


Supported mbox types

All local: mbox, mmdf (not tested by me), maildir, mh

For maildir, currently only the new directory is checked: When old
mails are copied from another folder, these will be recognized only
later according to Timeout/user input. (cur directories can easily be
added, but would double the number of watches.)

For NFS all this is not really useful in most cases. It shouldn't break,
but AFAIK there is no way to get change events from remote machines.


Tested

- Mailbox types: see above
- General compilation: on Linux with/without **1 inotify
- Sidebar compilation: breaks with same message in original and patched
source with my configuration/system (current git master HEAD).

**1 I tested by intentionally braking a configure for a required method.


Technical notes

Maildir:
According the specification the expected event is IN_MOVED_TO . This
works when Exim delivers the mail, however when mutt copies messages
around the only usable event is IN_ATTRIB .

MH:
Mutt replaces file .mh_sequences, so this is the only case where a
monitor will be "refreshed".

Identify files:
Comparing st_dev and st_ino should be sufficient, correct? Why does
compare_stat() (which I couldn't use here anyway) care about st_rdev?

#ifdef:
More of these ;-) I consider it not a bad thing, because on the event
that mutt's architecture is modernized it helps to understand
dependencies.


So, have fun!


Kind regards,
Gero

[Gero Treuner]

Hi,

Post by Gero Treuner
Tested
- Mailbox types: see above
- General compilation: on Linux with/without **1 inotify
- Sidebar compilation: breaks with same message in original and patched
source with my configuration/system (current git master HEAD).

Regarding sidebar compatibility:
Compiling ("touch OPS" helped) and using it (first time for me) works
without problems. IMO that is the option where I expected most
interference,

Attached is a 2nd patch with cosmetic cleanups.


Gero

[Kevin J. McCarthy]

Post by Gero Treuner
I felt that it would be nice when mutt instantly notifies about new
mail. Think about mails like "There is on piece of cake left!".

Hi Gero,

Thank you for sending the patches. I won't have time to look more
closely for a few days, but I'll try to give feedback this week.

-Kevin

[Kevin J. McCarthy]

Post by Gero Treuner
I felt that it would be nice when mutt instantly notifies about new
mail. Think about mails like "There is on piece of cake left!".

Hi Gero,

Sorry it took a bit longer than I expected to get back to your review.

I have taken a _quick_ look at the patch. Overall I like the idea and
(with some fixes) think this would be worth including. I'll take a more
thorough look when I have a bit more time.

A few comments:

* I greatly appreciate you using a minimally invasive approach. It's
tempting to want to rewrite things, but that makes my review job much
harder.

* Some of the other callers of mutt_getch() may be adversely affected by
the patch. e.g., _mutt_enter_fname() may now abort the prompt if a
new mail notification happens at the wrong time. Hmmm... it looks
like the other callers are okay because they now handle SIGWINCH
redraws, but I'll want to take a second look to make sure all the
callers properly handle a timeout retval.

* I need to test this myself, but I'm assuming the poll() in
mutt_monitor_poll() will just return 0 when the timeout() set in
km_dokey() fires?

* Why not add and remove monitors in the same locations that
mutt_sb_notify_mailbox() is called inside mutt_parse_mailboxes()? I
think that makes more sense than where you are doing it now.

* I'd rather not have private data, e.g. INotifyFd, Monitor, exposed in
monitor.h, unless there is a good reason.

* I think we should have some kind of configure.ac option to "opt out"
of the inotify, even if your system has it. There may be a reason
someone doesn't want to use it.

I need to release 1.9.5 this week, and would like to release 1.10.0 the
beginning of next month. It's a bit late in the cycle to include this
for 1.10, so how about if we work toward getting it committed after
1.10.0 is released?

[Gero Treuner]

Hi Kevin and all,

Post by Kevin J. McCarthy
* I greatly appreciate you using a minimally invasive approach. It's
tempting to want to rewrite things, but that makes my review job much
harder.

Your feedback is welcome, also for improved style, better ordering,
enhancements etc. Thank you very much for the review and your work on
Mutt.

Post by Kevin J. McCarthy
* Some of the other callers of mutt_getch() may be adversely affected by
the patch. e.g., _mutt_enter_fname() may now abort the prompt if a
new mail notification happens at the wrong time. Hmmm... it looks
like the other callers are okay because they now handle SIGWINCH
redraws, but I'll want to take a second look to make sure all the
callers properly handle a timeout retval.

File prompts quickly advance to mutt_get_field(), so
for mutt_enter_fname() it is only about the moment when it is entered. I
don't know exactly which events might come here, for what the return is
there. Maybe SIGINT or similar?
If this is a real case, a new code for event_t.ch can be introduced to
distinguish the situation, or a way to silence the monitoring events.

Post by Kevin J. McCarthy
* I need to test this myself, but I'm assuming the poll() in
mutt_monitor_poll() will just return 0 when the timeout() set in
km_dokey() fires?

yes

Post by Kevin J. McCarthy
* Why not add and remove monitors in the same locations that
mutt_sb_notify_mailbox() is called inside mutt_parse_mailboxes()? I
think that makes more sense than where you are doing it now.

I don't remember - maybe I didn't fully trust inotify handles to be
really stable and chose a place where things can potentially be easier
fixed. I changed it. And inotify is really stable even on my Linux 3.2
machine.

Post by Kevin J. McCarthy
* I'd rather not have private data, e.g. INotifyFd, Monitor, exposed in
monitor.h, unless there is a good reason.

changed

Post by Kevin J. McCarthy
* I think we should have some kind of configure.ac option to "opt out"
of the inotify, even if your system has it. There may be a reason
someone doesn't want to use it.

added

With --disable-filemonitor I used a more general wording, anticipating
that each platform has its specific technique and support for more will
come sooner or later.

Post by Kevin J. McCarthy
I need to release 1.9.5 this week, and would like to release 1.10.0 the
beginning of next month. It's a bit late in the cycle to include this
for 1.10, so how about if we work toward getting it committed after
1.10.0 is released?

Perfect. The new patch against that version (gitlab current) is
attached. Maybe I additionally find the button to create a merge
request in Gitlab...


Gero