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 PathUtil handling of spaces in file names #9305

Merged
merged 2 commits into from Jun 27, 2021

Conversation

mdonatas
Copy link
Contributor

Proposed changes

  • Currently PathUtil strips leading spaces in file names resulting in "file not found" messages when viewing files in FormCommit
  • Encoding file name before giving it to Uri fixes this

Screenshots

image

Test methodology

  • Unit tests + manually

Test environment(s)

  • Git Extensions 33.33.33
  • Build 2c32e7e
  • Git 2.31.1.windows.1
  • Microsoft Windows NT 10.0.19042.0
  • .NET 5.0.6
  • DPI 192dpi (200% scaling)

✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned mdonatas Jun 22, 2021
@@ -185,6 +185,7 @@ public void Resolve(string path, string expected)
}

[TestCase(@"C:\WORK\", @"GitExtensions\", @"C:\WORK\GitExtensions\")]
[TestCase(@"C:\WORK\", @" file.txt", @"C:\WORK\ file.txt")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a testcase for trailing space as well as space before/after /?
(could be in this string)
I

Suggested change
[TestCase(@"C:\WORK\", @" file.txt", @"C:\WORK\ file.txt")]
[TestCase(@"C:\WORK\", @" file.txt ", @"C:\WORK\ file.txt ")]
[TestCase(@"C:\WORK\", @" \ dir \ file.txt ", @"C:\WORK\ \ dir \ file.txt ")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to @" file .txt ". From my limited testing it seems that a space at the end of a file name is not possible in Windows but is possible under Linux.

@gerhardol
Copy link
Member

If this is broken in 3.5 and working in 3.4, a separate PR should be submitted to 3.5

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

The FullPathResolverTests.Resolve_should_return_full_path fails for the almost-64K path:

System.UriFormatException : Invalid URI: The Uri string is too long.
   at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind)
   at System.Uri.CreateUri(Uri baseUri, String relativeUri, Boolean dontEscape)
   at System.Uri..ctor(Uri baseUri, String relativeUri)
   at GitCommands.PathUtil.ResolveRelativePath(String path, String relativePath) in C:\projects\gitextensions\GitCommands\PathUtil.cs:line 153
   at GitCommands.PathUtil.Resolve(String path, String relativePath) in C:\projects\gitextensions\GitCommands\PathUtil.cs:line 121
   at GitCommands.FullPathResolver.Resolve(String path) in C:\projects\gitextensions\GitCommands\FullPathResolver.cs:line 65
   at GitCommandsTests.FullPathResolverTests.Resolve_should_return_full_path(String path) in C:\projects\gitextensions\UnitTests\GitCommands.Tests\FullPathResolverTests.cs:line 48

What would be an acceptable limit of the maximum path length supported?

@RussKie
Copy link
Member

RussKie commented Jun 24, 2021 via email

@gerhardol
Copy link
Member

What would be an acceptable limit of the maximum path length supported?

Is this PR changing the behavior for relative paths?
This is something that has been reorted on, both when GE internal handling wxceeds 260 chars and when the actual path is over 260. If this is the first, it should be addressed.

@mdonatas
Copy link
Contributor Author

What would be an acceptable limit of the maximum path length supported?

Is this PR changing the behavior for relative paths?
This is something that has been reorted on, both when GE internal handling wxceeds 260 chars and when the actual path is over 260. If this is the first, it should be addressed.

I don't think that it does.. the limit for path length (after some googling) seems to be a puny 260 chars for Windows and 4096 for Linux so a test-case with 64KB worth of path is an overkill :)
Apart from using relative paths which after encoding + base path would exceed 64KB external behavior doesn't change.

@mdonatas
Copy link
Contributor Author

The FullPathResolverTests.Resolve_should_return_full_path fails for the almost-64K path:

System.UriFormatException : Invalid URI: The Uri string is too long.
   at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind)
   at System.Uri.CreateUri(Uri baseUri, String relativeUri, Boolean dontEscape)
   at System.Uri..ctor(Uri baseUri, String relativeUri)
   at GitCommands.PathUtil.ResolveRelativePath(String path, String relativePath) in C:\projects\gitextensions\GitCommands\PathUtil.cs:line 153
   at GitCommands.PathUtil.Resolve(String path, String relativePath) in C:\projects\gitextensions\GitCommands\PathUtil.cs:line 121
   at GitCommands.FullPathResolver.Resolve(String path) in C:\projects\gitextensions\GitCommands\FullPathResolver.cs:line 65
   at GitCommandsTests.FullPathResolverTests.Resolve_should_return_full_path(String path) in C:\projects\gitextensions\UnitTests\GitCommands.Tests\FullPathResolverTests.cs:line 48

What would be an acceptable limit of the maximum path length supported?

It seems that Linux has a limit of 4096 so going much beyond that is not necessary but since I don't know how to choose this arbitrary limit I simply shortened that test-case to still fit the resulting path into 64KB

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Did the second proposed test case with spaces in folder names fail?

@mdonatas
Copy link
Contributor Author

Did the second proposed test case with spaces in folder names fail?

Uff, I've completely misread the proposed diff and took a second line as something which has already existed so didn't even read it :D now that I see it, the test case passes just fine but I am more hesitant to include it as a requirement on the PathUtil. Just look at what it takes to work with such folders https://superuser.com/questions/915197/how-do-i-create-a-directory-with-a-trailing-space I bet many other things would break left and right before this ever comes to execute. Ideas as to whether this requirement on behavior should really be there?

@RussKie RussKie merged commit f0fe571 into gitextensions:master Jun 27, 2021
@ghost ghost added this to the 3.6 milestone Jun 27, 2021
@RussKie
Copy link
Member

RussKie commented Jun 27, 2021

Please send a matching PR to 3.5 branch

mdonatas pushed a commit to mdonatas/gitextensions that referenced this pull request Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants