Revise Git watchers #6352

AlexTugarev · 2019-10-09T09:01:13Z

What it does

Current Git watchers have several problems:

sync calls for not-watching watchers leave them in watching state
git status runs may supersede each other
singleton repository manager depending on connection scoped git

Fixes #4436

How to test

there's some debug logging added in a separate commit to this PR, it helps to see what's going on behind the scene.
test cases:
1. open a workspace with two git repositories
  - see only one watcher is running; try switch to second repo
2. open a second frontend and make sure to have the same git repo selected as in first one
  - see only one watcher is running
  - switch to the other git repo in both; see only one watcher is running
  - have different repos selected; see two watchers are running

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

westbury · 2019-10-09T13:12:20Z

We have tested the commits here with Mbed Studio and it works as intended. However this breaks our application because the user no longer sees SCM decorators for any changes the user may have made outside the selected Source Control Provider.

We have been releasing Mbed Studio with #3971 cherry-picked into the release branch. This has solved this issue very well for our particular case. I was unaware of issue #4436 and I had thought this was a performance problem that was specific to us, so the issue had not been a priority for me.

We would like to see a solution that maintains the decorators on all projects. The showing of aggregated SCM decorators on parent folders has little value if the users can't see the current status of all folders/projects. I do believe there are solutions that can satisfy all situations and keep all the decorators.

AlexTugarev · 2019-10-09T13:34:14Z

However this breaks our application because the user no longer sees SCM decorators for any changes the user may have made outside the selected Source Control Provider

@westbury if it's part of your application to watch for changes of all repositories, please add this functionality to your application.

from what I learned is, that it was never intended to watch for all repositories for changes. the bug was a combination of issues and the effect was everything but not consistent, i.e. you might have seen changes from a second, but not from a third repo in your workspace.

packages/git/src/common/git.ts

AlexTugarev · 2019-10-09T13:38:01Z

packages/git/src/node/dugite-git.spec.ts

@@ -769,20 +769,20 @@ describe('log', function (): void {

    // See https://github.com/eclipse-theia/theia/issues/2143
    it('should not fail when executed from the repository root', async () => {
+        const git = await createGit();


otherwise git is not found

AlexTugarev · 2019-10-09T13:38:41Z

packages/git/src/node/git-backend-module.ts

-    bindBackendService(GitPath, Git);
+    // DugiteGit is bound in singleton scope; each connection should use a proxy for that.
+    const GitProxy = Symbol('GitProxy');
+    bind(GitProxy).toDynamicValue(ctx => new Proxy(ctx.container.get(DugiteGit), {}));


any disadvantages doing it like this?

AlexTugarev · 2019-10-09T13:41:25Z

packages/git/src/node/git-backend-module.ts

+    bind(DugiteGit).toSelf().inSingletonScope();
+    bind(Git).toService(DugiteGit);
+    bind(DefaultGitInit).toSelf();
+    bind(GitInit).toService(DefaultGitInit);


I still don't get why this is actually working :-(
I'd expected this to fail as DefaultGitInit depends on MessageService bound in connection scope.
any idea why?
@kittaakos, this used to fail, no?

AlexTugarev · 2019-10-09T13:44:42Z

packages/git/src/node/git-repository-manager.ts

        const reference = await this.getWatcher(repository);
        const watcher = reference.object;
-        reference.dispose();


in case the watcher was created here to sync after a git operation before a client called in to watch this repo, it happened to be start a zombie watcher not longer managed here.

AlexTugarev · 2019-10-09T14:24:50Z

@westbury, I double-checked with current master and I cannot follow your comment. switching between repositories causes updates of the tree decorates. this PR doesn't change anything there.

westbury · 2019-10-09T15:47:14Z

switching between repositories causes updates of the tree decorates. this PR doesn't change anything there.

You are correct, my mistake. I am so used to having our patch in our application that I was not aware that that was the default behavior. I checked back to 11ce1e7 (the oldest commit I could test without installing node 8) and that also only showed decorators on the selected project.

if it's part of your application to watch for changes of all repositories, please add this functionality to your application

Yes, that is what we do but I backed it out for the purpose of testing this PR as I was unaware of the intended default behavior so misunderstood the intent of this PR. Thank you for clarifying.

AlexTugarev · 2019-10-10T10:28:04Z

@svenefftinge please have a look again, I replaced the stm with a loop, it shaves off ~40loc, keeping the rest intact.

svenefftinge

LGTM, thank you!

- trying to guarantee watcher do not leak - `sync` called in `GitRepositoryManager.run` in not-watching state must not leave it watching when done - watcher created per repository should not depend on something bound in connection scope - `Git` should be bound as singleton - connection scoped git access should be a proxy for the singleton git - `git status` calls for same repository should not supersede each other - `sync` requests should not pile up when a watcher is busy ref #4436 Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>

kittaakos

I have verified the followings on macOS with the electron example:

test cases:

open a workspace with two git repositories
- see only one watcher is running; try switch to second repo
open a second frontend and make sure to have the same git repo selected as in first one
- see only one watcher is running
- switch to the other git repo in both; see only one watcher is running
- have different repos selected; see two watchers are running

Please update the changelog with the API changes. Thank you!

AlexTugarev · 2019-10-10T14:28:08Z

Thanks for reviewing! I'm removing the debug logging commit & waiting for the build & mergin then.

AlexTugarev force-pushed the at/fix-git-watcher branch from 9e93a75 to 80cbe80 Compare October 9, 2019 09:11

AlexTugarev force-pushed the at/fix-git-watcher branch from 80cbe80 to a1524f0 Compare October 9, 2019 13:22

AlexTugarev requested review from kittaakos, akosyakov, westbury and svenefftinge October 9, 2019 13:35

AlexTugarev marked this pull request as ready for review October 9, 2019 13:36

AlexTugarev commented Oct 9, 2019

View reviewed changes

packages/git/src/common/git.ts Outdated Show resolved Hide resolved

AlexTugarev commented Oct 9, 2019

View reviewed changes

AlexTugarev force-pushed the at/fix-git-watcher branch from a1524f0 to dd8ef79 Compare October 10, 2019 10:26

svenefftinge approved these changes Oct 10, 2019

View reviewed changes

AlexTugarev force-pushed the at/fix-git-watcher branch from dd8ef79 to 8d78ae6 Compare October 10, 2019 12:01

kittaakos approved these changes Oct 10, 2019

View reviewed changes

AlexTugarev force-pushed the at/fix-git-watcher branch from 8d78ae6 to 20648a2 Compare October 10, 2019 14:29

akosyakov added the git issues related to git label Oct 10, 2019

AlexTugarev merged commit 3b34e86 into master Oct 10, 2019

akosyakov mentioned this pull request Oct 23, 2019

[git] Replace polling of Git repositories with file watching #3971

Closed

arekzaluski mentioned this pull request Dec 10, 2019

Fix git dispose issue ARMmbed/theia#40

Closed

1 task

vince-fugnitto deleted the at/fix-git-watcher branch September 29, 2022 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise Git watchers #6352

Revise Git watchers #6352

AlexTugarev commented Oct 9, 2019 •

edited

Loading

westbury commented Oct 9, 2019

AlexTugarev commented Oct 9, 2019

AlexTugarev Oct 9, 2019

AlexTugarev Oct 9, 2019

AlexTugarev Oct 9, 2019

AlexTugarev Oct 9, 2019

AlexTugarev commented Oct 9, 2019

westbury commented Oct 9, 2019

AlexTugarev commented Oct 10, 2019

svenefftinge left a comment

kittaakos left a comment

AlexTugarev commented Oct 10, 2019

Revise Git watchers #6352

Revise Git watchers #6352

Conversation

AlexTugarev commented Oct 9, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

westbury commented Oct 9, 2019

AlexTugarev commented Oct 9, 2019

AlexTugarev Oct 9, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 9, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 9, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 9, 2019

Choose a reason for hiding this comment

AlexTugarev commented Oct 9, 2019

westbury commented Oct 9, 2019

AlexTugarev commented Oct 10, 2019

svenefftinge left a comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

AlexTugarev commented Oct 10, 2019

AlexTugarev commented Oct 9, 2019 •

edited

Loading