-
Notifications
You must be signed in to change notification settings - Fork 2
Git command line changes for WSL #3893
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
|
@@ -42,18 +42,25 @@ public static GitCommandRunnerResultInfo ExecuteGitCommand(string gitApplication | |||||
|
||||||
// Add timeout for 1 minute | ||||||
process.WaitForExit(TimeSpan.FromMinutes(1)); | ||||||
|
||||||
if (process.ExitCode != 0) | ||||||
{ | ||||||
Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode); | ||||||
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. Nit:
Suggested change
|
||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, output, "Execute Git process exited unsuccessfully", string.Empty, null, arguments, process.ExitCode); | ||||||
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. NIT: |
||||||
} | ||||||
|
||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Success, output); | ||||||
} | ||||||
else | ||||||
{ | ||||||
Log.Error("Failed to start the Git process: process is null"); | ||||||
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. NIT: remove |
||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, "Git process is null", string.Empty, new InvalidOperationException("Failed to start the Git process: process is null"), null); | ||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, null, "Git process is null", string.Empty, new InvalidOperationException("Failed to start the Git process: process is null"), null, null); | ||||||
} | ||||||
} | ||||||
catch (Exception ex) | ||||||
{ | ||||||
Log.Error(ex, "Failed to invoke Git with arguments: {Argument}", arguments); | ||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, "Failed to invoke Git with arguments", string.Empty, ex, arguments); | ||||||
return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, null, "Failed to invoke Git with arguments", string.Empty, ex, arguments, null); | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ | |
|
||
using System.Runtime.InteropServices; | ||
using DevHome.Common.Services; | ||
using LibGit2Sharp; | ||
using Microsoft.Windows.DevHome.SDK; | ||
using Serilog; | ||
|
||
|
@@ -35,14 +34,14 @@ GetLocalRepositoryResult ILocalRepositoryProvider.GetRepository(string rootPath) | |
{ | ||
return new GetLocalRepositoryResult(new GitLocalRepository(rootPath, _repositoryCache)); | ||
} | ||
catch (RepositoryNotFoundException libGitEx) | ||
catch (ArgumentException ex) | ||
{ | ||
_log.Error("GitLocalRepositoryProviderFactory", "Failed to create GitLocalRepository", libGitEx); | ||
return new GetLocalRepositoryResult(libGitEx, _stringResource.GetLocalized("RepositoryNotFound"), $"Message: {libGitEx.Message} and HRESULT: {libGitEx.HResult}"); | ||
_log.Error(ex, "GitLocalRepositoryProviderFactory: Failed to create GitLocalRepository"); | ||
return new GetLocalRepositoryResult(ex, _stringResource.GetLocalized("RepositoryNotFound"), $"Message: {ex.Message} and HRESULT: {ex.HResult}"); | ||
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. Nit: Maybe this needs to be changed? Do we still tell the user "Repo not found"? 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.
|
||
} | ||
catch (Exception ex) | ||
{ | ||
_log.Error("GitLocalRepositoryProviderFactory", "Failed to create GitLocalRepository", ex); | ||
_log.Error(ex, "GitLocalRepositoryProviderFactory: Failed to create GitLocalRepository"); | ||
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. NIT: |
||
if (ex.Message.Contains("not owned by current user") || ex.Message.Contains("detected dubious ownership in repository")) | ||
{ | ||
return new GetLocalRepositoryResult(ex, _stringResource.GetLocalized("RepositoryNotOwnedByCurrentUser"), $"Message: {ex.Message} and HRESULT: {ex.HResult}"); | ||
|
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.
With so many nullable parameters can you split the constructor into multiple constructors so callers can more easily use this object.
I don't like calling constructors with multiple nullable parameters because the constructor does not tell me what combination of values ill throw an exception.
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.
Consider adding more constructors instead of more nullable parameters.