Hi Kevin and all, On Wed, Apr 11, 2018 at 06:47:27PM -0700, Kevin J. McCarthy wrote: > * 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. > * 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. > * 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 > * 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. > * I'd rather not have private data, e.g. INotifyFd, Monitor, exposed in > monitor.h, unless there is a good reason. changed > * 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. > 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
mutt_inotify_1.10.0_2018-06-02.diff.gz
Description: Binary data
