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

Implement lfs.realpath() #85

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

n1tehawk
Copy link
Contributor

The function wraps realpath() for POSIX systems, and uses _fullpath() to achieve something similar on Windows. For now, I've deliberately chosen not to hide different / OS-specific behavior caused by the underlying implementation - e.g. Windows will not complain on non-existent paths.

This pull request addresses #64.

Regards, NiteHawk

For POSIX systems, the function is a straightforward wrapper
around realpath() - see "man 3 realpath".
On Windows, the CRT function `_fullpath` gets used instead,
see https://msdn.microsoft.com/en-us/library/506720ff.aspx
@coveralls
Copy link
Collaborator

coveralls commented Oct 18, 2016

Coverage Status

Coverage increased (+0.7%) to 78.788% when pulling 84628e4 on n1tehawk:20161018_realpath into 3c4e563 on keplerproject:master.

@hishamhm
Copy link
Member

This is a welcome addition! :)

For now, I've deliberately chosen not to hide different / OS-specific behavior caused by the underlying implementation - e.g. Windows will not complain on non-existent paths.

I think lfs should strive to hide OS differences as much as possible. I remember we already have some headaches when other functions did not follow this principle, let's not add more portability corner cases.

Related question: does the Windows version resolve symlinks?

Also, this PR should target branch 1.7 since it adds to the API.

@coveralls
Copy link
Collaborator

coveralls commented Oct 19, 2016

Coverage Status

Coverage increased (+0.7%) to 78.788% when pulling 2ad5d9a on n1tehawk:20161018_realpath into 3c4e563 on keplerproject:master.

@n1tehawk
Copy link
Contributor Author

n1tehawk commented Oct 19, 2016

I think lfs should strive to hide OS differences as much as possible. I remember we already have some headaches when other functions did not follow this principle, let's not add more portability corner cases.

I understand that concern, but a cross-platform wrapper isn't trivial - especially as _fullpath doesn't seem to care about non-existing components in the path.

Related question: does the Windows version resolve symlinks?

Unfortunately: No. - according to my (preliminary) testing. :( This is likely again related to the fact that the Windows function doesn't care about the actual filesystem, but simply applies certain rules to "canonicalize" the given path; as illustrated by _fullpath("C:\\foo\\..\\bar") .

My impression is that support for the Windows platform - and especially handling "symlinks" (NTFS reparse points) - would benefit from some major refactoring and possibly introducing some platform-specific helper functions (which probably makes it a candidate for the 2.0 version). I'll have to further investigate that when I find some time.

Regards, NiteHawk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants