Conversation
Pull Request Test Coverage Report for Build 23194065055Details
💛 - Coveralls |
| if (isDirectoryLink(path)) { | ||
| deletePath(path); |
There was a problem hiding this comment.
Fully correct to fix the issue.
Just as a question: Since you correctly also added the LinkOption.NOFOLLOW_LINKS to the isDirectory check in deleteRecursive why do we need this if anymore at all?
We could now always just call deleteRecursive for simplification...
| LOG.trace("Creating a Windows {} at {} pointing to {}", type, link, source); | ||
| ProcessContext pc = this.context.newProcess().executable("cmd").addArgs("/c", "mklink", type.getMklinkOption()); | ||
| Path finalSource = source; | ||
| Path absoluteSource = source.isAbsolute() ? source : link.getParent().resolve(source).normalize(); |
There was a problem hiding this comment.
nice. This seems exactly the fix we need to resolve foo/../bar properly.
Great job 👍
But shouldn't we already do that in link method before already?
Also for real (sym)links without mklink we should not link to foo/../bar but normalize to bar then...
| if (type == PathLinkType.SYMBOLIC_LINK) { | ||
| boolean directoryTarget = Files.isDirectory(absoluteSource, LinkOption.NOFOLLOW_LINKS); | ||
| if (directoryTarget && runMklink(finalSource, finalLink, cwd, "/j")) { | ||
| return; | ||
| } | ||
| if (runMklink(finalSource, finalLink, cwd, "/d")) { | ||
| return; | ||
| } | ||
| throw new IllegalStateException("Failed to create Windows link at " + link + " pointing to " + source); | ||
| } | ||
|
|
||
| if (!runMklink(finalSource, finalLink, cwd, type.getMklinkOption())) { |
There was a problem hiding this comment.
Also an excellent improvement (actually a fix not related to #1738).
Small suggestion to simplify the code:
| if (type == PathLinkType.SYMBOLIC_LINK) { | |
| boolean directoryTarget = Files.isDirectory(absoluteSource, LinkOption.NOFOLLOW_LINKS); | |
| if (directoryTarget && runMklink(finalSource, finalLink, cwd, "/j")) { | |
| return; | |
| } | |
| if (runMklink(finalSource, finalLink, cwd, "/d")) { | |
| return; | |
| } | |
| throw new IllegalStateException("Failed to create Windows link at " + link + " pointing to " + source); | |
| } | |
| if (!runMklink(finalSource, finalLink, cwd, type.getMklinkOption())) { | |
| String options = type.getMklinkOption(); | |
| if (type == PathLinkType.SYMBOLIC_LINK) { | |
| boolean directoryTarget = Files.isDirectory(absoluteSource, LinkOption.NOFOLLOW_LINKS); | |
| if (directoryTarget) { | |
| options = "/j"; | |
| } else { | |
| options = "/d"; | |
| } | |
| } | |
| if (!runMklink(finalSource, finalLink, cwd, type.getMklinkOption())) { |
I do not expect that mklink /j ... on a directory fails but then mklink /d ... will succeed. That both variants will be tried in case the first failed is for me rather confusing.
BTW: Do we really want LinkOption.NOFOLLOW_LINKS for targetDirectory?
What if we create a link to a link linking to a directory...?
3 participants
This PR fixes #1738
Implemented changes:
IDEasy/cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.javato stop following links:deleteRecursive(Path path)forFiles.isSymbolicLink(path) || isJunction(path), then delete link and return. * This prevents recursion into link targets (including self-referencing links/junctions)Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal