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

Submodule status: Throttle instead of drop frequent updates #8119

Merged

Conversation

gerhardol
Copy link
Member

I believe this is ready, but did not plan to include it in 3.4
Submitting now for discussion of #8049

Proposed changes

SubmoduleStatusProvider will not update the status more often than every 15th second to limit load.
This was added before the status update was fully async and before updates were pruned (latest change in #8116).
The status should not be noticable (at least comparable to git-status, that can take a long time in big repos with many submodules).

The update limitation remains with this PR, but instead of dropping frequent updates, the handling is throttled to update no more often than every 15th second.
The handling has also been simplified.

A change to specific mention is that previously, if not the top module was selected, the complete structure including the current submodule was updated. The current submodule was then updated a second time. (There was a comment that the update not triggered by git-status would skip this, but that was incorrect.)
This will maybe be noticeable on a deep tree where a module below submodule is updated. (but very minor effect)

Test methodology

Tests exists, a test is disabled for the same reason as before.
Can maybe be improved.


✒️ I contribute this code under The Developer Certificate of Origin.

@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

Merging #8119 into master will increase coverage by 0.26%.
The diff coverage is 81.94%.

@@            Coverage Diff             @@
##           master    #8119      +/-   ##
==========================================
+ Coverage   49.53%   49.80%   +0.26%     
==========================================
  Files         852      845       -7     
  Lines       61296    61459     +163     
  Branches    11016    11066      +50     
==========================================
+ Hits        30364    30607     +243     
+ Misses      28669    28545     -124     
- Partials     2263     2307      +44     
Flag Coverage Δ
#production 37.41% <81.60%> (+0.08%) ⬆️
#tests 94.38% <82.00%> (-0.46%) ⬇️

@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from e29a74a to 5e7df72 Compare May 17, 2020 15:31
@gerhardol
Copy link
Member Author

Squashed and rebased https://github.com/RussKie/gitextensions/tree/fix_8077 on master, rebased this on that commit and fixed the commented out tests, added a few lines more
This was a problem with the datastructures

The bug in the application is not too severe. The status for submodules if not the top project was checked out was incorrect. So the problem requires at least three levels of submodules.

The drop down looked like this:
image

With the correction, it looks like the following (sidepanel is correct)
image

@gerhardol gerhardol mentioned this pull request May 17, 2020
@gerhardol gerhardol requested review from mstv and RussKie May 17, 2020 15:46
@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch 2 times, most recently from e98acd1 to 5e7df72 Compare May 17, 2020 16:45
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

LGTM, needs another rebase when #8049 is merged.

@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from 5e7df72 to a8d832f Compare May 18, 2020 23:02
@gerhardol
Copy link
Member Author

gerhardol commented May 18, 2020

The current implementation can be further simplified, no need to separate topmodule and submodule structure update

I will adjust some details

@gerhardol
Copy link
Member Author

gerhardol commented May 20, 2020

I submitted this to support #8049 in a safe way, but it has maybe grown too big for 3.4
I would like to add some more test cases for changing revisions as well (could be done later too).
I may submit an alternative with a few line changes for 3.4 instead: gerhardol@df91842

@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from 9827c27 to 07e4023 Compare May 20, 2020 20:55
@gerhardol
Copy link
Member Author

gerhardol commented May 20, 2020

Testcases added, also checking AllSubmodules/OurSubmodules (what failed in the old testcases)
I added 300 lines of testcases, now many more but not all usages are covered. So it is still not a safenet. (Also before reviews, I am blind for issues myself now.)
Added some tests for the actual working, even if it is incomplete. For instance, if not a top module is checked out, then git-status run on the submodule and can determine that the top module is dirty. However, clearing the dirty file cannot determine that the top module is no longer dirty.

Edit: ChangedFiles does not report the expected number of changes in Appveyor, cannot see why now...

@mstv
Copy link
Member

mstv commented May 20, 2020

ChangedFiles does not report the expected number of changes in Appveyor

Without having looked at the code, it might help to add
await AsyncTestHelper.JoinPendingOperationsAsync(AsyncTestHelper.UnexpectedTimeout);
and forceUpdate: true.

@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from 96e9314 to 38fd912 Compare May 22, 2020 15:21
@gerhardol
Copy link
Member Author

Split tests and added a few more. Six tests fails on AppVeyor though. It is consistent behavior though, no evident race condition.
I check what Git options Appveyor used, nothing strange (and GE should override user settings anyway if needed.)
Will squash this (it is test cases that has changed since the review).

@mstv any more hints to why the tests fail?
Anything specific to be done so the testcases are not cleaned up and access with RDP?

@mstv
Copy link
Member

mstv commented May 22, 2020

Six tests fails on AppVeyor though.

I have not found a regarding failed AppVeyor run.

Will squash.

OK for me.

@mstv
Copy link
Member

mstv commented May 22, 2020

Printf debugging still is not the worst idea.
You could attach the debugger to the test runner.
(IIRC, the AppVeyor VM has only one cpu core with hyperthreading.)

@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from 38fd912 to d1a35ea Compare May 23, 2020 12:07
@gerhardol
Copy link
Member Author

Eventually fixed the problem, same problem as #7861: AppVeyor does not set an email address which causes commit to fail.
The testcases sets email addresses in the local config but when adding a module as a submodule, the local config is not copied.
I intend to squash the two first commits.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Thank you

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request May 23, 2020
A few changes backported from gitextensions#8119
 * If not top project is current and current module has changed commits,
git-status with no dirty will not clear the current ahead/behind

 * If not top project is current and git-status is initially not dirty for super projects
but there are changed commits in superior projects, the status is now set as dirty

* If a submodule is no longer dirty, the status was no longer cleared when initiated from git-Status

* Some attempts to not refresh status twice
@gerhardol
Copy link
Member Author

One more corner case, with another test case

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request May 24, 2020
A few changes backported from gitextensions#8119

 * If not top project is current and current module has changed commits,
git-status with no dirty will not clear the current ahead/behind

 * If not top project is current and git-status is initially not dirty for
super projects but there are changed commits in superior projects,
the status is now set as dirty

* If a submodule is no longer dirty, the status was no longer cleared when
initiated from git-Status

* Some attempts to not refresh status twice
@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from c326c8b to 8267e9a Compare May 31, 2020 19:17
@gerhardol gerhardol force-pushed the bugfix/i8115-submodule-gitstatus branch from 8267e9a to 67b328d Compare June 8, 2020 04:50
@gerhardol
Copy link
Member Author

@msftbot merge in 24 hours

@ghost ghost added the status: auto merge label Jun 8, 2020
@ghost
Copy link

ghost commented Jun 8, 2020

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 09 Jun 2020 05:02:25 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit d83d689 into gitextensions:master Jun 9, 2020
@ghost ghost added this to the 4.0 milestone Jun 9, 2020
@gerhardol gerhardol deleted the bugfix/i8115-submodule-gitstatus branch June 9, 2020 19:49
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants