-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Unified Clone / Open from GitHub UI #1919
Changes from all commits
b2f0679
5e955d6
f2ed1fd
732f9d2
ba3bdfa
1726986
5295642
f8d0c7b
f10bf85
b82ac27
43fd57d
2aa841d
a47cc29
f13312e
9ec30fb
efcf29d
b361468
fff3255
bb181eb
fdca558
95f8a01
b147324
bde9863
698196c
c37522b
9cd6c4c
451a5b0
d91c9ab
af55f6a
8067273
9f8757a
3172217
527b1d1
3d2b580
09347ae
43ba772
6de1bbb
f1ba0d2
7344458
d789c4e
27e13d2
43162db
5f2012a
cfadc62
ca1a215
37e9acf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -34,6 +34,7 @@ public class RepositoryCloneService : IRepositoryCloneService | |||
readonly IOperatingSystem operatingSystem; | ||||
readonly string defaultClonePath; | ||||
readonly IVSGitServices vsGitServices; | ||||
readonly ITeamExplorerServices teamExplorerServices; | ||||
readonly IGraphQLClientFactory graphqlFactory; | ||||
readonly IUsageTracker usageTracker; | ||||
ICompiledQuery<ViewerRepositoriesModel> readViewerRepositories; | ||||
|
@@ -42,11 +43,13 @@ public class RepositoryCloneService : IRepositoryCloneService | |||
public RepositoryCloneService( | ||||
IOperatingSystem operatingSystem, | ||||
IVSGitServices vsGitServices, | ||||
ITeamExplorerServices teamExplorerServices, | ||||
IGraphQLClientFactory graphqlFactory, | ||||
IUsageTracker usageTracker) | ||||
{ | ||||
this.operatingSystem = operatingSystem; | ||||
this.vsGitServices = vsGitServices; | ||||
this.teamExplorerServices = teamExplorerServices; | ||||
this.graphqlFactory = graphqlFactory; | ||||
this.usageTracker = usageTracker; | ||||
|
||||
|
@@ -103,6 +106,54 @@ public async Task<ViewerRepositoriesModel> ReadViewerRepositories(HostAddress ad | |||
return result; | ||||
} | ||||
|
||||
/// <inheritdoc/> | ||||
public async Task CloneOrOpenRepository( | ||||
CloneDialogResult cloneDialogResult, | ||||
object progress = null) | ||||
{ | ||||
Guard.ArgumentNotNull(cloneDialogResult, nameof(cloneDialogResult)); | ||||
|
||||
var repositoryPath = cloneDialogResult.Path; | ||||
var url = cloneDialogResult.Url; | ||||
|
||||
if (DestinationFileExists(repositoryPath)) | ||||
{ | ||||
throw new InvalidOperationException("Can't clone or open a repository because a file exists at: " + repositoryPath); | ||||
} | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be checking whether the repository at this path has the same clone URL? I'm thinking of the case where e.g. the user wants to clone a fork and they already have the parent repository at this location. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! In fact there are a few scenarios we should be checking for.
I'll let you know when these checks land. |
||||
var repositoryUrl = url.ToRepositoryUrl(); | ||||
var isDotCom = HostAddress.IsGitHubDotComUri(repositoryUrl); | ||||
if (DestinationDirectoryExists(repositoryPath)) | ||||
{ | ||||
teamExplorerServices.OpenRepository(repositoryPath); | ||||
|
||||
if (isDotCom) | ||||
{ | ||||
await usageTracker.IncrementCounter(x => x.NumberOfGitHubOpens); | ||||
} | ||||
else | ||||
{ | ||||
await usageTracker.IncrementCounter(x => x.NumberOfEnterpriseOpens); | ||||
} | ||||
} | ||||
else | ||||
{ | ||||
var cloneUrl = repositoryUrl.ToString(); | ||||
await CloneRepository(cloneUrl, repositoryPath, progress).ConfigureAwait(true); | ||||
|
||||
if (isDotCom) | ||||
{ | ||||
await usageTracker.IncrementCounter(x => x.NumberOfGitHubClones); | ||||
} | ||||
else | ||||
{ | ||||
await usageTracker.IncrementCounter(x => x.NumberOfEnterpriseClones); | ||||
} | ||||
} | ||||
|
||||
teamExplorerServices.ShowHomePage(); | ||||
} | ||||
|
||||
/// <inheritdoc/> | ||||
public async Task CloneRepository( | ||||
string cloneUrl, | ||||
|
@@ -121,21 +172,8 @@ public async Task CloneRepository( | |||
try | ||||
{ | ||||
await vsGitServices.Clone(cloneUrl, repositoryPath, true, progress); | ||||
|
||||
await usageTracker.IncrementCounter(x => x.NumberOfClones); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is being called by
|
||||
|
||||
var repositoryUrl = new UriString(cloneUrl).ToRepositoryUrl(); | ||||
var isDotCom = HostAddress.IsGitHubDotComUri(repositoryUrl); | ||||
if (isDotCom) | ||||
{ | ||||
await usageTracker.IncrementCounter(x => x.NumberOfGitHubClones); | ||||
} | ||||
else | ||||
{ | ||||
// If it isn't a GitHub URL, assume it's an Enterprise URL | ||||
await usageTracker.IncrementCounter(x => x.NumberOfEnterpriseClones); | ||||
} | ||||
|
||||
if (repositoryPath.StartsWith(DefaultClonePath, StringComparison.OrdinalIgnoreCase)) | ||||
{ | ||||
// Count the number of times users clone into the Default Repository Location | ||||
|
@@ -150,7 +188,10 @@ public async Task CloneRepository( | |||
} | ||||
|
||||
/// <inheritdoc/> | ||||
public bool DestinationExists(string path) => Directory.Exists(path) || File.Exists(path); | ||||
public bool DestinationDirectoryExists(string path) => operatingSystem.Directory.DirectoryExists(path); | ||||
|
||||
/// <inheritdoc/> | ||||
public bool DestinationFileExists(string path) => operatingSystem.File.Exists(path); | ||||
|
||||
string GetLocalClonePathFromGitProvider(string fallbackPath) | ||||
{ | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ public enum ErrorType | |
CannotDropFolder, | ||
CannotDropFolderUnauthorizedAccess, | ||
ClipboardFailed, | ||
ClonedFailed, | ||
CloneOrOpenFailed, | ||
CloneFailedNotLoggedIn, | ||
CommitCreateFailed, | ||
CommitRevertFailed, | ||
|
@@ -123,7 +123,7 @@ public static class StandardUserErrors | |
}, | ||
{ ErrorType.ClipboardFailed, Map(Defaults("Failed to copy text to the clipboard.")) }, | ||
{ | ||
ErrorType.ClonedFailed, Map(Defaults("Failed to clone the repository '{0}'", "Email [email protected] if you continue to have problems."), | ||
ErrorType.CloneOrOpenFailed, Map(Defaults("Failed to clone or open the repository '{0}'", "Email [email protected] if you continue to have problems."), | ||
new[] | ||
{ | ||
new Translation(@"fatal: bad config file line (\d+) in (.+)", "Failed to clone the repository '{0}'", @"The config file '$2' is corrupted at line $1. You may need to open the file and try to fix any errors."), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether we need both
Clone
andCloneOrOpen
? Wouldn'tClone
andOpen
make more sense? The decision of which to call would typically be made at a higher level I would have thought as we're checking whether the repository already exists in order to show/hide theOpen
button.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this as discussed in Slack. There is now a
CloneOrOpenRepository(CloneDialogResult)
method which is indented to be used when the clone dialog returns a non-nullCloneDialogResult
.While updating this I discovered that the create repository functionality is also calling
CloneRepository
. I this means that for everyNumberOfReposCreated
incremented, theNumberOfClones
counter will be incremented as well. Oops! //cc @telliott27