Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

dirname with DOS prefixes#4111

Merged
ethomson merged 4 commits intolibgit2:masterlibgit2/libgit2:masterfrom
pks-t:pks/dos-prefixpks-t/libgit2:pks/dos-prefixCopy head branch name to clipboard
Feb 10, 2017
Merged

dirname with DOS prefixes#4111
ethomson merged 4 commits intolibgit2:masterlibgit2/libgit2:masterfrom
pks-t:pks/dos-prefixpks-t/libgit2:pks/dos-prefixCopy head branch name to clipboard

Conversation

@pks-t
Copy link
Member

@pks-t pks-t commented Feb 7, 2017

This is part 1/2 of the fix to #4106. I already debugged into the issue and have a fix in mind, but I'm currently stuck at a copy shop and cannot fire up my Windows VM to actually test it. I'll post the second part tomorrow.

The actual problem fixed here is that e.g. git_path_dirname_r("C:/") returns ".", which is simply wrong. It has been fixed to instead return "C:/". The same for network names ("//networkname/").

pks-t added 4 commits February 8, 2017 12:03
Extract code which determines if a path is at a Windows system's root.
This incluses drive prefixes (e.g. "C:\") as well as network computer
names (e.g. "//computername/").
Getting the dirname of a filesystem root should return the filesystem
root itself. E.g. the dirname of "/" is always "/". On Windows, we
emulate this behavior and as such, we should return e.g. "C:/" if
calling dirname on "C:/". But we currently fail to do so and instead
return ".", as we do not check if we actually have a Windows prefix
before stripping off the last directory component.

Fix this by calling out to `win32_prefix_length` immediately after
stripping trailing slashes, returning early if we have a prefix.
When calling `git_path_dirname_r` on a Win32 prefix, e.g. a drive
or network share prefix, we always want to return the trailing
'/'. This does not work currently when passing in a path like
'C:', where the '/' would not be appended correctly.

Fix this by appending a '/' if we try to normalize a Win32 prefix
and there is no trailing '/'.
As of recently, we failed to correctly discover repositories at a
Win32 system root. Instead of aborting the upwards-traversal of
the file system, we were looping infinitely when traversal
started at either a Win32 drive prefix ("C:/") or a network path
("//somehost").

The issue has been fixed, so add a test to catch regressions.
@pks-t
Copy link
Member Author

pks-t commented Feb 8, 2017

Okay, wrapped this up now. This fixes #4106

@pks-t
Copy link
Member Author

pks-t commented Feb 8, 2017

By the way, I'm open to bikeshedding on the win32_path_prefix interface. I'm not quite keen on its name and behavior, but got no other ideas here.

@ethomson
Copy link
Member

Thanks for fixing this. 😀

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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.