gnodet-bot commented on code in PR #387:
URL: https://github.com/apache/maven-filtering/pull/387#discussion_r4093587082
##########
src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java:
##########
@@ -176,15 +176,16 @@ public static String getRelativeFilePath(final String
oldPath, final String newP
return "";
}
- // normalise the path delimiters
- String fromPath = new File(oldPath).getPath();
- String toPath = new File(newPath).getPath();
+ // normalise the path delimiters to forward slashes for cross-platform
consistency
+ String fromPath = new File(oldPath).getPath().replace('\\', '/');
+ String toPath = new File(newPath).getPath().replace('\\', '/');
- // strip any leading slashes if its a windows path
- if (toPath.matches("^\\[a-zA-Z]:")) {
+ // strip any leading slashes if its a windows path (require separator
after colon to avoid
+ // false positives on unusual Unix paths like /a:/something)
+ if (toPath.matches("^[/\\\\][a-zA-Z]:[/\\\\].*")) {
Review Comment:
⚠️ **Dead backslash alternative in regex**
At this point, `toPath` (and `fromPath` below) already had `.replace('\\',
'/')` applied two lines above — so backslashes are guaranteed absent. The `\\`
alternative in `[/\\]` can never match.
Consider simplifying both `matches(...)` calls:
```suggestion
if (toPath.matches("^/[a-zA-Z]:/.*")) {
```
##########
src/test/java/org/apache/maven/shared/filtering/FilteringUtilsTest.java:
##########
@@ -146,4 +146,10 @@ void escapeWindowsPathNotAtBeginning() {
"jdbc:derby:C:\\\\Users\\\\Administrator/test;create=true",
FilteringUtils.escapeWindowsPath("jdbc:derby:C:\\Users\\Administrator/test;create=true"));
}
+
+ @Test
+ void relativeFilePathStripsLeadingSeparatorFromWindowsDrivePath() {
+ assertEquals("file.txt", FilteringUtils.getRelativeFilePath("C:/base",
"/C:/base/file.txt"));
+ assertEquals("../other/file.txt",
FilteringUtils.getRelativeFilePath("/C:/base/dir", "C:/base/other/file.txt"));
+ }
Review Comment:
⚠️ **Missing regression tests for documented Unix-path behavior**
The Javadoc documents several Unix-path contracts that have zero test
coverage — both before and after this PR. Given that the separator logic was
changed (hardcoded `'/'`, new trailing-slash check), please add tests for the
cases the Javadoc guarantees:
```suggestion
@Test
void relativeFilePathStripsLeadingSeparatorFromWindowsDrivePath() {
assertEquals("file.txt",
FilteringUtils.getRelativeFilePath("C:/base", "/C:/base/file.txt"));
assertEquals("../other/file.txt",
FilteringUtils.getRelativeFilePath("/C:/base/dir", "C:/base/other/file.txt"));
}
@Test
void relativeFilePathUnixStylePaths() {
assertEquals("java/bin",
FilteringUtils.getRelativeFilePath("/usr/local", "/usr/local/java/bin"));
assertEquals("../../bin",
FilteringUtils.getRelativeFilePath("/usr/local/", "/bin"));
assertEquals("../usr/local",
FilteringUtils.getRelativeFilePath("/bin", "/usr/local/"));
}
```
##########
src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java:
##########
@@ -176,15 +176,16 @@ public static String getRelativeFilePath(final String
oldPath, final String newP
return "";
}
- // normalise the path delimiters
- String fromPath = new File(oldPath).getPath();
- String toPath = new File(newPath).getPath();
+ // normalise the path delimiters to forward slashes for cross-platform
consistency
+ String fromPath = new File(oldPath).getPath().replace('\\', '/');
+ String toPath = new File(newPath).getPath().replace('\\', '/');
- // strip any leading slashes if its a windows path
- if (toPath.matches("^\\[a-zA-Z]:")) {
+ // strip any leading slashes if its a windows path (require separator
after colon to avoid
+ // false positives on unusual Unix paths like /a:/something)
+ if (toPath.matches("^[/\\\\][a-zA-Z]:[/\\\\].*")) {
toPath = toPath.substring(1);
}
- if (fromPath.matches("^\\[a-zA-Z]:")) {
+ if (fromPath.matches("^[/\\\\][a-zA-Z]:[/\\\\].*")) {
Review Comment:
Same dead backslash alternative — `fromPath` also has no backslashes at this
point.
```suggestion
if (fromPath.matches("^/[a-zA-Z]:/.*")) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
