Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix separator in file name within File History form #4770

Merged

Conversation

lanfeust69
Copy link
Contributor

Following generalization of use of IFullPathResolver, the separator used in the
File History form ends up as the native one, while it should be the posix '/'
for later git commands to work properly (via right-click command for instance).

Following generalization of use of IFullPathResolver, the separator used in the
File History form ends up as the native one, while it should be the posix '/'
for later git commands to work properly (via right-click command for instance).
Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal is to have the FileName property be a proper POSIX path then perhaps the conversion should be done on line 203 where the property is assigned. The path can be sourced from multiple paths, and at this point this PR only addresses one of them.

What do you think?

@drewnoakes
Copy link
Member

I see the fileName local is used below (eg. line 220), so it's not enough to just call ToPosixPath on the assignment. Instead maybe insert fileName = fileName.ToPosixPath() on line 202.

@drewnoakes
Copy link
Member

The bug this PR fixes causes file histories to not work across renames, even when the option for this behaviour is requested.

@lanfeust69
Copy link
Contributor Author

I tried to fix the regression introduced here in a minimalist fashion :

  • the local fileName is supposed to be posix-formatted per comment on line 148
  • this is no longer true at the one place where fileName depends on fullFilePath, line 195
  • so fix line 195

The alternative would be to move the lines 173-176 to line 202, possibly slightly better...

BTW, I checked that fullFilePath is not used elsewhere in a way were it being a native path could cause problem.

@drewnoakes drewnoakes merged commit d086855 into gitextensions:master Apr 6, 2018
@drewnoakes
Copy link
Member

Looks good. Thanks!

@lanfeust69 lanfeust69 deleted the fix-file-history-separator branch April 6, 2018 17:41
@RussKie
Copy link
Member

RussKie commented Apr 6, 2018

@lanfeust69 could you please the same fix to 2.51 branch?

@RussKie RussKie added this to the 2.51.x + 3.00 milestone Apr 6, 2018
@lanfeust69
Copy link
Contributor Author

@RussKie : the bug shouldn't happen in 2.51, which doesn't include 66db699. Or am I misreading the history ?

@RussKie
Copy link
Member

RussKie commented Apr 8, 2018 via email

@gitextensions gitextensions deleted a comment from codecov bot Apr 9, 2018
@RussKie RussKie modified the milestones: 2.51.x + 3.00, 3.00 Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants