GitProcess: allow GIT_TRACE to point to full path

[derrickstolee]

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]

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]

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.

[derrickstolee]

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.

[jeschu1]

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!

[derrickstolee]

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

[derrickstolee]

No need for this change anymore.

[kewillford]

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

[derrickstolee]

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

[kewillford]

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?

[derrickstolee]

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]

@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]

I don't see where this is used?

[derrickstolee]

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

[kewillford]

Do we need to verify anything for these?

[derrickstolee]

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

[kewillford]

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?

[derrickstolee]

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

[derrickstolee]

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().

[derrickstolee]

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