Git command line changes for WSL #3893

ssparach · 2024-09-23T19:43:18Z

Summary of the pull request

This PR contains the last set of changes to allow file explorer integration of a WSL git repository. It does this by using WSL command line to obtain all property information from git.

References and relevant issues

#3829

Detailed description of the pull request / Additional comments

This PR:

Uses git rev-parse --show-toplevel to validate a git repository instead of LibGit2Sharp.Repository()
Uses git -no-optional-locks-branch --show-current to retrieve branch name, detached state
Adds --branch to RetrieveStatusInformation to obtain branch sha, ahead by value, behind by value and upstream branch info
Uses rev-parse HEAD to find head commit
Removes CommitWrapper(LibGit2Sharp.Commit commit) constructor and removes FindLastCommitUsingLibGit2Sharp from CommitLogCache

Note: This PR removes LibGit2Sharp usage where applicable and other unused library using statements. However, it does not completely clean usage of LibGit2Sharp library as the enum definitions from this library are being relied on such as FIleStatus etc. A separate bug has been created to track the complete removal of LibGit2Sharp from the git extension project (i.e. Create custom enum definitions for FileStatus, SubmoduleStatus to remove LibGit2Sharp dependency from git extension project · Issue #3894 · microsoft/devhome (github.com)). This improvement is not blocking to this PR.

Validation steps performed

Build of msix package
Unit Testing
Validation of functionality in VM

PR checklist

Closes #xxx
Tests added/passed
Documentation updated

extensions/GitExtension/FileExplorerGitIntegration/Models/StatusCache.cs

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepositoryProviderFactory.cs

dhoehna · 2024-09-23T23:21:16Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        }
+
+        var output = validateGitRootRepo.Output;
+        if (output is null || output.Contains("fatal: not a git repository"))


Can the string fatal: not a git repository be in different languages? Should this be localized?

Got curious. This seems like a complicated question.
https://github.com/git/git/blob/master/po/README.md#marking-strings-for-translation

In this case, it does not seem like the git output needs to be translated. The output is parsed to throw the right exception and eventually the error displayed to the user in Dev Home is localized.

Can this use the exit code of the process? I don't know if git.exe sets the exit code though.

Got curious. This seems like a complicated question. https://github.com/git/git/blob/master/po/README.md#marking-strings-for-translation

That's how the linux flavors of git do translation - I don't know all the details, but it seems to be more runtime-oriented and uses gettext in contrast to how we provide translations in Windows land. There might be some other stuff they use on Windows, but at current, it doesn't appear to be active in most cases: git-for-windows/git#724

Can this use the exit code of the process? I don't know if git.exe sets the exit code though.

I agree that parsing the output for this specific message feels somewhat brittle. We can most likely rely on git returning a non-zero exit code for failure, but it doesn't return any specific error codes to tell us what the problem was. Can we check the exit code of the process and also return a failure if it is non-zero?

Can we check the exit code of the process and also return a failure if it is non-zero?

Incorporated in latest changes. Thanks!

krschau · 2024-09-24T14:56:15Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        }
+
+        var output = validateGitRootRepo.Output;
+        if (output is null || output.Contains("fatal: not a git repository"))


Got curious. This seems like a complicated question.
https://github.com/git/git/blob/master/po/README.md#marking-strings-for-translation

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

vineeththomasalex · 2024-09-27T17:54:36Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepositoryProviderFactory.cs

-            _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}");


Nit: Maybe this needs to be changed? Do we still tell the user "Repo not found"?

Repository not found.

krschau · 2024-09-27T17:53:47Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitExecute.cs

+
+                if (process.ExitCode != 0)
+                {
+                    Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode);


Nit:

Suggested change

Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode);

Log.Error($"Execute Git process exited unsuccessfully with exit code {process.ExitCode}");

vineeththomasalex · 2024-09-27T18:01:27Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        if (WslIntegrator.IsWSLRepo(rootFolder))
+        {
+            var normalizedLinuxPath = WslIntegrator.GetNormalizedLinuxPath(rootFolder);
+            if (output.TrimEnd('\n') != normalizedLinuxPath)


Nit: Should we get Env.NewLine or do we want to hard code \n here?

I don't think Env.NewLine is correct because this is for WSL. WIndows and Linux have different line endings. I'm afraid that Env.NewLine is the windows definition of a line ending.

EDIT: Unless this only applies to file endings and not line endings.

dhoehna · 2024-09-27T18:34:09Z

extensions/GitExtension/FileExplorerGitIntegration/Helpers/GitCommandRunnerResultInfo.cs

    public GitCommandRunnerResultInfo(ProviderOperationStatus status, string? output)
    {
        Status = status;
        Output = output;
    }

-    public GitCommandRunnerResultInfo(ProviderOperationStatus status, string? displayMessage, string? diagnosticText, Exception? ex, string? args)
+    public GitCommandRunnerResultInfo(ProviderOperationStatus status, string? output, string? displayMessage, string? diagnosticText, Exception? ex, string? args, int? processExitCode)


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.

Consider adding more constructors instead of more nullable parameters.

dhoehna · 2024-09-27T18:35:38Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitExecute.cs

+                if (process.ExitCode != 0)
+                {
+                    Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode);
+                    return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, output, "Execute Git process exited unsuccessfully", string.Empty, null, arguments, process.ExitCode);


NIT: exited with a failure error code "Exit unsuccessfully" tells me that the process was unsuccessful in exiting.

dhoehna · 2024-09-27T18:40:54Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitExecute.cs

+                    Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode);
+                    return new GitCommandRunnerResultInfo(ProviderOperationStatus.Failure, output, "Execute Git process exited unsuccessfully", string.Empty, null, arguments, process.ExitCode);
+                }
+
                return new GitCommandRunnerResultInfo(ProviderOperationStatus.Success, output);
            }
            else
            {
                Log.Error("Failed to start the Git process: process is null");


NIT: remove process is null The definition of a null process is one that did not start.

dhoehna · 2024-09-27T18:42:11Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepositoryProviderFactory.cs

        }
        catch (Exception ex)
        {
-            _log.Error("GitLocalRepositoryProviderFactory", "Failed to create GitLocalRepository", ex);
+            _log.Error(ex, "GitLocalRepositoryProviderFactory: Failed to create GitLocalRepository");


NIT: nameof(GitLocalRepository)

dhoehna · 2024-09-27T18:42:52Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

-    private Commit? _head;
+    private readonly ILogger _log = Log.ForContext("SourceContext", nameof(RepositoryWrapper));
+
+    private string? _head;


Please add a newline.

dhoehna · 2024-09-27T18:46:43Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+    public void ValidateGitRepositoryRootPath(string rootFolder)
+    {
+        var validateGitRootRepo = GitExecute.ExecuteGitCommand(_gitDetect.GitConfiguration.ReadInstallPath(), rootFolder, "rev-parse --show-toplevel");
+        var output = validateGitRootRepo.Output;


Rename output to a name that describes the string. Maybe GitRootRepositoryPath?

dhoehna · 2024-09-27T18:50:33Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+            throw validateGitRootRepo.Ex ?? new ArgumentException($"Not a valid git repository root path:  RootFolder: {rootFolder} Git output: {output}");
+        }
+
+        if (WslIntegrator.IsWSLRepo(rootFolder))


Most of the validation code and error reposting is the same. Maybe pull the call to get a normalized path and the error string into their own if block.

Then the error path check and error reporting can be de-duped.

dhoehna · 2024-09-27T18:53:57Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        var result = GitExecute.ExecuteGitCommand(_gitDetect.GitConfiguration.ReadInstallPath(), _workingDirectory, "rev-parse HEAD");
+        if (result.Status != ProviderOperationStatus.Success)
+        {
+            throw result.Ex ?? new InvalidOperationException(result.ProcessExitCode?.ToString(CultureInfo.InvariantCulture) ?? "Unknown error while obtaining HEAD commit");


Can we provide any information to the user instead of "Unknown error while obtaining HEAD commit"? Maybe a call to action. Can they check logs somewhere else?

dhoehna · 2024-09-27T18:55:08Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        var result = GitExecute.ExecuteGitCommand(_gitDetect.GitConfiguration.ReadInstallPath(), _workingDirectory, "rev-parse HEAD");
+        if (result.Status != ProviderOperationStatus.Success)
+        {
+            throw result.Ex ?? new InvalidOperationException(result.ProcessExitCode?.ToString(CultureInfo.InvariantCulture) ?? "Unknown error while obtaining HEAD commit");


I noticed twice a check if the extension is not null. I thought the exception is always not null if the status is not success.

dhoehna · 2024-09-27T18:56:26Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

+        string? head = result.Output?.Trim();
+        if (string.IsNullOrEmpty(head))
+        {
+            throw new InvalidOperationException("Git command output is null or the repository has no commits");


No way to distinguish between a null value and a repo with no commits?

A repository with no commits is a valid repository state.

dhoehna · 2024-09-27T18:57:18Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs

-                string.Format(CultureInfo.CurrentCulture, _folderStatusDetached, _repo.Head.Tip.Sha[..7]) :
-                string.Format(CultureInfo.CurrentCulture, _folderStatusBranch, _repo.Head.FriendlyName);
-            if (_repo.Head.IsTracking)
+            branchName = repoStatus.IsHeadDetached ?


NIT: rename repoStatus to repositoryStatus

ssparach · 2024-09-27T20:17:33Z

Thanks a lot for reviewing, everyone! I went through the PR comments and did not find any blocking issues. To speed up the selfhost of this functionality, I will go ahead and wrap up this PR now. I will be sure to address the minor suggestions in a future PR.

git command line changes for WSL

a4d1f7e

ssparach changed the title ~~git command line changes for WSL~~ Git command line changes for WSL Sep 23, 2024

ssparach linked an issue Sep 23, 2024 that may be closed by this pull request

File Explorer version control integration should work with WSL repos/paths #3829

Closed

ssparach added the Area-FileExplorerIntegration 📁 label Sep 23, 2024

ssparach requested a review from DefaultRyan September 23, 2024 20:15

ssparach marked this pull request as ready for review September 23, 2024 20:20

DefaultRyan requested changes Sep 23, 2024

View reviewed changes

extensions/GitExtension/FileExplorerGitIntegration/Models/StatusCache.cs Outdated Show resolved Hide resolved

extensions/GitExtension/FileExplorerGitIntegration/Models/StatusCache.cs Outdated Show resolved Hide resolved

dhoehna reviewed Sep 23, 2024

View reviewed changes

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepositoryProviderFactory.cs Outdated Show resolved Hide resolved

dhoehna reviewed Sep 23, 2024

View reviewed changes

krschau reviewed Sep 24, 2024

View reviewed changes

address PR feedback

ac127e4

dhoehna reviewed Sep 24, 2024

View reviewed changes

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryWrapper.cs Outdated Show resolved Hide resolved

address PR feedback

1b89675

ssparach requested review from DefaultRyan, dhoehna and krschau September 25, 2024 19:59

address PR feedback

59a3112

DefaultRyan approved these changes Sep 25, 2024

View reviewed changes

ssparach requested a review from vineeththomasalex September 26, 2024 16:56

ssparach added the Needs-Second Pull request that needs another approval label Sep 26, 2024

vineeththomasalex reviewed Sep 27, 2024

View reviewed changes

krschau reviewed Sep 27, 2024

View reviewed changes

vineeththomasalex reviewed Sep 27, 2024

View reviewed changes

dhoehna reviewed Sep 27, 2024

View reviewed changes

vineeththomasalex approved these changes Sep 27, 2024

View reviewed changes

dhoehna reviewed Sep 27, 2024

View reviewed changes

ssparach merged commit c0936f9 into main Sep 27, 2024
4 checks passed

DefaultRyan mentioned this pull request Sep 28, 2024

Git extension integration in Repository Management. #3896

Merged

3 tasks

krschau removed the Needs-Second Pull request that needs another approval label Oct 2, 2024

krschau modified the milestone: Dev Home v0.19 Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Git command line changes for WSL #3893

Git command line changes for WSL #3893

ssparach commented Sep 23, 2024 •

edited

Loading

dhoehna Sep 23, 2024

krschau Sep 24, 2024

ssparach Sep 24, 2024

dhoehna Sep 24, 2024

DefaultRyan Sep 24, 2024

DefaultRyan Sep 24, 2024 •

edited

Loading

ssparach Sep 25, 2024

krschau Sep 24, 2024

vineeththomasalex Sep 27, 2024

dhoehna Sep 27, 2024

krschau Sep 27, 2024

vineeththomasalex Sep 27, 2024

dhoehna Sep 27, 2024 •

edited

Loading

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024 •

edited

Loading

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

dhoehna Sep 27, 2024

ssparach commented Sep 27, 2024

	Log.Error("Execute Git process exited unsuccessfully with exit code {ExitCode}", process.ExitCode);
	Log.Error($"Execute Git process exited unsuccessfully with exit code {process.ExitCode}");

Git command line changes for WSL #3893

Git command line changes for WSL #3893

Conversation

ssparach commented Sep 23, 2024 • edited Loading

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DefaultRyan Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssparach commented Sep 27, 2024

ssparach commented Sep 23, 2024 •

edited

Loading

DefaultRyan Sep 24, 2024 •

edited

Loading

dhoehna Sep 27, 2024 •

edited

Loading

dhoehna Sep 27, 2024 •

edited

Loading