Refactor repository models (repository refactor part 1) #2008

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

else if (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

return 17 * 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....

src/GitHub.Exports/Services/IGitService.cs

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

src/GitHub.Exports/Models/RepositoryModel.cs

        /// <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

src/GitHub.Exports/Models/RepositoryModel.cs

        /// <summary>
        /// Gets an icon for the repository that displays its private and fork state.
        /// </summary>
        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.

src/GitHub.Exports/Models/BranchModel.cs

src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs

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

src/GitHub.VisualStudio/Commands/LinkCommandBase.cs

                {
                    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.

src/GitHub.Exports/Models/ILocalRepositoryModel.cs

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

src/GitHub.Exports/Services/GitService.cs

+                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

src/GitHub.Exports/Services/IGitService.cs

+        /// 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

src/GitHub.App/Services/TeamExplorerContext.cs

+                    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

src/GitHub.App/Services/TeamExplorerContext.cs

-                        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 db41ef9 Compare 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

Refactor repository models (repository refactor part 1) #2008

Refactor repository models (repository refactor part 1) #2008

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!

codecov bot 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!

jcansdale Nov 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!

3 participants

jcansdale commented Oct 23, 2018 •

edited

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading

jcansdale Nov 2, 2018 •

edited

Loading