Skip to content

Fix #514#515

Merged
BoboTiG merged 4 commits into
gorakhargosh:masterfrom
rrzaripov:fix-subdirs-not-renamed-after-moving-parent-dir
Jul 2, 2019
Merged

Fix #514#515
BoboTiG merged 4 commits into
gorakhargosh:masterfrom
rrzaripov:fix-subdirs-not-renamed-after-moving-parent-dir

Conversation

@rrzaripov

Copy link
Copy Markdown
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

BoboTiG commented Feb 16, 2019

Copy link
Copy Markdown
Collaborator

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

@BoboTiG

BoboTiG commented Mar 24, 2019

Copy link
Copy Markdown
Collaborator

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

pablomartinezm commented Mar 25, 2019

Copy link
Copy Markdown

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

rrzaripov commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

@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

@BoboTiG BoboTiG left a comment

Copy link
Copy Markdown
Collaborator

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 :)

Comment thread src/watchdog/observers/inotify_c.py Outdated
Comment thread src/watchdog/observers/inotify_c.py Outdated
Comment thread src/watchdog/observers/inotify_c.py Outdated
@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
Copy Markdown
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

BoboTiG commented Apr 23, 2019

Copy link
Copy Markdown
Collaborator

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

@BoboTiG

BoboTiG commented Apr 24, 2019

Copy link
Copy Markdown
Collaborator

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

@rrzaripov

Copy link
Copy Markdown
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

Comment thread tests/test_emitter.py Outdated
Comment thread tests/test_emitter.py
@BoboTiG

BoboTiG commented Jun 11, 2019

Copy link
Copy Markdown
Collaborator

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

rrzaripov commented Jul 1, 2019

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Contributor Author

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

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

BoboTiG commented Jul 2, 2019

Copy link
Copy Markdown
Collaborator

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.

3 participants