only update submodule status at changes

[gerhardol]

This is a follow up to #5747 after discussions with @amaiorano
This is part 1 of #5769 - should maybe be considered for 3.0

This is based on #5747 minus last commit, submitted for @amaiorano to give feedback

Changes proposed in this pull request:

SubmoduleStatus: Update only when commit-count reports changes to submodules

Update submodule status at changes directly (async)
when changes detected, do not wait for dropdown or form reactivation
Submodule status setting is now dependent on the status updates

There are two special cases:

• If submodule status changes to None, the status must be cleared
• Changes are only triggered on submodule changes, supermodule changes are not detected, If a module has supermodules, the status is always calculated when opening the module (but not triggered if there are changes in the supermodules.

Screenshots before and after (if PR changes UI):

• n/a

What did I do to test the code and ensure quality:

• Manual tests

Has been tested on (remove any that don't apply):

• Windows 7/10

[amaiorano]

Am testing the change. Will report back.

[amaiorano]

I think calling the bool "updateStatus" is confusing, considering the function we call is UpdateSubmodulesStatus. Not sure what's a better name, though. Either the bool or the function should be renamed. Maybe "updateDetailedStatus" or "getDetailedStatus" to match the name of the value that gets set: SubmoduleInfo.Detailed.

In any case, I'm not sure this is working the way you wanted. The last commit makes it so that I never see submodule status icons in the browse menu (when the option is enabled). The only place where it could possibly be updated is in the FormBrowse anonymous func registered on GitWorkingDirectoryStatusChanged:

                    if (AppSettings.ShowSubmoduleStatus && _submoduleStatusProvider.HasSubmodulesStatusChanged(status))
                    {
                        UpdateSubmodulesStatus(updateStatus: true);
                    }

When I change repo, and this gets called, _submoduleStatusProvider.HasSubmodulesStatusChanged(status) returns false. Not sure when this would return true.

Overall, I'm not sure I understand this change. If we enable status update in browse menu, shouldn't refresh, etc. always update it?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@a4534d3). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #5777   +/-   ##
=========================================
  Coverage          ?   36.62%           
=========================================
  Files             ?      623           
  Lines             ?    47464           
  Branches          ?     6307           
=========================================
  Hits              ?    17383           
  Misses            ?    29349           
  Partials          ?      732

[gerhardol]

When I change repo, and this gets called, _submoduleStatusProvider.HasSubmodulesStatusChanged(status) returns false. Not sure when this would return true.

The submodule status updates when the commit-count reports that submodules are changed. If not, there is no need to update the status for submodules as they are not changed. (Changes to supermodules are ignored).

[amaiorano]

1. I'm sorry, I guess I don't understand. What is the "commit-count" in this context?

2. Also, are you saying we won't ever see status icons for super modules? What about recursive child modules? If that's the case, I don't want to lose this feature, especially since the submodule ROT shows all modules.

3. Finally, and most importantly, in my test, I never saw any status icons. This is probably related to question Default .gitignore excludes bebug directories instead of debug directories #1, but how do I trigger the icons to show now?

[vbjay]

[vbjay]

Commit count is how many commits did the submodule move.

[amaiorano]

Thanks @vbjay. I'm going to test this change again soon to better understand what I was seeing.

[amaiorano]

@gerhardol I just tried the latest, and now I see the status icons appear. Not sure what was happening before.

Anyway, I personally don't like that we don't get the status of all submodules, including parents. It feels inconsistent. For example, when I'm at the root module, I see the following:

But if I check out the nested Git.hub, which itself has a diff, I see this:

Now it looks like all my submodules are up to date. This feels weird and inconsistent, don't you think?

And if I check out the parent of Git.hub, we see all status updates, even for super modules:

I find this difficult to understand as a user. Indeed if I've got a module currently opened, and I make any modification, I'd like to see that the parent module is dirty in the submodule list. All of this, of course, will apply to the Submodule ROT.

[gerhardol]

I have seen the problem with status not always updated, not what the cause is.
Good discussion

Now it looks like all my submodules are up to date. This feels weird and inconsistent, don't you think?

Really similar if status is never updated. I suggest that the icon is changed so you can see if the status is updated or not for these scenarios. Also add a button for refresh of status.
The superproject submodules is in a separate section in the menu too.

The current implementation will calculate status when opening a repo (and refresh) but only trigger stus refresh when the commit-count reports changed submodules. So this PR would not be a major difference from v3.

The core problem is what to trigger recalculations on superprojects from.

I assume the submodule structure will be separate from the status, i.e. done separately and that we want status (configurable off).
A summary of the options as I see them (please add):

• (this PR) Triggered on commit-count and updating all submodules (when there are reported changes)
• (next step) Triggered on commit-count and only updating submodules (with reported changes)
• (similar to v2) Refresh status for all submodules every time Browse form is activated (clicked in)
• Git-status for superproject (extra run) and trigger recalculation at all changes (if the load is lower than for previous).
• Supermodule status is updated when opening the repo (as done right now)
  • never updated 'automatically'. Maybe a new status icon to reflect the status is not updated?
  • Update all supermodules on any change to submodules (behavior in current master)
• Do not show super modules status at all, use another icon (I know @amaiorano do not want that, need to mention). Go to super should still be there

[amaiorano]

• (this PR) Triggered on commit-count and updating all submodules (when there are reported changes)
• (next step) Triggered on commit-count and only updating submodules (with reported changes)

I don't think either of the two options above are great. The first because of the reason I specified above - that it's weird to see parent module status only when a current module's submodule status has changed. The second because it means we'd never see parent module status changed by default. Adding a special icon to show that their status is unknown seems weird to me. And adding a extra button to force update these status just adds more friction.

• (similar to v2) Refresh status for all submodules every time Browse form is activated

Just to be sure, would this option only refresh submodule status when form is activated (upon opening a module)? It would not update status if other changes are detected?

I think another option would be:

• Add a setting like "Show super submodules", which when active, will display super modules in the menu and update their status like v2; otherwise, we only display the current module's submodules and update their status. This means to go to super projects, you would need to press the "go to super project" button (or select that option in the menu), which is probably fine for projects with lots of submodules that don't want to pay the cost of seeing them all with status updates. This would also impact how I display submodules in ROT, of course.

I'm adding the above option, but I personally don't want it. My opinion is to leave things as they are. If we're going to optimize updating all submodules, it needs to be done in a seamless way -- the user shouldn't have to press any extra buttons, or see "unknown status" icons. I think the right solution will have something to do with scheduling tasks with priorities, or perhaps for now, introducing an artificial delay on the status updates of super modules.

[gerhardol]

Just to be sure, would this option only refresh submodule status when form is activated (upon opening a module)? It would not update status if other changes are detected?

In 2.x, the submodules are reread every time the Browse form becomes active. Click on another window, click back on Browse and the structure/status is refreshed.
The old threading implementation was less resource intensive it seems. I was very bothered with that the dropdown was not available when switching back to Browse in 2.x.

Add a setting like "Show super submodules", which when active, will display super modules in the menu and update their status like v2

Every time the Browse window is activated then?
This can be pruned to just supermodules and assuming that the superrepo is open most of the time, it is not that major performance issue.

[amaiorano]

In 2.x, the submodules are reread every time the Browse form becomes active. Click on another window, click back on Browse and the structure/status is refreshed.

Okay, then no, not like in v2, but like in the current version before your last commit. Basically, setting a dirty flag to update submodules when certain actions are performed. This works well in my opinion. And when it doesn't work, hitting F5/refresh to force it does the job.

Add a setting like "Show super submodules", which when active, will display super modules in the menu and update their status like v2

Every time the Browse window is activated then?
This can be pruned to just supermodules and assuming that the superrepo is open most of the time, it is not that major performance issue.

You're right, most of the time you're in the top-most module, so this option I put forth isn't worth it.

You didn't respond to the last paragraph I wrote, but I would like your thoughts, so here it is again:

My opinion is to leave things as they are. If we're going to optimize updating all submodules, it needs to be done in a seamless way -- the user shouldn't have to press any extra buttons, or see "unknown status" icons. I think the right solution will have something to do with scheduling tasks with priorities , or perhaps for now, introducing an artificial delay on the status updates of super modules. (this won't help, as you pointed out)

[gerhardol]

Okay, then no, not like in v2, but like in the current version before your last commit. Basically, setting a dirty flag to update submodules when certain actions are performed. This works well in my opinion. And when it doesn't work, hitting F5/refresh to force it does the job.

In v3 all status are refreshed at start. All status are updated when any submodule is updated. There is no trigger for supermodules changes only. This is incomplete. The v2 approach is the only I can think of that will be updating supermodules at changes. This could be documented too.

I added some other proposals to the list.

You didn't respond to the last paragraph I wrote

I considered that answered in the other paragraph.
Of course, information should be consistent. But without any reliable trigger for supermodules it will require constant polling or that the data is potentially out of date.

[amaiorano]

Of course, information should be consistent. But without any reliable trigger for supermodules it will require constant polling or that the data is potentially out of date.

Right. So I don't know if it's really that bad that we don't detect super module changes automatically, as long as F5/Refresh does it. This is similar in spirit to how you can make repo changes outside of GE, and it will only show once you hit F5/Refresh. Thoughts?

• Supermodule status is updated when opening the repo (as done right now)
  • never updated 'automatically'. Maybe a new status icon to reflect the status is not updated?
  • Update all supermodules on any change to submodules (behavior in current master)

Should that second point be "Update all supermodules on any change to submodules or upon refreshing explicitly (behavior in current master) ?

[gerhardol]

Updated to refresh status immediately when there are supermodules

Will clean up and remove wip in some days

[amaiorano]

Updated to refresh status immediately when there are supermodules

This is a great idea @gerhardol! We get the fast performance when we're in the top-level module, and the supermodule updates otherwise. There is a bug, though, that I found while testing:

1. Open a submodule
2. Make any modification (create a new file)
3. The status monitor will see the change, and the submodules status will get updated showing dirty modules (great!)
4. Now undo the modification
5. The status monitor will see the change, but will not update the submodule status, so the submodules will still look dirty

The reason this happens is this code in FormBrowse in the GitWorkingDirectoryStatusChanged handler:

                    if (AppSettings.ShowSubmoduleStatus && _submoduleStatusProvider.HasSubmodulesStatusChanged(status))
                    {
                        UpdateSubmodulesStructure(updateStatus: true);
                    }

When we go from dirty back to clean, this code won't call UpdateSubmoduleStructure because 'status' will be empty. I think the right fix is changing it to something like this:

                    bool updateStatus = AppSettings.ShowSubmoduleStatus && _submoduleStatusProvider.HasSubmodulesStatusChanged(status);
                    UpdateSubmodulesStructure(updateStatus: updateStatus);

This way, we always call UpdateSubmoduleStructure with either 'true' if there are submodule files to update, or false otherwise. This will make sure to update the status icons all the time.

Edit: incidentally, this might also fix the bug of there being no submodules at all in the submodules menu on startup sometimes. I saw it happen the first time I launched with your latest changes.

[gerhardol]

When we go from dirty back to clean, this code won't call UpdateSubmoduleStructure because 'status' will be empty.

Thanks
Will add a new method to detect this without needing to reinit the submodule structure, something like:

_submoduleStatusProvider.HasSubmodulesStatusCleanToDirty(status)

Edit: incidentally, this might also fix the bug of there being no submodules at all in the submodules menu on startup sometimes.

That should not make any change, if something make it worse. The occasional "no submodules" (very often in 2.x) is probably occurring when the structure is not updated or being refreshed. Splitting structure and status should improve this further (but the main reason for a split is performance).

[gerhardol]

The remaining problem I know of is that the submodule list is not always updated after starting GE

[amaiorano]

The remaining problem I know of is that the submodule list is not always updated after starting GE

My educated guess is that this is related to CancellationException being thrown, and code in an async handler using a cancelled token, similar to the problem I fixed recently with RoT remaining blank. One thing I think I noticed last time is that on startup, there are two calls to UpdateSubmoduleStructure very close together, so the second would cancel the first.

[amaiorano]

I just ran your latest branch, and ran with "browse ", and I got the load with no submodules in the menu. Below is the partial output in VS. As you can see, there's a bunch of System.OperationCanceledException that got thrown at the end. I suspect this to be the problem:

...
=== Pre-bind state information ===
LOG: DisplayName = Microsoft.TeamFoundation.VersionControl.Client, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
 (Fully-specified)
LOG: Appbase = file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/
LOG: Initial PrivatePath = NULL
Calling assembly : Microsoft.TeamFoundation.Build.Client, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a.
===
LOG: This bind starts in default load context.
LOG: Using application configuration file: E:\DATA\code\active\gitextensions\GitExtensions\bin\Debug\GitExtensions.exe.Config
LOG: Using host configuration file: 
LOG: Using machine configuration file from C:\Windows\Microsoft.NET\Framework64\v4.0.30319\config\machine.config.
LOG: Post-policy reference: Microsoft.TeamFoundation.VersionControl.Client, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
LOG: The same bind was seen before, and was failed with hr = 0x80070002.

System.IO.FileLoadException: Could not load file or assembly 'Microsoft.TeamFoundation.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
File name: 'Microsoft.TeamFoundation.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

=== Pre-bind state information ===
LOG: DisplayName = Microsoft.TeamFoundation.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
 (Fully-specified)
LOG: Appbase = file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/
LOG: Initial PrivatePath = NULL
Calling assembly : Microsoft.TeamFoundation.Build.Client, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a.
===
LOG: This bind starts in default load context.
LOG: Using application configuration file: E:\DATA\code\active\gitextensions\GitExtensions\bin\Debug\GitExtensions.exe.Config
LOG: Using host configuration file: 
LOG: Using machine configuration file from C:\Windows\Microsoft.NET\Framework64\v4.0.30319\config\machine.config.
LOG: Post-policy reference: Microsoft.TeamFoundation.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/Microsoft.TeamFoundation.Common.DLL.
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/Microsoft.TeamFoundation.Common/Microsoft.TeamFoundation.Common.DLL.
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/plugins/Microsoft.TeamFoundation.Common.DLL.
WRN: Comparing the assembly name resulted in the mismatch: Major Version
ERR: Failed to complete setup of assembly (hr = 0x80131040). Probing terminated.

Exception thrown: 'System.Reflection.ReflectionTypeLoadException' in mscorlib.dll
GitExtensions.exe Error: 0 : Failed to load type when getting exports: System.IO.FileLoadException: Could not load file or assembly 'Microsoft.VisualStudio.Services.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
File name: 'Microsoft.VisualStudio.Services.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

=== Pre-bind state information ===
LOG: DisplayName = Microsoft.VisualStudio.Services.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
 (Fully-specified)
LOG: Appbase = file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/
LOG: Initial PrivatePath = NULL
Calling assembly : Microsoft.TeamFoundation.Client, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a.
===
LOG: This bind starts in default load context.
LOG: Using application configuration file: E:\DATA\code\active\gitextensions\GitExtensions\bin\Debug\GitExtensions.exe.Config
LOG: Using host configuration file: 
LOG: Using machine configuration file from C:\Windows\Microsoft.NET\Framework64\v4.0.30319\config\machine.config.
LOG: Post-policy reference: Microsoft.VisualStudio.Services.Common, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/Microsoft.VisualStudio.Services.Common.DLL.
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/Microsoft.VisualStudio.Services.Common/Microsoft.VisualStudio.Services.Common.DLL.
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/plugins/Microsoft.VisualStudio.Services.Common.DLL.
WRN: Comparing the assembly name resulted in the mismatch: Major Version
ERR: Failed to complete setup of assembly (hr = 0x80131040). Probing terminated.

Exception thrown: 'System.Reflection.ReflectionTypeLoadException' in mscorlib.dll
GitExtensions.exe Error: 0 : Failed to load type when getting exports: System.IO.FileLoadException: Could not load file or assembly 'RestSharp, Version=105.0.1.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
File name: 'RestSharp, Version=105.0.1.0, Culture=neutral, PublicKeyToken=null'

=== Pre-bind state information ===
LOG: DisplayName = RestSharp, Version=105.0.1.0, Culture=neutral, PublicKeyToken=null
 (Fully-specified)
LOG: Appbase = file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/
LOG: Initial PrivatePath = NULL
Calling assembly : Stash, Version=2.49.0.0, Culture=neutral, PublicKeyToken=null.
===
LOG: This bind starts in default load context.
LOG: Using application configuration file: E:\DATA\code\active\gitextensions\GitExtensions\bin\Debug\GitExtensions.exe.Config
LOG: Using host configuration file: 
LOG: Using machine configuration file from C:\Windows\Microsoft.NET\Framework64\v4.0.30319\config\machine.config.
LOG: Policy not being applied to reference at this time (private, custom, partial, or location-based assembly bind).
LOG: Attempting download of new URL file:///E:/DATA/code/active/gitextensions/GitExtensions/bin/Debug/RestSharp.DLL.
WRN: Comparing the assembly name resulted in the mismatch: PUBLIC KEY TOKEN
ERR: Failed to complete setup of assembly (hr = 0x80131040). Probing terminated.

System.TypeLoadException: Method 'Execute' in type 'Stash.StashPlugin' from assembly 'Stash, Version=2.49.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
Exception thrown: 'System.OperationCanceledException' in mscorlib.dll
'GitExtensions.exe' (CLR v4.0.30319: GitExtensions.exe): Loaded 'C:\WINDOWS\Microsoft.Net\assembly\GAC_MSIL\System.Numerics\v4.0_4.0.0.0__b77a5c561934e089\System.Numerics.dll'. Skipped loading symbols. Module is optimized and the debugger option 'Just My Code' is enabled.
The thread 0x1060 has exited with code 0 (0x0).

[amaiorano]

Okay, did a bit more debugging, since I'm able to repro the problem of no submodules showing up right now. The problem is that on launch, and opening the first repo, we never get a call to UpdateSubmodulesStructure(). I think this boils down to the fact that the code in UICommands_PostRepositoryChanged doesn't run the first time we launch and browse a repo; only if we change the repo afterwards. Note that this also means UpdateStashCount() isn't run on launch.

Eplicitly invoking UICommands_PostRepositoryChanged from the FormBrowse does work, but we probably want to call a function "OnPostRepositoryChanged" from both places instead:

            InitializeComplete();
            RestorePosition();
            UICommands_PostRepositoryChanged(this, null);

            return;

Edit: Ah, and I can explain why it works sometimes on launch. If you have at least one submodule change, then when it executes the GitWorkingDirectoryStatusChanged callback on launch, and you have ShowSubmoduleStatus enabled, then this code will end up calling UpdateSubmoduleStructure:

                        if (AppSettings.ShowSubmoduleStatus)
                        {
                            if (_submoduleStatusProvider.HasChangedToNone(status))
                            {
                                UpdateSubmodulesStructure(updateStatus: false);
                            }
                            else if (_submoduleStatusProvider.HasStatusChanges(status))
                            {
                                UpdateSubmodulesStructure(updateStatus: true);
                            }
                        }

However, if ShowSubmoduleStatus is false, or you have no submodule changes whatsoever, then UpdateSubmoduleStructure never gets called on launch, and we end up with no submodules in the menu.

Edit 2: Yeah, so I guess if we explicitly call UpdateSubmodulesStructure in the FormBrowse constructor, we will end potentially calling it twice in succession (second time in UICommands_PostRepositoryChanged callback), which means the first one will be cancelled, etc. Maybe in the UICommands_PostRepositoryChanged callback, we want to make sure to call UpdateSubmoduleStructure if it has never been called before?

[gerhardol]

Eplicitly invoking UICommands_PostRepositoryChanged from the FormBrowse does work, but we probably want to call a function "OnPostRepositoryChanged" from both places instead:

Should be the solution
Thanks

[amaiorano]

Should be the solution
Thanks

No problem. Just want to make sure you saw my 2 edits to my comment, especially the second one, where I mention that adding this call means we'll end up cancelling it if we have submodule changes and status update enabled...

[RussKie]

Is this something we need for v3?

…

On Fri, Nov 30, 2018, 1:38 AM Antonio Maiorano ***@***.***> wrote: Should be the solution Thanks No problem. Just want to make sure you saw my 2 edits to my comment, especially the second one, where I mention that adding this call means we'll end up cancelling it if we have submodule changes and status update enabled... — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5777 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXosNbFyJqYPfS9kcH_VNetrRXrBoks5uz_F4gaJpZM4Yezd-> .

[gerhardol]

Is this something we need for v3?

No

Further decrease the load when opening a repo for next release after 3.0.
I also delayed #5747 as some of the the details I investigated in this PR was not ready before the release branch was created.

[gerhardol]

Updated and squashed. (If someone is interested in the changes to fix the last issue discussed, you can look at bugfix/submodule-performance-test-3 in my clone).

The CodeFactor comment is on FormBrowse() constructor, I do not want to rewrite that now...