Ingore empty paths in tar files. Fix regex match for .helmignore. #201
Conversation
| @@ -259,11 +259,11 @@ public void addPatterns(final Collection<? extends String> stringPatterns) { | |||
| regex.append("\\."); | |||
| break; | |||
| case '*': | |||
| regex.append("[^").append(File.separator).append("]*"); | |||
| break; | |||
| regex.append("[^").append(File.separator).append(File.separator).append("]*"); | |||
There was a problem hiding this comment.
Thanks for your PR. Won't this doubling-up of File.separator be valid only on Windows?
There was a problem hiding this comment.
I don't think it's platform-specific since the slash is there to escape the other slash to make the regular expression valid. The use of File.separator is misleading in this scenario because it's really just a slash in the regular expression. We are using the code in Linux-based containers and it's working with no issues.
There was a problem hiding this comment.
The reason I'm asking is because normally in a regular expression you escape something with a backslash. And on windows, indeed File.separator would be a backslash. But on Linux, it would be a forward slash, which doesn't escape anything. I'll see if I can put a test together to reproduce the issue you're talking about.
There was a problem hiding this comment.
Also what's odd in this particular case is that [^x] means "a single character that is not x". So [^/], for example, means "a single character that is not /". You've changed this to be, exactly, [^//], which means...I'm actually not entirely sure what it means 😄. Maybe [^xx] is equivalent to [^x]; not sure. Anyway, I'm somewhat surprised here—not that there isn't a problem; I believe you—but that doubling a forward slash inside a single character negation pattern would somehow be responsible for fixing it.
There was a problem hiding this comment.
I agree. My guess is that it fails on Windows (or at least doesn't work as intended). I'll replace the file separators with \ characters so that it acts consistently.
There was a problem hiding this comment.
But then you'll end up with [^\\], which means "any character that is not a backslash", and I doubt that's actually what you mean. I think we'll need a test here to show the behavior that you're observing.
There was a problem hiding this comment.
Yeah, I can't say I understand 100% what this logic is doing. My best guess is that it's attempting to emulate wildcard behavior by skipping all non backslash characters after the * (or one in the case of ?). If that's the case, I think the updated code is valid. There are test cases here for a lot of the potential .helmignore patterns and they are passing with the updated code.
There was a problem hiding this comment.
Do you have a link showing the problematic behavior with the current code? Better yet, could you attach such a test case to your PR, such that it fails with the current microbean-helm codebase, but passes with your PR changes? I need to fully understand the problem that these changes are claiming to solve.
Empty paths in chart tar files cause a null pointer exception. Added code to skip those entries since some published chart archives have this issue. Also, the regex handling for
.helmignoreproduces invalid regular expressions because of a slash that is not escaped.