You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2021/12/15 15:34:20 UTC
[GitHub] [jackrabbit-filevault] reschke opened a new pull request #188: JCRVLT-575: handle symlinks in homeDir path
reschke opened a new pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188
Functional changes besides logging:
1. do not call "createDirectories" when the path already exists (this avoids a failure when the last path segment is a symlink)
2. when walking the homeDir, follow symlinks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin edited a comment on pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
kwin edited a comment on pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#issuecomment-995633441
> Is there an existing test class that I can extend?
~~There is org.apache.jackrabbit.vault.packaging.registry.impl.FSInstallStateTest (pretty obvious, right?)~~
Update: There is no test class yet for FSInstallStateCache, but a test with one method should be fairly easy to add.
> and we really really want to finally upgrade
Yes, I am not saying that this blocks the merge, but it has regressed before because there were no test covering that. Let's do better this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] reschke commented on pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#issuecomment-995624038
> @reschke Can you add a test leveraging https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createSymbolicLink-java.nio.file.Path-java.nio.file.Path-java.nio.file.attribute.FileAttribute...- to make sure it doesn't regress in the future?
Is there an existing test class that I can extend? (Note that this is a regression from 3.4.0 which works with symlinks just fine, and we really really want to finally upgrade)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] reschke commented on a change in pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
reschke commented on a change in pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#discussion_r770392916
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSInstallStateCache.java
##########
@@ -53,10 +58,14 @@
private Map<Path, PackageId> pathIdMapping = new ConcurrentHashMap<>();
private final Path homeDir;
-
+
public FSInstallStateCache(Path homeDir) throws IOException {
this.homeDir = homeDir;
- Files.createDirectories(homeDir);
+ log.debug("checking for presence of {} - exists {} - isDirectory {}", homeDir, Files.exists(homeDir), Files.isDirectory(homeDir));
+ if (Files.notExists(homeDir)) {
Review comment:
Ok, in that case we should fail. Will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#issuecomment-994930780
@reschke Can you add a test leveraging https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createSymbolicLink-java.nio.file.Path-java.nio.file.Path-java.nio.file.attribute.FileAttribute...- to make sure it doesn't regress in the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#discussion_r769768009
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSInstallStateCache.java
##########
@@ -53,10 +58,14 @@
private Map<Path, PackageId> pathIdMapping = new ConcurrentHashMap<>();
private final Path homeDir;
-
+
public FSInstallStateCache(Path homeDir) throws IOException {
this.homeDir = homeDir;
- Files.createDirectories(homeDir);
+ log.debug("checking for presence of {} - exists {} - isDirectory {}", homeDir, Files.exists(homeDir), Files.isDirectory(homeDir));
+ if (Files.notExists(homeDir)) {
Review comment:
What happens if the path `homeDir` is a file and not a directory?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#issuecomment-995633441
> Is there an existing test class that I can extend?
There is org.apache.jackrabbit.vault.packaging.registry.impl.FSInstallStateTest (pretty obvious, right?)
> and we really really want to finally upgrade
Yes, I am not saying that this blocks the merge, but it has regressed before because there were no test covering that. Let's do better this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] reschke commented on pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188#issuecomment-995745990
@kwin - please see https://github.com/apache/jackrabbit-filevault/pull/188/commits/b88b2838cf8a53ba83d20c51e821ca3931f5476b
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] reschke merged pull request #188: JCRVLT-575: handle symlinks in homeDir path
Posted by GitBox <gi...@apache.org>.
reschke merged pull request #188:
URL: https://github.com/apache/jackrabbit-filevault/pull/188
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org