[GitHub] [sling-org-apache-sling-feature-cpconverter] karlpauls commented on pull request #47: SLING-9956 : RepPolicyEntryHandler ignores ACEs on repository level

[GitBox <gi...@apache.org>]

karlpauls commented on pull request #47:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/47#issuecomment-759353902


   > the regex in the new RepRepoPolicyEntryHandler looks a rather ugly to me.... if you have a better idea on how to do this without changing too much of the logic in RepPolicyEntryHandler, please let me know. in Oak the rep:repoPolicy can only occur at the root node but the call to matcher.group(1) in AbstractPolicyParser forces a group in the regex
   
   I think that is ok. It is not super easy to read but at the same time the parsing in general is not easy to read so I guess it doesn't make sense to invest time in optimizing this case.
   
   > RepoPath: should isRepositoryPath be reflected in the equals/hashcode methods?
   
   I think that is already covered because the path would be empty.
   
   > DefaultAclManager: the method getRepoInitPath introduced in the patch has a param SystemUser which is not used yet.... but will it be needed for SLING-9692 and SLING-9953. since i derived the patch from some initial work done for those issues i decided to leave it in.
   
   Makes sense.
   
   > TestUtils: i noticed that quite some test copy the logic on how to mock the fvault parts. i didn't yet replace all copies (just the one in the SystemUserHandlerTest). should i rather create a separate improvement for that?
   
   No. I think we can just introduce it here and then refactor while we go along with the other issues.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org