Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #514 #515

Conversation

rrzaripov
Copy link
Contributor

This PR will fix the issue with observing on directory in recursive mode. When directory be moved, then new events for child directories and files still report old path.
This PR needs code review, I think this patch may to be done in more intelligent or elegant way.
Tests will be added in near time.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 16, 2019

Thanks @rrzaripov!
Do you are working on adding tests?

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 24, 2019

Also, we should check this is fixed for all patforms.

@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from e0b2ecc to 90476cd Compare March 25, 2019 13:44
@pablomartinezm
Copy link

pablomartinezm commented Mar 25, 2019

There's a potential error using 'in' and 'startswith'. Figure out a folder system like this:

My folder:
a/
a/b
a/bb

Action

  1. Move /a/b to /a/c

My virtual folder after action:
a/
a/c
a/cb

You should at least append the system separator at the end of the move_src_path to be sure that you are comparing the path:

if _path.startswith(move_src_path + os.path.sep):

@rrzaripov
Copy link
Contributor Author

rrzaripov commented Mar 25, 2019

@pablomartinezm good catch! I start rework this ASAP.

@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from c7fc0cf to 8d5f60b Compare March 26, 2019 12:08
Copy link
Collaborator

@BoboTiG BoboTiG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you @rrzaripov. Just few adjusments proposed :)

src/watchdog/observers/inotify_c.py Outdated Show resolved Hide resolved
src/watchdog/observers/inotify_c.py Outdated Show resolved Hide resolved
src/watchdog/observers/inotify_c.py Outdated Show resolved Hide resolved
@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from 8cfd007 to b4a7699 Compare April 23, 2019 07:35
@rrzaripov
Copy link
Contributor Author

@BoboTiG I'm reworked PR, your suggestions applied, thank you! I'm sorry, your suggestions from code review not included as additional commit, my mistake, just first time work with code review on github.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Apr 23, 2019

Thanks @rrzaripov, I will merge it later in the day :)

@BoboTiG
Copy link
Collaborator

BoboTiG commented Apr 24, 2019

🤔 The test passes without the patch applied. Something is missing perhaps?

@rrzaripov
Copy link
Contributor Author

@BoboTiG I have stuck with this. When similar test scenario running from console (as bash script), I get expected fail. When test running using pytest result is unexpectedly success. I'm still trying understand what going on, but this make me crazy.

Bash script

(venv) root@debian-9:/home/vagrant/watchdog# cat test.sh
python3 ./test.py > /tmp/log.txt &
p=$!
sleep 3

mkdir A
mkdir A/B
mv A A2
touch A2/B/example.txt

sleep 3
kill -KILL $p

cat /tmp/log.txt
rm -rf A2

Output

2019-06-11 16:00:33 - Created directory: ./A
2019-06-11 16:00:33 - Modified directory: .
2019-06-11 16:00:33 - Created directory: ./A/B
2019-06-11 16:00:33 - Modified directory: ./A
2019-06-11 16:00:34 - Moved directory: from ./A to ./A2
2019-06-11 16:00:34 - Modified directory: .
2019-06-11 16:00:34 - Moved directory: from ./A/B to ./A2/B
2019-06-11 16:00:34 - Moved file: from ./A/B/example.txt to ./A2/B/example.txt
2019-06-11 16:00:34 - Created file: ./A/B/example.txt
2019-06-11 16:00:34 - Modified directory: ./A/B
2019-06-11 16:00:34 - Modified file: ./A/B/example.txt

tests/test_emitter.py Outdated Show resolved Hide resolved
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 11, 2019

With my new suggestions, the test is failing. do you think it is more correct like that?

@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from da6d9c3 to 685051f Compare June 28, 2019 16:22
@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from 685051f to f3c8af0 Compare June 28, 2019 16:29
@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from 991ff70 to 87dd988 Compare July 1, 2019 09:43
@rrzaripov
Copy link
Contributor Author

rrzaripov commented Jul 1, 2019

@BoboTiG I'm reworked test and added separate test for Windows, because events sets very different in comparison with Linux. While testing, two interesting things was discovered.

  1. In Linux sometimes DirModifiedEvent event is catched. Need to investigate when this occuring and, possible increase timeout to make this test more simple and robust.
  2. In Windows DirCreatedEvent event emitted twice while created subdirectory B. I think this is a bug, and I will to investigate.

@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch 2 times, most recently from 5068e7d to caa9875 Compare July 2, 2019 09:33
@rrzaripov rrzaripov force-pushed the fix-subdirs-not-renamed-after-moving-parent-dir branch from caa9875 to 858e7b4 Compare July 2, 2019 09:38
@rrzaripov
Copy link
Contributor Author

@BoboTiG tests passed, PR ready to merge:-)

@BoboTiG BoboTiG merged commit 18bb843 into gorakhargosh:master Jul 2, 2019
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jul 2, 2019

Thank you very much @rrzaripov!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants