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

POSIX-compliant implementation of relative_path.sh #16838

Merged
merged 5 commits into from Jun 9, 2016
Merged

POSIX-compliant implementation of relative_path.sh #16838

merged 5 commits into from Jun 9, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jun 8, 2016

This is an alternative implementation of relative_path.sh, authored by @simonair on http://stackoverflow.com/a/14914070
Known limitations: it doesn't clean multiple slashes and doesn't resolve symlinks.
Let's see if it passes Travis CI.

This is an alternative implementation of relative_path.sh, authored by @simonair on http://stackoverflow.com/a/14914070
Known limitations: it doesn't clean multiple slashes and doesn't resolve symlinks.
Let's see if it passes Travis CI.
@ghost ghost mentioned this pull request Jun 8, 2016

echo $result
#!/bin/sh
# This file is a part of StackOverflow. License is CC-BY-SA
Copy link
Contributor

Choose a reason for hiding this comment

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

"based on code from" - and add this file to the exception list in contrib/add_license_to_files.jl

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

seems to work on travis. does the existing version handle symlinks or multiple slashes? how does this handle the problem cases from #14097 / #14103?

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

there's also a comment on SO that "Doesn't work when the first path ends in / I'm afraid." not sure what to make of that or how general we need this to be.

@ghost
Copy link
Author

ghost commented Jun 9, 2016

there's also a comment on SO that "Doesn't work when the first path ends in / I'm afraid."

I did a quick test and it appears to work. Maybe that comment refers to another implementation, there are 3 of them in simon's answer.
Can you test in your machine too?

About multiple slashes, if it's really a concern, we can pipe the output of relpath through sed s#//*#/#g
But I don't know how to deal with symlinks. But if the current version of relative_path.sh doesn't deal with symlinks, then we're fine to merge.

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

there were failure cases linked above that we want to be sure we're not reintroducing

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

If we decide this version is okay it would also be worth asking @simonair if he would be willing to relicense his code snippet as MIT.

@simonheimlicher
Copy link

I'm honored to see my little code snippet be of use. I would be happy to license it under the MIT license – what do I need to do?

@StefanKarpinski
Copy link
Member

Saying so is all you need to do. Thanks!

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

Yep, thanks much! We can then remove the license special-casing for that file, and give it the standard same MIT banner we give everything else. More convenient to have fewer exceptions.

@ghost
Copy link
Author

ghost commented Jun 9, 2016

With licensing solved, we just have to wait for @Keno to give the green-light and it can be merged.

@ghost
Copy link
Author

ghost commented Jun 9, 2016

Two simple questions.

  • Do we need to deal with multiple slashes?
    If so, the solution is a one line change.
    -echo "$relative"
    +echo "$relative" | sed s#//*#/#g
  • Do we need to handle symlinks?
    I don't know. Apparently the current version doesn't?
    If so we should be fine to merge.

Can someone answer these questions?

@Keno
Copy link
Member

Keno commented Jun 9, 2016

Did you check that it correctly handles the two corner cases we had issues for above, i.e.
/foo/bar relative to /foo/bar-baz? and
/foo relative to /bar?

@ghost
Copy link
Author

ghost commented Jun 9, 2016

Is this what you ask?

$ ./relative_path.sh /foo/bar /foo/bar-baz
../bar-baz
$ ./relative_path.sh /foo /bar
../bar

@Keno
Copy link
Member

Keno commented Jun 9, 2016

Yes, those were the two failure cases in the old script that we had to fix. LGTM then.

@ghost
Copy link
Author

ghost commented Jun 9, 2016

Do you think it's necessary to deal with multiple slashes?
-echo "$relative"
+echo "$relative" | sed s#//*#/#g

Or can it be merged already?

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

Doesn't look like the current script does so. Everywhere we call it should be getting passed off to things that will deal with multiple slashes as expected.

@tkelman tkelman merged commit b5c1479 into JuliaLang:master Jun 9, 2016
@ghost ghost mentioned this pull request Jun 9, 2016
9 tasks
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