add capability to ignore paths

[imsodin]

The reason for this PR is that currently installing watches recursively on a directory is aborted when any error is encountered. This PR adds the possibility to ignore errors based on their path. To achieve this a function that determines whether a path should be ignored or not has to be passed as argument to the Watch function. This function has to accepts a path as a string as argument and returns a bool (true for should be ignored).

Possible caveats:

• Only parts of the package that were used in my scenario or that were necessary for compilation were modified. There are possibly more modifications necessary to adapt the package as a whole to the introduced change to the high level Watch function.
• The PR was only tested in conjunction with synchting-inotify (adjust for notify ignore capability syncthing/syncthing-inotify#103) on linux and ext4 FS. All changes seem to be on a higher level than OS/FS, so it should work also on other OS/FS, but the point above may be a problem.

[Zillode]

Thanks for your work, but could you first propose/discuss it upstream? I'm gonna leave this PR open and see how the discussion goes.

[imsodin]

As it is a very syncthing-inotify centered approach (I just needed it to get it running correctly on my own system) I thought I propose it on your fork. I wrote a comment on rjeczalik#79 , lets see what the reaction is.

[imsodin]

I now separated the ignoreTest specification from the Watch function. Thus there is no change in how to use the Watch function and everything remains just as it was before. Ignoring is enabled by calling SetIgnoreTest with the desired function that determines which paths are ignored.
So far no reaction over on rjeczalik's repo.

[imsodin]

As there is no reaction whatever (not even a "not interested") to this upstream it seems to be up to you what to do. I just want to repeat: My already incorporated PR syncthing/syncthing-inotify#101 is incorrect (suggesting everything is working while it is not) without the changes proposed here.
I am running these changes on my system and it works fine.

[Zillode]

@plouj any comments?

[plouj]

My suggestion is to rename the function to explain what it does more
clearly. I can't tell from a quick read of the code if this completely
ignores watching paths for which the function returns true, or if it just
ignores errors from such paths. In the former case I would rename
ignoreTest -> doNotWatch (and SetIgnoreTest -> SetDoNotWatch). In the
latter case I would rename ignoreTest -> ignoreErrorsFrom (and
SetIgnoreTest -> SetIgnoreErrorsFrom)

Another software design comment: this is adding a global function and
passes it around to other global functions which in essence makes the whole
package acts as an implicit class. A better practice would be to extract
affected functions into a real class (struct) and properly encapsulate them
as methods.

On Fri, Apr 29, 2016 at 1:20 PM, Zillode notifications@github.com wrote:

@plouj https://github.com/plouj any comments?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3 (comment)

[imsodin]

As far as I interpreted the code it should be the former: It does not install watches if the path is ignored (i.e. the introduced function returns true) and just continues installing watches. I will adapt the naming as proposed (probably by Sunday).

On your second note: I think I get the point (remember: I don't have any real software design training). My approach was just to implement what I need for syncthing-inotify to work with files that it has no read permissions on if those are ignored and at the same time make this change in the notify package optional, such that everything works as before if one does not use it. So I just tried to minimally change the code. I could try to create a class/struct/whatever is the appropriate object in golang, but it is likely that it will still not be good software design due to my lack of knowledge. I don't know if it is worth the time, also considering @plouj is working on a integration of realtime-notificatoin into syncthing, where this will anyway be done differently (? I do not know the approach there).

[plouj]

Ok. The second comment was more of a suggestion for future improvement, not
something that should block this feature from being added.
On Apr 29, 2016 16:47, "imsodin" notifications@github.com wrote:

As far as I interpreted the code it should be the former: It does not
install watches if the path is ignored (i.e. the introduced function
returns true) and just continues installing watches. I will adapt the
naming as proposed (probably by Sunday).

On your second note: I think I get the point (remember: I don't have any
real software design training). My approach was just to implement what I
need for syncthing-inotify to work with files that it has no read
permissions on if those are ignored and at the same time make this change
in the notify package optional, such that everything works as before if one
does not use it. So I just tried to minimally change the code. I could try
to create a class/struct/whatever is the appropriate object in golang, but
it is likely that it will still not be good software design due to my lack
of knowledge. I don't know if it is worth the time, also considering @plouj
https://github.com/plouj is working on a integration of
realtime-notificatoin into syncthing, where this will anyway be done
differently (? I do not know the approach there).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3 (comment)

[imsodin]

I looked at my changes again and I a lot of question marks popped up in my mind (it might be my implementation is completely erroneous and just worked by sheer luck):
The newly introduced function doNotWatch is global, so I should not have to pass it around by argument. So this prompted the first question: "How global" are global variables in go? doNotWatch is set in a bunch of different goroutines, however I do not find any documentation supporting that all variables are "confined within a goroutine". That means, that doNotWatch changes for all goroutines when it is changed somewhere by SetDoNotWatch. So actually if I had not passed around doNotWatch by argument, this would have been completely broken.
So now there are two possibilities:

• Variable scope is indeed restricted to a subroutine (like in shells, where the parent shell passes all its variables to the child process, but the child is completely independent afterwards). In this case I can remove all the passing around of arguments.
• Global variables are "shared between subroutines": Then this has to be passed around by argument, but then consistently. So no SetDoNotWatch function but passing doNotWatch as an additional argument to Watch.

I hope what I wrote is understandable and someone with actual go knowledge can bring me up to speed.

[Zillode]

It would be cleaner to pass in an 'initialized' function to the top level Watch call. This reduces the possibility for null pointers (e.g. when SetDoNotWatch was not called) and the need for global functions.

[imsodin]

@Zillode I think this is what I did for the updated PR rjeczalik#107 except that I did want to keep backwards compatibility and go has no optional arguments, so I added a new function which is basically Watch but takes an ignore function as argument. Maybe you can have a look at it there (no reaction over there once again) or I could make a provisional PR against this repo making the changes clear.

I am pretty certain now that my presently in use implementation is based on a race condition that just luckily works out in the correct way...