From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Michael Albinus Newsgroups: gmane.emacs.bugs Subject: bug#26126: 26.0.50; file-notify-rm-watch removes arbitrary watches Date: Sat, 25 Mar 2017 15:04:03 +0100 Message-ID: <87a889jvvw.fsf@detlef> References: <87r31x9ulw.fsf@luca> <87shmcney8.fsf@detlef> <87efxw7xvc.fsf@luca> <87mvcjophx.fsf@detlef> <87tw6rssoi.fsf@luca> <87pohfkmvh.fsf@detlef> <87lgs2sobr.fsf@luca> <87y3w2gywc.fsf@detlef> <8737e8excq.fsf@luca> <877f3el80j.fsf@luca> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1490450725 1371 195.159.176.226 (25 Mar 2017 14:05:25 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 25 Mar 2017 14:05:25 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: 26126@debbugs.gnu.org To: Andreas Politz Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Mar 25 15:05:17 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1crmJb-0007K3-3x for geb-bug-gnu-emacs@m.gmane.org; Sat, 25 Mar 2017 15:05:11 +0100 Original-Received: from localhost ([::1]:37484 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1crmJf-0008Rk-EE for geb-bug-gnu-emacs@m.gmane.org; Sat, 25 Mar 2017 10:05:15 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:59274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1crmJV-0008Py-Ij for bug-gnu-emacs@gnu.org; Sat, 25 Mar 2017 10:05:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1crmJS-0004gL-Tc for bug-gnu-emacs@gnu.org; Sat, 25 Mar 2017 10:05:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:46207) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1crmJS-0004g9-QH for bug-gnu-emacs@gnu.org; Sat, 25 Mar 2017 10:05:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1crmJS-0001qX-Hi for bug-gnu-emacs@gnu.org; Sat, 25 Mar 2017 10:05:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Michael Albinus Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 25 Mar 2017 14:05:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 26126 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 26126-submit@debbugs.gnu.org id=B26126.14904506547037 (code B ref 26126); Sat, 25 Mar 2017 14:05:02 +0000 Original-Received: (at 26126) by debbugs.gnu.org; 25 Mar 2017 14:04:14 +0000 Original-Received: from localhost ([127.0.0.1]:44406 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1crmIg-0001pR-GX for submit@debbugs.gnu.org; Sat, 25 Mar 2017 10:04:14 -0400 Original-Received: from mout.gmx.net ([212.227.15.19]:49607) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1crmIf-0001pE-9I for 26126@debbugs.gnu.org; Sat, 25 Mar 2017 10:04:13 -0400 Original-Received: from detlef.gmx.de ([87.146.63.110]) by mail.gmx.com (mrgmx001 [212.227.17.190]) with ESMTPSA (Nemesis) id 0MY7ZA-1ceSTk3qBE-00Usht; Sat, 25 Mar 2017 15:04:05 +0100 In-Reply-To: <877f3el80j.fsf@luca> (Andreas Politz's message of "Fri, 24 Mar 2017 21:44:28 +0100") X-Provags-ID: V03:K0:m9fVJudXhPh5H+bK8rENW2BRdGpZeFhqhX4VBspUjZI/G6zpP06 I3XNuWH7vHGNYS1QPxtxpWV+CUS29Lud2fRHfeqkP2HI0CLUmGMSdsCPAcuIgpaOhpZNjJk AV9G6mYU3SWIXqmt8XzKc6ZcYdA76Ld+rGiqAKNsZJwufUusoDgzMFYcsoI71jcFU6Vm/fd 9GpSUqWToJCP5xSVzi59w== X-UI-Out-Filterresults: notjunk:1;V01:K0:pRGl7Xprf6s=:Fo0/oq98ukLQkxVMJHsvHf SrdBRor66bJvLYcdrqXqx489x8C3ccS3QPcYlt5ok8YsEux6U/e0jbOUiov6nJuvTW0sKlw/F WgxnqCZBpsV1IqzOEW0oEwO8IfZ2cQ8ukxM7LlXa637iLi2PSRNBu0g0zLqjLBhoapdwlVdxH zskwT5TBq3PT3d8ZnB1NUj9OUWW5wUlxymazEFHwp5UCbmvQXeNGngqPFtkS6Tr1U92D9Bj7O +TPrrtsdrEpxKK+hOIetLuipFjT+4bdM/XVzgXLdyD8NDkPjSiv8DQPlb9Ypu8WdQgJs0ernJ RiwJGY3R5qdL6ef/7+uHhxtYaSFHuLmk1pl340ZT6GgXSLSyMpbDO7pXwRpu02lq1FOdqwPQt jKdoeWwh4didkuBEEcv0uYRHKnVEYDbUsCUMTgIaHf+kr+EYDnfVn4VOsWW1TnrgKvTM5ssPn jlYEYDs6IhuyQNp4rHgz2XzaLq5bZaJPklEqwSB4kONkS8B3yPzIouXxgXbFeo9f5vveRHwSr S051Tw8+zyJJXeUdOlQIRWs2ckS5Ry434IN7rKns4dzltZkozOerRW4VYAjdy9GUBN9Qzf/1S bIVSKSYwhb1E5+PBGzJLLdAPb7bwQFeq+y4qgRM1yWr1hWgtp4dG+n0dfHmlFF9tRpsubTXlc zuvAkbLHT0JlyOSEd4zXtmv/F263EFhscjpLk3Uq7WEasOiHFI/6VcDtL3sE6u9R+2p9W7WgJ N+BbEVIxWewTldRTt79gVei8BfOcSSiZHEYe0/vc07Za5mpT03OvvB1Sp96hsXKj1BbZIJyT X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:130925 Archived-At: Andreas Politz writes: > Hey ! Hi Andreas, > Below is a second version of the previous patch. It is somewhat > conservative, since neither did I attempt to > > + further simplify filenotify.el nor > > + handle differing masks in inotify.c . Thanks for this. Next time you provide a patch, could you pls merge emacs recent changes from master first? Your patch was rejected partly, and I had to apply rejected hunks manually. Comments: > diff --git a/lisp/filenotify.el b/lisp/filenotify.el > index 7eb6229976..5dea67b580 100644 > --- a/lisp/filenotify.el > +++ b/lisp/filenotify.el > @@ -25,6 +25,20 @@ > ;; file notification packages `inotify', `kqueue', `gfilenotify' and > ;; `w32notify'. > > +;; TODO: Pls move TODOs at the end of the file. > +;; * "inotify_add_watch adds a new watch, or modifies an existing watch" > +;; We need to make sure that different watches for the same directory > +;; don't set the mask in a conflicting way regarding changed/attribute-changed > +;; * Also check which other inotify flags are problematic > +;; for concurrent use of the underlying descriptor Well, I had always the hope to modify inotify watches in this case. If there is a watch with flags f1, and a new watch for the same file is requested with flags f2, and f2 contains a flag which is not part of f1, then either the existing watch shall be adapted, or the existing watch shall be removed, and a new shall be installed. Don't know what's possible in inotify. > @@ -48,16 +62,14 @@ file-notify-descriptors > > (DIR (FILE . CALLBACK) (FILE . CALLBACK) ...) > > -Several values for a given DIR happen only for `inotify', when > -different files from the same directory are watched.") > +Several values for a given DIR should currently not occur.") Remove "currently". Every docstring speaks about current state. > (defun file-notify-rm-watch (descriptor) > "Remove an existing watch specified by its DESCRIPTOR. > DESCRIPTOR should be an object returned by `file-notify-add-watch'." > - (let* ((desc (if (consp descriptor) (car descriptor) descriptor)) > - (file (if (consp descriptor) (cdr descriptor))) > - (registered (gethash desc file-notify-descriptors)) > + (let* ((file nil) > (defun file-notify-valid-p (descriptor) > "Check a watch specified by its DESCRIPTOR. > DESCRIPTOR should be an object returned by `file-notify-add-watch'." > - (let* ((desc (if (consp descriptor) (car descriptor) descriptor)) > - (file (if (consp descriptor) (cdr descriptor))) > - (registered (gethash desc file-notify-descriptors)) > + (let* ((file nil) In both functions I believe we don't need to bind `file'. The code could be simplified, because (or (not file) ...) always succeeds. Your changes look good; "make -C test filenotify-tests SELECTOR='$(SELECTOR_DEFAULT)'" passes all tests. Even if there is room for improvement I believe you could push your patch to master now, in order to get feedback from other developers. > I also thought about the test-cases and more generally about how to > develop a specification for this library, i.e. how do we want this to > behave. Do we have the desire that it works uniformly across all > participating back-ends ? And is that even possible ? As Eli said, that's the intention. But we cannot reach this goal completely due to the different behaviour of the libraries. > I think it is to easy to adapt the tests for each back-end, until they > succeed and thereby potentially masking actual bugs. That's what file-notify-test.el intends to do. Well, the code has evolved over the time, and it is somehow hard to read. Improvements are welcome! > One way to go about this would be to write a series of definitive > unit-tests which specify the intended behavior. Then, allow them to fail > for a specific back-end, until someone has fixed potential bugs for it > and confirmed that the test succeeds. This would allow for an > incremental improvement on fairly solid grounds. I'm assuming that > people of the future are interested in improving their used back-end > (e.g. make kqueue watch directories properly, if that is possible). Could you show an example how this shall look like? > Anyway, I was bored today, so I took a look at what events these > libraries actually produce, the result of which you may also find below. Thanks; I'll review it next days. > Finally, I'm tempted to suggest to get rid of the flags argument of > file-notify-add-watch. As it is, things are already complicated enough > and we don't seem to have many people working on this. I think we could > make it backward-compatible to a certain degree. Note also, that many > file operations trigger both kinds of events anyway. I agree with you. I haven't seen any different use of the flags yet (but I maybe wrong). Best regards, Michael.