GitProcess: allow GIT_TRACE to point to full path by derrickstolee · Pull Request #326 · microsoft/VFSForGit

derrickstolee · 2018-10-03T12:01:53Z

The GIT_TRACE variable can be helpful for diagnosing problems during Git commands. However, there are many places in VFS for Git that require parsing the Git output. Using GIT_TRACE=1 sends the output to stdout, which messes up this parsing. In the past, we have blocked that variable for this reason.

It would still be helpful to allow logging the trace output. Git allows a fully-qualified path as the value of GIT_TRACE and will append to that file instead. This is how we would like users to use GIT_TRACE when investigating issues. Unblock that type of setting.

Resolves #276

derrickstolee · 2018-10-03T13:26:38Z

If anyone is looking at the builds right now, I do have a passing functional tests build but it didn't get updated. I'm re-running.

jeschu1 · 2018-10-03T14:31:32Z

GVFS/GVFS.Common/Git/GitProcess.cs

+                // If GIT_TRACE is set to a fully-rooted path, then Git sends the trace
+                // output to that path instead of stdout (GIT_TRACE=1) or stderr (GIT_TRACE=2).
+                if (key.StartsWith("GIT_TRACE", StringComparison.OrdinalIgnoreCase) &&
+                    !(key.Equals("GIT_TRACE") && Path.IsPathRooted(processInfo.EnvironmentVariables[key])))


Any reason why we wouldn't want to leave it in fancy linq?
Aka

foreach (string key in processInfo.EnvironmentVariables.Keys.Cast<string>() .Where(x => x.StartsWith("GIT_TRACE", StringComparison.OrdinalIgnoreCase) && !Path.IsPathRooted(processInfo.EnvironmentVariables[x])) .ToList()) { processInfo.EnvironmentVariables.Remove(key); }

Also IsPathRooted can throw an exception if value contains invalid chars, We should guard against that I think https://docs.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?redirectedfrom=MSDN&view=netframework-4.7.2.

Good find on that exception! I'll need to check for invalid chars first, I guess.

The extra complication we are doing here is something that is too large to fit nicely into a .Where() clause. It's also kind of pointless that we .ToList() something that is looped in a foreach.

Thanks for going after this bug, it was actually a change I requested :-)

Why do you need the !(key.Equals(GIT_TRACE) bit?

Agree we can drop the ToList. It reads nicely to me in the Linq, but I'm happy to follow your lead here!

There are other GIT_TRACE_ variables, like GIT_TRACE_PERF, but I suppose we could stop interfering with those, too.

GVFS/GVFS.Common/Git/GitProcess.cs

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs

GVFS/GVFS.Common/Git/GitProcess.cs

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs

derrickstolee · 2018-10-04T14:46:50Z

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitRepoTests.cs

 using NUnit.Framework;
-using NUnit.Framework.Interfaces;
-using System;
+using System.Collections.Generic;


No need for this change anymore.

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs

kewillford · 2018-10-04T16:28:39Z

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs

+            GitHelpers.ValidateGitOutput(command, expectedResult, actualResult);
+
+            string.IsNullOrWhiteSpace(expectedResult.Errors).ShouldBeFalse();
+            string.IsNullOrWhiteSpace(actualResult.Errors).ShouldBeTrue();


Why did one write to Errors but the other didn't? Shouldn't the ValidateGitOutput be checking the Errors?

ValidateGitOutput does check the errors, which is why this test was failing.

Yeah test failure was

Extra: 15:11:06.229742 exec-cmd.c:236 trace: resolved executable dir: C:/Program Files/Git/mingw64/bin Extra: 15:11:06.245368 run-command.c:638 trace: run_command: C:/Repos/GVFSFunctionalTests/enlistment/5fa79051845344039854/src/.git/hooks/pre-command.exe status --git-pid=792 Extra: 15:11:06.354746 git.c:477 trace: built-in: git status Extra: 15:11:06.370374 run-command.c:638 trace: run_command: C:/Repos/GVFSFunctionalTests/enlistment/5fa79051845344039854/src/.git/hooks/post-command.exe status --git-pid=792 --exit_code=0 Missing: 15:11:06.151614 exec-cmd.c:236 trace: resolved executable dir: C:/Program Files/Git/mingw64/bin Missing: 15:11:06.151614 git.c:477 trace: built-in: git status

Extra means it was in the actual but not in expected
Missing means it was in expected but not in actual

So it looks like both expected and actual will have something in Errors right?

These tests are testing the wrong thing :(. The environment stuff we are checking is only for Git commands invoked from GVFS, while these tests are calling Git directly (not going through the code paths I'm checking). That's why they are failing.

kewillford

The changes look good, just need to get the tests fixed and passing.

derrickstolee · 2018-10-05T19:31:31Z

@kewillford Could you take a look at the new test structure? It took me a while to figure out the right thing to do, and it's very different than what you approved.

kewillford

This makes a lot more sense, since we are really only testing how VFSForGit handles the GIT_TRACE and not how it compares with the control repo. Only had a couple other questions but it looks good.

kewillford · 2018-10-05T19:35:24Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/StatusVerbTests.cs

+    [TestFixture]
+    public class StatusVerbTests : TestsWithEnlistmentPerFixture
+    {
+        private enum ExpectedReturnCode


I don't see where this is used?

Oops. It was needed for a helper function I had copied in an earlier version. Will delete.

kewillford · 2018-10-05T19:35:57Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/StatusVerbTests.cs

+            Dictionary<string, string> environmentVariables = new Dictionary<string, string>();
+
+            this.Enlistment.Status(trace: "1");
+            this.Enlistment.Status(trace: "2");


Do we need to verify anything for these?

So gvfs status could fail to parse the git status it calls if the GIT_TRACE values are not removed.

Looks like we are sending a gvfs status command. Will that cause a git status to run as well? I wasn't able to find that in the quick search I did. Could you point me at that code?

You're right. I thought it did, but it doesn't. I must call something, since logPath is filled with contents.

Ok, I took a look at my logs, and we actually run git config to get the remote url:

07:52:55.681330 exec-cmd.c:236 trace: resolved executable dir: C:/Program Files/Git/mingw64/bin 07:52:55.684326 run-command.c:638 trace: run_command: C:/_git/ForTests2/src/.git/hooks/pre-command.exe config --local remote.origin.url --git-pid=16732 07:52:55.808825 git.c:477 trace: built-in: git config --local remote.origin.url 07:52:55.809326 run-command.c:638 trace: run_command: C:/_git/ForTests2/src/.git/hooks/post-command.exe config --local remote.origin.url --git-pid=16732 --exit_code=0

This is run by GitProcess.GetOriginUrl().

The output is interpreted here in Enlistment.cs and would fail if we didn't redirect GIT_TRACE=1 (the output would not be a URL) or GIT_TRACE=2 (originResult.HasErrors would be true).

derrickstolee requested a review from wilbaker October 3, 2018 12:01

derrickstolee force-pushed the git-trace branch 2 times, most recently from 8eb553f to 70bfc78 Compare October 3, 2018 12:57

jeschu1 reviewed Oct 3, 2018

View reviewed changes

derrickstolee force-pushed the git-trace branch from 70bfc78 to 056470e Compare October 3, 2018 14:44

kewillford suggested changes Oct 3, 2018

View reviewed changes

GVFS/GVFS.Common/Git/GitProcess.cs Outdated Show resolved Hide resolved

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs Outdated Show resolved Hide resolved

derrickstolee force-pushed the git-trace branch from 056470e to 88823d9 Compare October 3, 2018 18:57

wilbaker reviewed Oct 3, 2018

View reviewed changes

GVFS/GVFS.Common/Git/GitProcess.cs Outdated Show resolved Hide resolved

GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs Outdated Show resolved Hide resolved

derrickstolee force-pushed the git-trace branch from 88823d9 to f5530c7 Compare October 4, 2018 14:42

derrickstolee commented Oct 4, 2018

View reviewed changes

kewillford reviewed Oct 4, 2018

View reviewed changes

derrickstolee force-pushed the git-trace branch 5 times, most recently from b84d90c to 350838e Compare October 5, 2018 13:16

kewillford approved these changes Oct 5, 2018

View reviewed changes

derrickstolee force-pushed the git-trace branch 4 times, most recently from 5e12623 to 3a5009f Compare October 5, 2018 16:37

kewillford approved these changes Oct 5, 2018

View reviewed changes

derrickstolee force-pushed the git-trace branch from 3a5009f to 841365f Compare October 5, 2018 20:13

GitProcess: allow GIT_TRACE to point to full path

95b9bf6

derrickstolee force-pushed the git-trace branch from 841365f to 95b9bf6 Compare October 8, 2018 15:33

derrickstolee merged commit 07c8b1d into master Oct 8, 2018

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

Conversation

derrickstolee commented Oct 3, 2018

Uh oh!

derrickstolee commented Oct 3, 2018

Uh oh!

jeschu1 Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kewillford left a comment

Choose a reason for hiding this comment

Uh oh!

derrickstolee commented Oct 5, 2018

Uh oh!

kewillford 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jeschu1 Oct 3, 2018 •

edited

Loading