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

Proper root init for MemMapFs in windows #226

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

Conversation

nofun97
Copy link

@nofun97 nofun97 commented Nov 11, 2019

Fixes #225

Problem:

MemMapFs is set to initialize with a map data structure and a root path was added to it. The path that was added uses the os.PathSeparator as the root path. While UNIX has no problem with this, Windows does as the volume name is not present in os.PathSeparator

Changes:

  • MemMapFs getData() init is now changed to add a root absolute instead of FilePathSeparator
  • normalizePath(path string) string is changed to return a proper root absolute if the cleaned path results in a root path (/ or \\) and an error
  • BasePathFs removes volume name from path when Name() string is called to hide real path
  • Several tests are changed to implement the changes

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2019

CLA assistant check
All committers have signed the CLA.

@nofun97 nofun97 changed the title Feature/proper root init mem map fs Proper root init for MemMapFs in windows Nov 11, 2019
@nofun97 nofun97 requested a review from bep January 14, 2020 22:55
composite_test.go Outdated Show resolved Hide resolved
composite_test.go Outdated Show resolved Hide resolved
@nofun97
Copy link
Author

nofun97 commented Mar 28, 2020

rebased and added requested changes

@nofun97 nofun97 requested a review from 0xmichalis March 28, 2020 11:42
@nofun97 nofun97 force-pushed the feature/proper-root-init-MemMapFs branch from 5e4c984 to f10f855 Compare March 28, 2020 11:45
absolutePath, err := filepath.Abs(FilePathSeparator)
if err != nil {
log.Fatal(err)
panic(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is panic needed if you log.Fatal above?

@0xmichalis
Copy link
Collaborator

This looks like a breaking change so need other folks to chime in and comment on whether it makes sense to have.

@0xmichalis
Copy link
Collaborator

See also #171

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.

MemMapFs open windows absolute path does not exist
3 participants