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: on windows, add path use absolute, more add object for father path #3010

Closed

Conversation

liliuhai
Copy link

old code run on winodw,use add -r commands,if use absolute path as param,the result have more added object like father path,but use relativity path,no this problem.

License: MIT
Signed-off-by: liliuhai [email protected]

@djdv
Copy link
Contributor

djdv commented Jul 29, 2016

I believe it would be more appropriate to change line 401 fpath, err := filepath.EvalSymlinks(fpath) to fpath, err := filepath.ToSlash(filepath.EvalSymlinks(fpath)) instead, fpath is supposed to be standardized to forward slashes to prevent this kind of problem, it seems fe7b01f may have re-introduced this problem by reassigning fpath from standard forward slashes into platform specific paths.

For reference on standard slash paths #1933 (comment)

ping @Kubuxu

@liliuhai
Copy link
Author

I think filepath.EvalSymlinks function cause problem,in winodws,before call EvalSymlinks,fpath is use “\”,but after called fpath changed for “/",so re-introduced this problem.

@djdv
Copy link
Contributor

djdv commented Jul 31, 2016

Sorry, I neglected the multiple return values of EvalSymlinks, this is what I should have suggested: djdv@31b4cbb

@liliuhai
Copy link
Author

liliuhai commented Aug 1, 2016

thanks, like this the problem can fixed

@djdv
Copy link
Contributor

djdv commented Aug 1, 2016

Sounds good, we'll have to wait for an IPFS member to make judgment since I don't know which method is preferable or if anything else has to be done. I'm glad this regression was spotted, thank you for finding it!

@whyrusleeping
Copy link
Member

@djdv @liliuhai Would this issue have been caught by our existing sharness tests?

@@ -416,7 +417,7 @@ func appendFile(fpath string, argDef *cmds.Argument, recursive, hidden bool) (fi
}
}

return files.NewSerialFile(path.Base(fpath), fpath, hidden, stat)
return files.NewSerialFile(path.Base(filepath.ToSlash(fpath)), fpath, hidden, stat)
Copy link
Member

Choose a reason for hiding this comment

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

instead of doing this here, we should on line 401 do:

fpath = filepath.Clean(fpath)
fpath, err = filepath.EvalSymlinks(fpath)
...
fpath = filepath.ToSlash(fpath)

@Kubuxu
Copy link
Member

Kubuxu commented Aug 1, 2016

I've taken over this PR in #3023. Thanks for bringing it up to us.

We need to work on Windows CI.

@liliuhai liliuhai deleted the ipfs/fix/window_add_absolute_path branch August 4, 2016 07:04
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.

4 participants