Skip to content

Fix UnionPath#4

Merged
cpw merged 7 commits intoMcModLauncher:mainfrom
noeppi-noeppi:main
Dec 31, 2021
Merged

Fix UnionPath#4
cpw merged 7 commits intoMcModLauncher:mainfrom
noeppi-noeppi:main

Conversation

@noeppi-noeppi
Copy link
Copy Markdown
Contributor

Currently UnionPath#relativize gives wrong results for absolute paths as a simple test program shows:

Path regularBase = Paths.get("/example/path");
Path regularPath = Paths.get("/example/path/with/a/few/elements");
System.out.println(regularBase.relativize(regularPath));

FileSystem fs = new UnionFileSystemProvider().newFileSystem(URI.create("union:file:/"), Map.of("filter", (BiPredicate<String, String>) (p, b) -> true));
Path unionBase = fs.getPath("/example/path");
Path unionPath = fs.getPath("/example/path/with/a/few/elements");
System.out.println(unionBase.relativize(unionPath));

which gives

with/a/few/elements
path/with/a/few/elements

where you see the intended behavior in the top line and the behavior with union path in the second line.

This is because when creating a subpath in relativize, poff is not added to the start index, so the resulting path will always have one element more than it should because of the empty root component.

Another thing I saw is that it is valid to relativize a non-absolute path with an absolute path which should not be allowed. This is not addressed by the pull request for now because I'm not certain whether It can safely be merged or might break existing code somewhere else.

@noeppi-noeppi
Copy link
Copy Markdown
Contributor Author

Noticed that other methods (like getNameCount and subPath also don't work as expected on absolute paths. I'll make this a draft until they are fixed as well

@noeppi-noeppi noeppi-noeppi marked this pull request as draft September 29, 2021 19:16
@LexManos
Copy link
Copy Markdown
Contributor

I've been meaning to get around to reviewing this. But haven't had the time to sit down and read the NIO specs on what these functions should actually return.
Sorry.
I'm sure there are several small cases where things could be fixed up.

…t is pasing test now however better testing for the UnionPath methods is required.
@noeppi-noeppi noeppi-noeppi changed the title Fix UnionPath#relativize Fix UnionPath Sep 29, 2021
@noeppi-noeppi
Copy link
Copy Markdown
Contributor Author

noeppi-noeppi commented Sep 30, 2021

Should be done now, however it breaks stuff when actually using it together with modlauncher. By that I also noticed that ServiceLoaderUtils#fileNameFor in modlauncher seems to not have worked correctly until now either. So I'll open a pull request on modlauncher (and maybe forge, depending on whether it breaks with these changes) as well.
When it is working, I'll mark this as ready for review.

…Path#getFileName return an empty string for the empty absolute path.
@noeppi-noeppi
Copy link
Copy Markdown
Contributor Author

I made it work without changes in modlauncher or forge. Should be ready for review now.

@noeppi-noeppi noeppi-noeppi marked this pull request as ready for review October 13, 2021 10:59
@LexManos
Copy link
Copy Markdown
Contributor

Sorry havent gotten around to testing this.
Can you add unit tests? So that we can be sure it returns what we think it does/should.

@noeppi-noeppi
Copy link
Copy Markdown
Contributor Author

For what should I add unit tests? I think for the methods in UnionPath there are already pretty extensive tests. But I can add more if needed.

@LexManos
Copy link
Copy Markdown
Contributor

My bad, I must of been looking at a single commit view and not the full changes. It looked like you skipped the unit tests entirely.

@sciwhiz12 sciwhiz12 added the bug Something isn't working label Dec 30, 2021
@cpw
Copy link
Copy Markdown
Member

cpw commented Dec 31, 2021

Does this interact with #18 at all?

@amadornes
Copy link
Copy Markdown
Contributor

I've been looking through the code and they shouldn't clash.

Just in case, I just tried merging both PRs locally and all tests pass.

@cpw cpw merged commit 218ceaa into McModLauncher:main Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants