Skip to content

Conversation

@danhper
Copy link

@danhper danhper commented Feb 2, 2016

This should fix #4252.

Copy link
Member

Choose a reason for hiding this comment

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

I am not very confident with this byte_size call. Maybe we could fix this by moving remove_dirsep inside the do_join calls below and only call it when appropriate? Can you give this alternative solution a try to see if things are better or worse? Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll give it a try!

@josevalim
Copy link
Member

Thank you! I have added a comment!

@danhper danhper force-pushed the fix-path-join-bug branch 2 times, most recently from aa19e27 to aae18d9 Compare February 2, 2016 08:48
@danhper
Copy link
Author

danhper commented Feb 2, 2016

@josevalim I tried to move the logic to do_join but I am not sure if there is
a way to avoid checking the byte size, as the / case is a little special.

Maybe it would be better to move the last remove_dirsep call in a normalize
function or something like this and handle this logic there.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need "" inside the pattern?
<<_, "">> is the same as <<_>>.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I changed the code a little!

@josevalim
Copy link
Member

Maybe we can change remove_dirsep to not do anything if it receives only
"/" then?

On Tuesday, February 2, 2016, Daniel Perez notifications@github.com wrote:

@josevalim https://github.com/josevalim I tried to move the logic to
do_join but I am not sure if there is
a way to avoid checking the byte size, as the / case is a little special.

Maybe it would be better to move the last remove_dirsep call in a
normalize
function or something like this and handle this logic there.


Reply to this email directly or view it on GitHub
#4253 (comment).

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@josevalim
Copy link
Member

@tuvistavie I have added a new comment so maybe we explicitly check for "/" in remove_dirsep. That should achieve the same but a bit more cleanly? Thanks for trying all of those different approaches!

@danhper
Copy link
Author

danhper commented Feb 4, 2016

@josevalim Sorry for the delay.

I changed the remove_dirsep and do_join
to make this a little cleaner. This should work prefectly for UNIX/Linux based OS,
but I am not very sure if we need to do something special for Windows here.

josevalim added a commit that referenced this pull request Feb 4, 2016
@josevalim josevalim merged commit 2028e7e into elixir-lang:master Feb 4, 2016
@danhper danhper deleted the fix-path-join-bug branch February 4, 2016 08:33
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.

Path.join(["/", "", "foo"]) != Path.join(["/", "foo"])

3 participants