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

pywin32 notify during build makes dag refresh all the time #516

Closed
sthalik opened this Issue Nov 2, 2015 · 20 comments

Comments

Projects
None yet
3 participants
@sthalik
Contributor

sthalik commented Nov 2, 2015

With pywin32 for notify support, make -j4 makes the git dag blink all the time. dag feels the need to refresh commit list on changes to gitignored build directory.

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 Nov 2, 2015

Contributor

Can you report which git-cola version you are using? My unreleased changes to the filesystem notification code should hopefully help alleviate this problem, as it now avoids refreshing until it has not detected any filesystem activity for at least 888ms (previously upon detecting filesystem activity, it would wait 888ms and then refresh, even if there had been more filesystem activity in the interim). However, as I just commented in issue #517, an additional improvement would be to not refresh at all if the file in question is ignored by git.

Contributor

living180 commented Nov 2, 2015

Can you report which git-cola version you are using? My unreleased changes to the filesystem notification code should hopefully help alleviate this problem, as it now avoids refreshing until it has not detected any filesystem activity for at least 888ms (previously upon detecting filesystem activity, it would wait 888ms and then refresh, even if there had been more filesystem activity in the interim). However, as I just commented in issue #517, an additional improvement would be to not refresh at all if the file in question is ignored by git.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Nov 2, 2015

Contributor

For the case of DAG, unless dot.git gets refreshed should be more precise, yes? DAG doesn't care about uncommitted changes.

Contributor

sthalik commented Nov 2, 2015

For the case of DAG, unless dot.git gets refreshed should be more precise, yes? DAG doesn't care about uncommitted changes.

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 Nov 2, 2015

Contributor

Ah, yes. I don't really use git dag but it makes sense that it doesn't need to refresh the way the status browser does. I'll need to look at a way to make sure filesystem monitoring doesn't affect the DAG. Thanks for reporting.

Contributor

living180 commented Nov 2, 2015

Ah, yes. I don't really use git dag but it makes sense that it doesn't need to refresh the way the status browser does. I'll need to look at a way to make sure filesystem monitoring doesn't affect the DAG. Thanks for reporting.

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 Nov 13, 2015

Contributor

@davvid, @sthalik Do you think it makes sense to have git dag auto-refresh on new commits? Trying to decide whether to use more limited filesystem monitoring for git dag or to disable it altogether. Since I don't really use git dag, user input would be helpful.

Contributor

living180 commented Nov 13, 2015

@davvid, @sthalik Do you think it makes sense to have git dag auto-refresh on new commits? Trying to decide whether to use more limited filesystem monitoring for git dag or to disable it altogether. Since I don't really use git dag, user input would be helpful.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Nov 13, 2015

Contributor

@living180 it could be refreshed on any new commit, branch, rebase, or tag change. These changes are rare enough to warrant no additional processing. Note the commit-ish selector in dag that can point at anything.

Contributor

sthalik commented Nov 13, 2015

@living180 it could be refreshed on any new commit, branch, rebase, or tag change. These changes are rare enough to warrant no additional processing. Note the commit-ish selector in dag that can point at anything.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Dec 19, 2015

Contributor

@living180 a simpler option would be to use Nagle's algorithm for notify refreshes in git dag.

Contributor

sthalik commented Dec 19, 2015

@living180 a simpler option would be to use Nagle's algorithm for notify refreshes in git dag.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Dec 22, 2015

Member

@living180 refreeshing on new commits or branches would be pretty cool, but maybe there's some things that can be tweaked project-side to make it more amenable.

If "make" is triggering refreshes then that suggests that the directories where file creation/modification is happening is inside a directory that is not ignored.

If you don't want git-dag refreshing then one way to make that happen is to add that directory to your .gitignore. That's the first thing to try to do. Possibly re-arranging the directory structure to place non-source files in a .gitignore-able subdir would help since we would no longer be watching items inside that directory.

For example, In the git-cola project we can do make && rm -rf build/scripts-* in a loop and git-dag will not refresh. This is because we have /buildin .gitignore. But, there are still situations where the file being created is adjacent to a tracked file, so maybe we can do something to make that less painful.

@sthalik, if I'm understanding your Nagle reference correctly then you are suggesting that fsmonitory/inotify refreshes should be throttled so that they don't happen too often. That's a great idea ;-) it actually already works that way, but our interval is rather short. I can imagine that maybe someone might want to crank this number up to something much higher if it makes their workflow smoother. Here's where the throttle is currently defined:

https://github.com/davvid/git-cola/blob/master/cola/fsmonitor.py#L75

I can see it being useful to make that a configurable run-time thing. git config cola.updateinterval or something like that.

The only downside is that we won't see new files appear in the untracked file list until after the interval expires, but it would help avoid this papercut. What do you all think?

Member

davvid commented Dec 22, 2015

@living180 refreeshing on new commits or branches would be pretty cool, but maybe there's some things that can be tweaked project-side to make it more amenable.

If "make" is triggering refreshes then that suggests that the directories where file creation/modification is happening is inside a directory that is not ignored.

If you don't want git-dag refreshing then one way to make that happen is to add that directory to your .gitignore. That's the first thing to try to do. Possibly re-arranging the directory structure to place non-source files in a .gitignore-able subdir would help since we would no longer be watching items inside that directory.

For example, In the git-cola project we can do make && rm -rf build/scripts-* in a loop and git-dag will not refresh. This is because we have /buildin .gitignore. But, there are still situations where the file being created is adjacent to a tracked file, so maybe we can do something to make that less painful.

@sthalik, if I'm understanding your Nagle reference correctly then you are suggesting that fsmonitory/inotify refreshes should be throttled so that they don't happen too often. That's a great idea ;-) it actually already works that way, but our interval is rather short. I can imagine that maybe someone might want to crank this number up to something much higher if it makes their workflow smoother. Here's where the throttle is currently defined:

https://github.com/davvid/git-cola/blob/master/cola/fsmonitor.py#L75

I can see it being useful to make that a configurable run-time thing. git config cola.updateinterval or something like that.

The only downside is that we won't see new files appear in the untracked file list until after the interval expires, but it would help avoid this papercut. What do you all think?

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Dec 22, 2015

Contributor

@davvid the build subdirectory is ignored and doesn't appear on Cola's untracked files list either.

The Nagle in this case is to ignore idempotent changes until they no longer come, and fire the signal after the last of them.

To my understanding none of the tracked project files change during the build process, only contents of the latter.

Contributor

sthalik commented Dec 22, 2015

@davvid the build subdirectory is ignored and doesn't appear on Cola's untracked files list either.

The Nagle in this case is to ignore idempotent changes until they no longer come, and fire the signal after the last of them.

To my understanding none of the tracked project files change during the build process, only contents of the latter.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Dec 22, 2015

Contributor

What should optimally be done is filter out everything but .git/* changes and ignore in dag for the purposes of commit list changes.

Contributor

sthalik commented Dec 22, 2015

What should optimally be done is filter out everything but .git/* changes and ignore in dag for the purposes of commit list changes.

living180 added a commit to living180/git-cola that referenced this issue Jan 1, 2016

fsmonitor: limit monitoring when running git-dag
When running git-dag, pass a flag into fsmonitor.instance().start()
which disables monitoring the worktree and the index, causing only
.git/HEAD and the .git/refs directory to be monitored.  This prevents
unnecessary refreshes when changes are made to the worktree or the
index, which do not affect what is displayed by git-dag.

Fixes: #516
Reported-by: Stanislaw Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>

@living180 living180 closed this in be9966a Jan 9, 2016

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Mar 29, 2016

Contributor

@living180 @davvid unfortunately there are refreshes in cola dag while compiling. .git/index is untouched for days or more. This is on Windows.

Contributor

sthalik commented Mar 29, 2016

@living180 @davvid unfortunately there are refreshes in cola dag while compiling. .git/index is untouched for days or more. This is on Windows.

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 Apr 12, 2016

Contributor

@sthalik sorry it has taken me a bit to look at this. Although I don't normally use git-dag (just git-cola), I do use Windows as my dev environment. I haven't been able to trigger refreshes in git-dag through any sort of filesystem changes in the worktree. (FWIW, .git/index changes shouldn't trigger refreshes either - just changes to .git/HEAD or anything under .git/refs/).

I'm happy to try to help figure this out - can you give me more information about your system/setup so we can figure out what is going on?

Contributor

living180 commented Apr 12, 2016

@sthalik sorry it has taken me a bit to look at this. Although I don't normally use git-dag (just git-cola), I do use Windows as my dev environment. I haven't been able to trigger refreshes in git-dag through any sort of filesystem changes in the worktree. (FWIW, .git/index changes shouldn't trigger refreshes either - just changes to .git/HEAD or anything under .git/refs/).

I'm happy to try to help figure this out - can you give me more information about your system/setup so we can figure out what is going on?

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 May 4, 2016

Contributor

@sthalik - I just stumbled upon a way to reproduce this. When running git-dag by itself, the problem is absent, but when choosing View -> DAG... from within git-cola, I see what you reported. I'll need to change my approach to handle that case.

Contributor

living180 commented May 4, 2016

@sthalik - I just stumbled upon a way to reproduce this. When running git-dag by itself, the problem is absent, but when choosing View -> DAG... from within git-cola, I see what you reported. I'll need to change my approach to handle that case.

@living180 living180 reopened this May 4, 2016

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik May 10, 2016

Contributor

@living180 thanks for the continued interest. Ping me when you need to test something.

Contributor

sthalik commented May 10, 2016

@living180 thanks for the continued interest. Ping me when you need to test something.

living180 added a commit to living180/git-cola that referenced this issue May 12, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

living180 added a commit to living180/git-cola that referenced this issue May 12, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>

living180 added a commit to living180/git-cola that referenced this issue May 12, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

living180 added a commit to living180/git-cola that referenced this issue May 12, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>
@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 May 12, 2016

Contributor

@sthalik if you want, you can try the fsmonitor_check_ignore branch in my fork and see how it does for you. I'll be testing it a bit for myself and if everything looks good will submit a PR.

Contributor

living180 commented May 12, 2016

@sthalik if you want, you can try the fsmonitor_check_ignore branch in my fork and see how it does for you. I'll be testing it a bit for myself and if everything looks good will submit a PR.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik May 13, 2016

Contributor

@living180 I get no refreshes at all during ninja install. On the other hand git checkout HEAD^^^^^ results in no automatic update to DAG.

Contributor

sthalik commented May 13, 2016

@living180 I get no refreshes at all during ninja install. On the other hand git checkout HEAD^^^^^ results in no automatic update to DAG.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Jun 9, 2016

Contributor

@living180 with pywin32 I get a refresh when HEAD changes, No refreshes during build. All is fine.

Contributor

sthalik commented Jun 9, 2016

@living180 with pywin32 I get a refresh when HEAD changes, No refreshes during build. All is fine.

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Aug 9, 2016

Contributor

@davvid @living180 please consider merging. The rebased version of this build works great after all the time.

Contributor

sthalik commented Aug 9, 2016

@davvid @living180 please consider merging. The rebased version of this build works great after all the time.

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>
@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik
Contributor

sthalik commented Aug 21, 2016

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 21, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Aug 22, 2016

Member

I'm on vacation right now so I haven't had a chance to get back to this but I do want to merge this topic. @living180 would you mind opening a PR so we can merge this? If you're busy I won't mind taking @sthalik 's rebased version

Member

davvid commented Aug 22, 2016

I'm on vacation right now so I haven't had a chance to get back to this but I do want to merge this topic. @living180 would you mind opening a PR so we can merge this? If you're busy I won't mind taking @sthalik 's rebased version

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 25, 2016

fsmonitor: remove the refs_only parameter
It turns out that using the refs_only parameter for the start() method
of the _Monitor class was inadequate for preventing unwanted refreshes
in the DAG window.  It worked when running git-dag directly, but when
the DAG window as launched from within git-cola, the undesired refreshes
were still present (because in that case the start() method is called
with refs_only=False).

Related-to: #516
Signed-off-by: Daniel Harding <dharding@living180.net>

sthalik added a commit to sthalik/git-cola that referenced this issue Aug 25, 2016

fsmonitor: avoid refreshing for ignored files
Changes to files ignored by git won't cause display changes in git-cola,
but they were still causing the file system change monitoring to refresh
the display.  This commit avoids this behavior by using git check-ignore
with if git version 1.8.5 or later is present.  The logic in the file
system change monitor is enhanced so that it no longer triggers a
refresh on any filesystem change.  Now, if the change is for a file, it
adds the file path to a set of paths that gets checked in the
_BaseThread.notify() method.  This method passes the paths to git
check-ignore, and only triggers a refresh if at least one of the paths
is for a non-ignored file.

Fixes: #517
Related-to: #516

Reported-by: Fabio Coatti <fabio.coatti@gmail.com>
Reported-by: Stanisław Halik <sthalik@misaki.pl>
Signed-off-by: Daniel Harding <dharding@living180.net>

@davvid davvid closed this in 35aeed2 Sep 9, 2016

@sthalik

This comment has been minimized.

Show comment
Hide comment
@sthalik

sthalik Sep 9, 2016

Contributor

@davvid thanks for merging this. I can confirm that modifying staged files, adding and removing files all work with cola following the rebase and the last commit of the series.

Contributor

sthalik commented Sep 9, 2016

@davvid thanks for merging this. I can confirm that modifying staged files, adding and removing files all work with cola following the rebase and the last commit of the series.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment