gnodet-bot commented on code in PR #393:
URL: https://github.com/apache/maven-filtering/pull/393#discussion_r4093653223
##########
src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java:
##########
@@ -56,7 +56,7 @@ public final class FilteringUtils {
*/
private static final int FILE_COPY_BUFFER_SIZE = ONE_MB * 30;
- private static final String WINDOWS_PATH_PATTERN =
"^(.*)[a-zA-Z]:\\\\(.*)";
+ private static final String WINDOWS_PATH_PATTERN =
"^(?:.*[a-zA-Z]:\\\\|[^:]+\\\\).*";
Review Comment:
⚠️ **Stale TODO/comments above this line (lines 75-76 of the new file):**
```
// TODO: Correct to handle relative windows paths.
(http://jira.apache.org/jira/browse/MSHARED-121)
// How do we distinguish a relative windows path from some other value that
happens to contain backslashes??
```
This PR answers the question in that second comment and implements the fix.
Both lines are now incorrect — the TODO is done, and the question has been
answered. Remove them.
```suggestion
private static final String WINDOWS_PATH_PATTERN =
"^(?:.*[a-zA-Z]:\\\\|[^:]+\\\\).*";
```
##########
src/test/java/org/apache/maven/shared/filtering/FilteringUtilsTest.java:
##########
@@ -113,6 +113,17 @@ void escapeWindowsPathStartingWithDrive() {
assertEquals("C:\\\\Users\\\\Administrator",
FilteringUtils.escapeWindowsPath("C:\\Users\\Administrator"));
}
+ @Test
+ void escapeWindowsPathRelative() {
+ assertEquals("src\\\\main\\\\java",
FilteringUtils.escapeWindowsPath("src\\main\\java"));
+ }
+
+ @Test
+ void escapeWindowsPathPreservesRepeatedBackslashes() {
+ // Already-escaped backslash pairs (\\) are preserved as-is
(idempotent guard)
+ assertEquals("C:\\\\Users",
FilteringUtils.escapeWindowsPath("C:\\\\Users"));
+ }
+
@Test
void escapeWindowsPathMissingDriveLetter() {
assertEquals(":\\Users\\Administrator",
FilteringUtils.escapeWindowsPath(":\\Users\\Administrator"));
Review Comment:
⚠️ **Two commented-out tests below (lines 137-151 of the new file) now pass
and must be uncommented.**
With the new `[^:]+\\` branch, both previously-failing cases now work:
- `\\Users\\Administrator` — driveless absolute path — now matched and
escaped correctly
- `${pathExpr}\\Documents` — expression prefix with backslash — now matched
and escaped correctly
Both test blocks carry the comment `// This doesn't work, see MSHARED-121`.
Since this PR fixes MSHARED-121, those comments are now wrong. Uncomment the
tests and remove the stale comments.
--
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]
