Refactor repository models (repository refactor part 1) by jcansdale · Pull Request #2008 · github/VisualStudio

jcansdale · 2018-10-23T11:32:37Z

This is the first iteration of refactoring LocalRepositoryModel and GitService (see #2007).

I've focused on moving the code to where is should be, with as few functional changes as possible. There is more to do, but I think this makes sense as a chunk.

Replace ILocalRepositoryModel.CurrentBranch property with IGitService.CreateCurrentBranchModel(model) method
Move construction of LocalRepositoryModel to GitService.CreateLocalRepositoryModel
Move LocalRepositoryModel.GenerateUrl to LinkCommandBase
Move LocalReposotoryModel.Refresh to GitService.RefreshCloneUrl (this will be removed in Remove repository responsibilities from TeamExplorerServiceHolder (repository refactor part 2) #2025)
Get rid of LocalRepositoryModelFactory (we can use GitService instead)
Remove unused LocalRepositoryModel.HeadSha property

The most significant functional change was to replace the LocalRepositoryModel.CurrentBranch property with a GitService.CreateCurrentBranchModel(repositoryModel) method.

This was necessary because the IGitExt.ActiveRepositories property changed event fires before the current branch is actually changed. If we populate LocalRepositoryModel.CurrentBranch when the model is created, CurrentBranch ends up with information about the previous branch.

By deferring creation of the current branch model, we can populate it when the branch has actually changed and only create the branch model when it's actually required (the property change event can fire a lot).

Bugs to fix

The ActiveRepositoriesChanged event fires before the branch has changed on the local repository
VisualStudio/src/GitHub.Exports/Services/GitService.cs
Line 52 in 47430fd
RefreshCurrentBranch(model);
Tuples aren't comparable so this was always coming back false
VisualStudio/src/GitHub.App/Services/TeamExplorerContext.cs
Line 146 in 47430fd
elseif(newPullRequest!=pullRequest)

Still to do

Now the junk has been removed from LocalReposotoryModel, we can decide how we want the models to look!

Where possible use ITeamExploerContext instead of ITeamExplorerServiceHolder (done in Remove repository responsibilities from TeamExplorerServiceHolder (repository refactor part 2) #2025)
Refactor BranchModel to make it an actual model
Change RemoteRepositoryModel.OwnerAccount to be a ActorModel instead of an IAccount (@grokys)
Remove the model interfaces
Remove the Octicon from the model Refactor repository models (repository refactor part 1) #2008 (comment)
BranchModel is being passed a repository constructed from a URL rather then a local path
VisualStudio/src/GitHub.Exports/Models/BranchModel.cs
Line 36 in e3f8114
Repository=branch.IsRemote?gitService.CreateLocalRepositoryModel(branch.Remote.Url):repo;
LocalRepositoryModel.GetHashCode and Equals should use consistent comparisons
VisualStudio/src/GitHub.Exports/Models/LocalRepositoryModel.cs
Line 36 in e3f8114
return17*23+(Name?.GetHashCode()??0)*23+(Owner?.GetHashCode()??0)*23+(LocalPath?.TrimEnd('\\').ToUpperInvariant().GetHashCode()??0);

VisualStudio/src/GitHub.Exports/Models/LocalRepositoryModel.cs
Line 55 in e3f8114
string.Equals(LocalPath?.TrimEnd('\\'),other.LocalPath?.TrimEnd('\\'),StringComparison.CurrentCultureIgnoreCase);

OrdinalIgnoreCase or InvariantCultureIgnoreCase maybe?

Refactor the constructor that took IGitService into LocalRepositoryModel where it was used.

Move GenerateUrl from LocalRepositoryModel to LinkCommandBase.

codecov · 2018-10-23T13:40:05Z

Codecov Report

Merging #2008 into master will decrease coverage by 1.3%.
The diff coverage is 52.63%.

@@ Coverage Diff @@## master #2008 +/- ##
==========================================
- Coverage 40.67% 39.36% -1.31% 
==========================================
Files 377 410 +33 Lines 16333 17572 +1239 Branches 2253 2422 +169 ==========================================
+ Hits 6643 6918 +275 - Misses 9150 10117 +967 + Partials 540 537 -3

Impacted Files	Coverage Δ
src/GitHub.Exports/Models/BranchModel.cs	`35.71% <0%> (ø)`	⬆️
...rc/GitHub.VisualStudio/Commands/OpenLinkCommand.cs	`0% <0%> (ø)`
...rc/GitHub.VisualStudio/Commands/CopyLinkCommand.cs	`0% <0%> (ø)`
...c/GitHub.VisualStudio/Commands/BlameLinkCommand.cs	`0% <0%> (ø)`
src/GitHub.App/Services/PullRequestService.cs	`35.29% <0%> (ø)`	⬆️
...eamFoundation.14/Base/TeamExplorerServiceHolder.cs	`0% <0%> (ø)`	⬆️
src/GitHub.TeamFoundation.14/RegistryHelper.cs	`0% <0%> (ø)`	⬆️
...ndation.14/Services/LocalRepositoryModelFactory.cs	`0% <0%> (ø)`	⬆️
...wModels/GitHubPane/PullRequestCreationViewModel.cs	`97.35% <100%> (+0.03%)`	⬆️
...nlineReviews/Services/PullRequestSessionManager.cs	`83.75% <100%> (ø)`	⬆️
... and 42 more

Move LocalRepositoryModel construction tests to new home.

This property wasn't being used.

We Now only need the CreateLocalRepositoryModel(localPath) overload.

Previously the current branch was being read when CurrentBranch was fetched. This changes it to be read when the LocalRepositoryModel is created.

Remove redundant code and usings.

jcansdale · 2018-10-24T11:41:05Z

I think updating the Checkout ... link state might have broken with this PR. Need to investigate....

grokys · 2018-10-25T09:01:33Z

 /// <summary>
 /// Gets the repository clone URL.
 /// </summary>
 public UriString CloneUrl


Future: should these properties be raising change notifications? If so, should other properties also be raising them (CurrentBranch for example).

I think we should aim to get rid of the Refresh command so this model would be immutable after creation.

grokys · 2018-10-25T09:02:31Z

@@ -85,7 +67,7 @@ protected set
 public Octicon Icon


Future: move this out of the model: it's a view model-level reponsibility that only really applies to the repository list in Team Explorer.

grokys · 2018-10-25T09:05:52Z

 {
 var repo = ServiceProvider.TryGetService<IGitService>().GetRepository(path);
- return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'), GitService.GitServiceHelper);
+ return GitService.GitServiceHelper.CreateLocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'));


Is TrimEnd needed here? If so, should that logic be put into CreateLocalRepositoryModel?

Previously CurrentBranch was created as the property was read. We now need a way to refresh it.

jcansdale

Should this logic be part of RepositoryViewModel?

jcansdale

Should this logic be part of RepositoryViewModel?

jcansdale

Should this logic be part of RepositoryViewModel?

It appears VSGitExt.ActiveRepositoriesChanged is fired before the local repository has actually changed its branch. This means we can't read the branch information immediately!

jcansdale

Comments about removing CurrentBranch.

jcansdale · 2018-10-31T09:40:29Z

+ Name = name,
+ Icon = Octicon.repo
+ };
+


This is currently being refreshed before the branch has actually changed. :-(

There is now a CreateCurrentBranchModel command instead of the CurrentBranch property. We'll still need to be careful about calling it too soon.

Remove the ILocalRepositoryModel.CurrentBranch property and explicitly call IGitService.CreateCurrentBranchModel instead. Fix all the broken tests.

Convert GetPullRequestForCurrentBranch to return (string owner, int number). This allows the tuple to be compared directly.

We can now use GitService as a LocalRepositoryMode factory.

Be explicit about what it does.

grokys

LGTM! a couple of nits and questions but going to approve anyway.

grokys · 2018-11-02T09:08:39Z

+ /// Creates a new branch model for the current branch.
+ /// </summary>
+ /// <param name="model">The <see cref="ILocalRepositoryModel" /> to create a current branch model for.</param>
+ /// <returns>A new branch model.</returns>


I think this might be less verbose if it were simply called GetBranch: I'd tend to think of it more as a method to get the state of an ILocalRepositoryModel than a factory method. What do you think?

We could change it to GetBranch(string name = "HEAD"). Do you think that would make sense? Otherwise we'd probably need to qualify it with GetHeadBranch or something?

Yeah I think that would make sense.

grokys · 2018-11-02T09:20:02Z

+ else if (newRepositoryPath != null)
 {
- log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository");
+ log.Debug("Fire StatusChanged event when on a repository and anything changes");


Reads a bit yoda-y ;)

I'm not sure if that's a feature? 😉
I'll change it to:
"Fire StatusChanged event if anything about an active repository has changed"

grokys · 2018-11-02T10:00:32Z

- log.Debug("Fire StatusChanged event when BranchName changes for ActiveRepository");
- StatusChanged?.Invoke(this, EventArgs.Empty);
- }
- else if (newHeadSha != headSha)


So StatusChanged is no longer firing when HEAD changes? Is this because it wasn't working before anyway? (i.e. it was firing before the actual change)

Now either a repository change event will be fired when we set ActiveRepository or a StatusChanged will be fired if we think anything about the active repository might have changed (including when the HEAD changed).
This is actually similar to what was happening before because newPullRequest != pullRequest was always returning true (the tuple comparison issue).
I have changed it so that if we're not on a repository, no StatusChanged event will fire.

jcansdale · 2018-11-08T14:06:52Z

Merged as part of #2028.

jcansdale added 5 commits October 23, 2018 09:30

Remove IGitService dependency from RepositoryModel
80abca2
Refactor the constructor that took IGitService into LocalRepositoryModel where it was used.

Move LocalRepositoryModel ctor into GitService 1/2
1177e93

Move LocalRepositoryModel ctor into GitService 2/2
c6c8b84

Fix tests after LocalRepositoryModel refactor
edb8232

Move GenerateUrl to LinkCommandBase
835e561
Move GenerateUrl from LocalRepositoryModel to LinkCommandBase.

jcansdale added 6 commits October 23, 2018 15:12

Convert LocalRepositoryModel to GitService tests
fe2dcf8
Move LocalRepositoryModel construction tests to new home.

Move LocalReposotoryModel.Refresh to GitService
683ad50

Remove LocalRepositoryModel.HeadSha property
59e5652
This property wasn't being used.

Remove CreateLocalRepositoryModel overload
01d2e46
We Now only need the CreateLocalRepositoryModel(localPath) overload.

Read the current branch when model is created
7bab478
Previously the current branch was being read when CurrentBranch was fetched. This changes it to be read when the LocalRepositoryModel is created.

Clean up LocalRepositoryModel
e3f8114
Remove redundant code and usings.

jcansdale changed the title ~~[wip] Refactor repository models~~Refactor repository models (part 1)Oct 24, 2018

jcansdale changed the title ~~Refactor repository models (part 1)~~[wip] Refactor repository models (part 1)Oct 24, 2018

grokys reviewed Oct 25, 2018
View reviewed changes

jcansdale added 2 commits October 30, 2018 13:20

ClonePath -> CloneUri
2f13d62

Make GitService.Refresh the CurrentBranch
c61d0f2
Previously CurrentBranch was created as the property was read. We now need a way to refresh it.

jcansdale commented Oct 30, 2018
View reviewed changes

jcansdale added 2 commits October 30, 2018 14:09

Merge branch 'master' into fixes/2007-refactor-repository-models
473fa48

Add workaround for out of date CurrentBranch issue
47430fd
It appears VSGitExt.ActiveRepositoriesChanged is fired before the local repository has actually changed its branch. This means we can't read the branch information immediately!

jcansdale commented Oct 31, 2018
View reviewed changes

jcansdale added 5 commits October 31, 2018 17:26

Refactor to IGitService.CreateCurrentBranchModel
713d9c4
Remove the ILocalRepositoryModel.CurrentBranch property and explicitly call IGitService.CreateCurrentBranchModel instead. Fix all the broken tests.

Make GetPullRequestForCurrentBranch return (tuple)
0f59f7e
Convert GetPullRequestForCurrentBranch to return (string owner, int number). This allows the tuple to be compared directly.

Avoid firing StatusChanged if we're not on a repo
42d09fa

Suppress "Do not directly await a Task" warning
a47af2b

Get rid of LocalRepositoryModelFactory
db41ef9
We can now use GitService as a LocalRepositoryMode factory.

jcansdale force-pushed the fixes/2007-refactor-repository-models branch from 3abf4d6 to db41ef9Compare October 31, 2018 17:30

jcansdale added 2 commits October 31, 2018 17:59

Merge branch 'master' into fixes/2007-refactor-repository-models
06d17e0

Rename IGitService.Refresh to RefreshCloneUrl
a4c8386
Be explicit about what it does.

jcansdale changed the title ~~[wip] Refactor repository models (part 1)~~Refactor repository models (part 1)Nov 1, 2018

jcansdale added the Ready for Review label Nov 1, 2018

jcansdale mentioned this pull request Nov 1, 2018
Remove repository responsibilities from TeamExplorerServiceHolder (repository refactor part 2) #2025
Closed

jcansdale changed the title ~~Refactor repository models (part 1)~~Refactor repository models (repository refactor part 1)Nov 1, 2018

grokys approved these changes Nov 2, 2018
View reviewed changes

This was referenced Nov 2, 2018
Repository model refactoring (part 3) #2028
Merged
[wip] Refactor repository model (part 4) #2040
Merged

Merge branch 'master' into fixes/2007-refactor-repository-models
2ce3e84

jcansdale closed this Nov 8, 2018

jcansdale mentioned this pull request Nov 12, 2018
Repository model refactoring still to do #2050
Open
7 tasks

meaghanlewis mentioned this pull request Nov 13, 2018
Development Sprint: December 10th release #2052
Closed

Uh oh!

Conversation

jcansdale commented Oct 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugs to fix

Still to do

Uh oh!

codecovBot commented Oct 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jcansdale commented Oct 24, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdaleNov 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Nov 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcansdale commented Oct 23, 2018•
edited
Loading

codecovBot commented Oct 23, 2018•
edited
Loading

jcansdaleNov 2, 2018•
edited
Loading