Fix FileHistory/Blame for files not existing in the working directory

[pmiossec]

Fixes #6458 and #6892

Proposed changes

The bad behavior comes from the fact that TryGetExactPath()
overwrite the out variable fileName with null
when return is false.
We should use another temporary variable.

Test methodology

• Manual

Test environment(s)

• Git Extensions 3.2.0
• Build 8fee24f (Dirty)
• Git 2.21.0.windows.1
• Microsoft Windows NT 10.0.17134.0
• .NET Framework 4.7.3416.0
• DPI 192dpi (200% scaling)

[gerhardol]

Good find
I added the issues to 3.2, this regression should be included

[pmiossec]

Even if I don't really know if 'TryGetExactPath()' is doing it's office...

I discovered with #6815 (comment), that the method is not working all the time well.

In this special case, where the casing is good because we got it from the file tree, the method that have for purpose to fix the casing, is to the contrary, returning a bad cased path....

My conclusion is that it should be either removed or called only when the input path is provided from places where we are not sure of the case accuracy, for example, the command line.

[RussKie]

Let's change the BuildFilter method from local to private and expose it via a TestAccessor (and FileName too).
Then add a simple test for the following codepath (we can add tests for other code paths later):

            var fileName = FileName;

            // we will need this later to look up proper casing for the file
            var fullFilePath = _fullPathResolver.Resolve(fileName);

            if (_controller.TryGetExactPath(fullFilePath, out fileName))
            {
                fileName = fileName.Substring(Module.WorkingDir.Length);
            }

            // Replace windows path separator to Linux path separator.
            // This is needed to keep the file history working when started from file tree in
            // browse dialog.
            FileName = fileName.ToPosixPath();

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@0ecb626). Click here to learn what that means.
The diff coverage is 45.78%.

@@            Coverage Diff            @@
##             master    #6893   +/-   ##
=========================================
  Coverage          ?   47.72%           
=========================================
  Files             ?      736           
  Lines             ?    54077           
  Branches          ?     7087           
=========================================
  Hits              ?    25806           
  Misses            ?    26878           
  Partials          ?     1393

Flag	Coverage Δ
#production	36.86% <22.41%> (?)
#tests	97.66% <100%> (?)

[RussKie]

@pmiossec I pushed a refactoring idea, to make the code testable.
Could you please have a look and confirm whether it is functionally equal to the original code.

[gerhardol]

I discovered with #6815 (comment), that the method is not working all the time well.

For 3.2, I propose this is merged.

For next release, I propose the call to TryGetExactPath() in FormFileHistory is removed and added to
GitUICommands.cs RunFileHistoryCommand(), the only "external" source for filenames.
Files in GitHub3 from the command line will still fail though...
This change could be done already for 3.2 if we are sure this is sufficient.

[pmiossec]

@RussKie It seems ok. I have added tests (and also refactored the one on TryGetExactPathName()).

This change could be done already for 3.2 if we are sure this is sufficient.

@gerhardol I have the same point of view on how to solve the problem.
And I'm not confident enough about not introducing bad side effects so I let the fix for a next release...

[gerhardol]

I approved the first commit. I am not sure after the rewrite, not sure if I see this as an improvement. Also most of the tests and changes should be removed with the proper fix: Remove TryGetExactPath from FormFileHistory*.cs

My first vote is to merge only the first commit here for 3.2.
Second alternative for 3.2 is to remove TryGetExactPath - this alternative feels better and better the more I look at it.
Third is to merge as is.
But I will not withdraw approval.

Post 3.2 is another discussion

[RussKie]

Feel free to take the first into 3.2 and then merge the rest after the 3.2 cut

[gerhardol]

I have the same point of view on how to solve the problem.
And I'm not confident enough about not introducing bad side effects so I let the fix for a next release...

I can only see that the only call path where a path is not a GitItem is for GitUICommands.cs RunFileHistoryCommand()
I feel confident enough to suggest that this should be the solution in 3.2
The refactoring and added tests can be added to 3.3 (some refactoring needed).
It may be OK to skip updating RunFileHistoryCommand() in 3.2, the change still fixes the case where the path do not exist.

@pmiossec
Since you done the investigation here, do you want to submit a PR for 3.2?

[pmiossec]

I would try to do it but most probably not before 2 or 3 days...

[gerhardol]

I would try to do it but most probably not before 2 or 3 days...

You can copy the commit I did, or I submit the pr

[gerhardol]

This should be WIP and rebased on master with slightly updated testcases after the 3.2 release branch is created.
(This is probably an improvement even if I was negative to merge close to a release.)

[RussKie]

Please resolve the merge conflict. We can probably squash it too 🤔

[RussKie]

@pmiossec could you please resolve conflicts and squash, and we can take it in the 3.2 release.

[pmiossec]

For me, this PR is no more needed since the merge of #6909

[gerhardol]

@pmiossec
I believe you did some refactoring here. It is up to you if that is something that should be included in master

[pmiossec]

No, I won't work on that. Maybe later.
I have some more priority changes to include first...

[RussKie]

Closing. @pmiossec please re-open or open a new PR when you're ready.