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

Add a check for pre-normalize'd paths, to relativize #56

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Add a check for pre-normalize'd paths, to relativize #56

merged 1 commit into from
Jul 14, 2017

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand requested a review from eed3si9n July 12, 2017 09:17
@eed3si9n
Copy link
Member

Could you explain how this fixes sbt/sbt#3286?

@@ -564,7 +564,7 @@ object IO {
def relativize(base: File, file: File): Option[String] = {
val basePath = base.toPath
val filePath = file.toPath
if (filePath.normalize() startsWith basePath.normalize()) {
if ((filePath startsWith basePath) || (filePath.normalize() startsWith basePath.normalize())) {
Copy link
Member

@eed3si9n eed3si9n Jul 12, 2017

Choose a reason for hiding this comment

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

Is this happening because JIO Path is unable to handle . and .git correctly?
What if you have ../foo and .git? Don't they start the same?

  1. It might make it easier for me to understand if the toPath and if expression part is factored out to an inner function named sharesPath.
  2. After resolving the symlinks, maybe you should convert it to an absolute URI instead?

@dwijnand
Copy link
Member Author

Of course.

FileExamples.files (link) lists the child paths of a given base directory and then relativizes in terms of that base directory:

val directory: File = ...
val childPaths = IO.listFiles(directory).toStream
val prefixedDirectChildPaths = childPaths map { IO.relativize(base, _).get } filter { _ startsWith prefix }

Because of the fundamental change to use getCanonicalPath instead of getAbsolutePath in sbt 1's IO in #18 (with subsequent fixes #33 and #34) using a base directory of "." doesn't relative its children:

Welcome to Scala 2.12.2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_131).
Type in expressions for evaluation. Or try :help.

scala> val base = new java.io.File(".")
base: java.io.File = .

scala> val file = new java.io.File("./.git")
file: java.io.File = ./.git

scala> val basePath = base.toPath
basePath: java.nio.file.Path = .

scala> val filePath = file.toPath
filePath: java.nio.file.Path = ./.git

scala> filePath.normalize() startsWith basePath.normalize()
res0: Boolean = false

scala> basePath.normalize()
res1: java.nio.file.Path =

scala> filePath.normalize()
res2: java.nio.file.Path = .git

scala> basePath relativize filePath
res3: java.nio.file.Path = .git

So this change fixes the downstream issue, without breaking any of the existing regression tests for #18/#33/#34.

@dwijnand
Copy link
Member Author

@eed3si9n I'm happy to iterate on a better fix for this, but I'm keen for this less-than-ideal fix to ship in RC1, please.

@eed3si9n
Copy link
Member

I was concerned about correctness issue, but I can't think of counter example foo and foobar, so I guess we can merge.

@eed3si9n eed3si9n merged commit f55849d into sbt:1.0 Jul 14, 2017
@dwijnand dwijnand deleted the relativize-fix-3 branch July 14, 2017 11:06
@dwijnand
Copy link
Member Author

I share your concern. This fix isn't satisfying in the least. But using it downstreams seems to solve the reported reproductions. Thanks for the merge.

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.

2 participants