-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve trailing newlines in stripLineNumbers for apply_diff (#8020) #8021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { MultiSearchReplaceDiffStrategy } from "../multi-search-replace" | ||
|
|
||
| describe("MultiSearchReplaceDiffStrategy - Issue #8020", () => { | ||
| let strategy: MultiSearchReplaceDiffStrategy | ||
|
|
||
| beforeEach(() => { | ||
| strategy = new MultiSearchReplaceDiffStrategy() | ||
| }) | ||
|
|
||
| it("should strip line numbers from last line without trailing newline", async () => { | ||
| // This reproduces the exact issue from #8020 | ||
| // The problem was that the regex was removing the trailing newline, | ||
| // which caused stripLineNumbers to fail on the last line | ||
| const originalContent = "line 1\nline 2\nline 3" | ||
|
|
||
| // Diff content with line numbers on every line including the last | ||
| const diffContent = `<<<<<<< SEARCH | ||
| 1 | line 1 | ||
| 2 | line 2 | ||
| 3 | line 3 | ||
| ======= | ||
| 1 | line 1 | ||
| 2 | modified | ||
| 3 | line 3 | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
|
|
||
| if (!result.success) { | ||
| console.error("Failed with:", result.error || result.failParts) | ||
| } | ||
|
|
||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.content).toBe("line 1\nmodified\nline 3") | ||
| } | ||
| }) | ||
|
|
||
| it("should handle the exact case from issue #8020", async () => { | ||
| // Simplified version of the actual case | ||
| const originalContent = "some code);" | ||
|
|
||
| const diffContent = `<<<<<<< SEARCH | ||
| 1479 | some code); | ||
| ======= | ||
| 1488 | some code); | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
|
|
||
| if (!result.success) { | ||
| console.error("Failed with:", result.error || result.failParts) | ||
| } | ||
|
|
||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| // Content should remain the same since we're just replacing with the same | ||
| expect(result.content).toBe("some code);") | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { MultiSearchReplaceDiffStrategy } from "../multi-search-replace" | ||
|
|
||
| describe("MultiSearchReplaceDiffStrategy - Line Number Stripping", () => { | ||
| describe("line number stripping at last line", () => { | ||
| let strategy: MultiSearchReplaceDiffStrategy | ||
|
|
||
| beforeEach(() => { | ||
| strategy = new MultiSearchReplaceDiffStrategy() | ||
| }) | ||
|
|
||
| it("should correctly strip line numbers from the last line in SEARCH and REPLACE blocks", async () => { | ||
| // This test case reproduces the issue from GitHub issue #8020 | ||
| // where the last line with a line number wasn't being stripped correctly | ||
| // Note: The original content should NOT have line numbers | ||
| const originalContent = ` ContactRelaEnum.MS.name(), addressList, contactStatusMap, SensitiveType.ADDRESS, curSector); | ||
| } | ||
| } | ||
| List<ContactInfoItemResp> addressInfoList = new ArrayList<>(CollectionUtils.size(repairInfoList) > 10 ? 10 | ||
| : CollectionUtils.size(repairInfoList) + CollectionUtils.size(homeAddressInfoList) | ||
| + CollectionUtils.size(idNoAddressInfoList) + CollectionUtils.size(workAddressInfoList) | ||
| + CollectionUtils.size(personIdentityInfoList));` | ||
|
|
||
| const diffContent = `<<<<<<< SEARCH | ||
| 1473 | ContactRelaEnum.MS.name(), addressList, contactStatusMap, SensitiveType.ADDRESS, curSector); | ||
| 1474 | } | ||
| 1475 | } | ||
| 1476 | List<ContactInfoItemResp> addressInfoList = new ArrayList<>(CollectionUtils.size(repairInfoList) > 10 ? 10 | ||
| 1477 | : CollectionUtils.size(repairInfoList) + CollectionUtils.size(homeAddressInfoList) | ||
| 1478 | + CollectionUtils.size(idNoAddressInfoList) + CollectionUtils.size(workAddressInfoList) | ||
| 1479 | + CollectionUtils.size(personIdentityInfoList)); | ||
| ======= | ||
| 1473 | ContactRelaEnum.MS.name(), addressList, contactStatusMap, SensitiveType.ADDRESS, curSector); | ||
| 1474 | } | ||
| 1475 | } | ||
| 1476 | | ||
| 1477 | // | ||
| 1478 | if (isAddressDisplayOptimizeEnabled()) { | ||
| 1479 | homeAddressInfoList = filterAddressesByThreeYearRule(homeAddressInfoList); | ||
| 1480 | personIdentityInfoList = filterAddressesByThreeYearRule(personIdentityInfoList); | ||
| 1481 | idNoAddressInfoList = filterAddressesByThreeYearRule(idNoAddressInfoList); | ||
| 1482 | workAddressInfoList = filterAddressesByThreeYearRule(workAddressInfoList); | ||
| 1483 | } | ||
| 1484 | | ||
| 1485 | List<ContactInfoItemResp> addressInfoList = new ArrayList<>(CollectionUtils.size(repairInfoList) > 10 ? 10 | ||
| 1486 | : CollectionUtils.size(repairInfoList) + CollectionUtils.size(homeAddressInfoList) | ||
| 1487 | + CollectionUtils.size(idNoAddressInfoList) + CollectionUtils.size(workAddressInfoList) | ||
| 1488 | + CollectionUtils.size(personIdentityInfoList)); | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
| if (!result.success) { | ||
| console.error("Test failed with error:", result.error || result.failParts) | ||
| } | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| const expectedContent = ` ContactRelaEnum.MS.name(), addressList, contactStatusMap, SensitiveType.ADDRESS, curSector); | ||
| } | ||
| } | ||
| // | ||
| if (isAddressDisplayOptimizeEnabled()) { | ||
| homeAddressInfoList = filterAddressesByThreeYearRule(homeAddressInfoList); | ||
| personIdentityInfoList = filterAddressesByThreeYearRule(personIdentityInfoList); | ||
| idNoAddressInfoList = filterAddressesByThreeYearRule(idNoAddressInfoList); | ||
| workAddressInfoList = filterAddressesByThreeYearRule(workAddressInfoList); | ||
| } | ||
| List<ContactInfoItemResp> addressInfoList = new ArrayList<>(CollectionUtils.size(repairInfoList) > 10 ? 10 | ||
| : CollectionUtils.size(repairInfoList) + CollectionUtils.size(homeAddressInfoList) | ||
| + CollectionUtils.size(idNoAddressInfoList) + CollectionUtils.size(workAddressInfoList) | ||
| + CollectionUtils.size(personIdentityInfoList));` | ||
| expect(result.content).toBe(expectedContent) | ||
| } | ||
| }) | ||
|
|
||
| it("should handle Windows CRLF line endings with line numbers at the last line", async () => { | ||
| // Test with Windows-style CRLF line endings | ||
| const originalContent = "line 1\r\nline 2\r\nline 3" | ||
| const diffContent = `<<<<<<< SEARCH | ||
| 1 | line 1 | ||
| 2 | line 2 | ||
| 3 | line 3 | ||
| ======= | ||
| 1 | line 1 | ||
| 2 | modified line 2 | ||
| 3 | line 3 | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.content).toBe("line 1\r\nmodified line 2\r\nline 3") | ||
| } | ||
| }) | ||
|
|
||
| it("should handle single line with line number at the end", async () => { | ||
| // Simple test case for a single line with line number | ||
| const originalContent = "some content here" | ||
| const diffContent = `<<<<<<< SEARCH | ||
| 1 | some content here | ||
| ======= | ||
| 1 | modified content here | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.content).toBe("modified content here") | ||
| } | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { MultiSearchReplaceDiffStrategy } from "../multi-search-replace" | ||
|
|
||
| describe("MultiSearchReplaceDiffStrategy - Simple Line Number Test", () => { | ||
| let strategy: MultiSearchReplaceDiffStrategy | ||
|
|
||
| beforeEach(() => { | ||
| strategy = new MultiSearchReplaceDiffStrategy() | ||
| }) | ||
|
|
||
| it("should handle simple line number stripping", async () => { | ||
| const originalContent = "line 1\nline 2\nline 3" | ||
| const diffContent = `<<<<<<< SEARCH | ||
| 1 | line 1 | ||
| 2 | line 2 | ||
| 3 | line 3 | ||
| ======= | ||
| 1 | line 1 | ||
| 2 | modified line 2 | ||
| 3 | line 3 | ||
| >>>>>>> REPLACE` | ||
|
|
||
| const result = await strategy.applyDiff(originalContent, diffContent) | ||
| console.log("Result:", result) | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.content).toBe("line 1\nmodified line 2\nline 3") | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,7 +163,14 @@ export function stripLineNumbers(content: string, aggressive: boolean = false): | |
|
|
||
| // Join back with original line endings (carriage return (\r) + line feed (\n) or just line feed (\n)) | ||
| const lineEnding = content.includes("\r\n") ? "\r\n" : "\n" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this approach intentional? The function detects line endings by checking if ANY '\r\n' exists in the content. This could cause issues with mixed line endings - for example, if a file has mostly LF endings with just one CRLF, it would treat everything as CRLF. Would it be better to check which line ending is more prevalent, or handle mixed endings differently? |
||
| return processedLines.join(lineEnding) | ||
| const result = processedLines.join(lineEnding) | ||
|
|
||
| // Preserve the original trailing newline if it existed | ||
| // This is important for diffs where the last line might not have a newline | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice fix for preserving trailing newlines! Consider enhancing this comment to explicitly mention that this addresses the issue where the regex in multi-search-replace.ts removes trailing newlines with |
||
| if (content.endsWith("\n") || content.endsWith("\r\n")) { | ||
| return result.endsWith(lineEnding) ? result : result + lineEnding | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good test coverage for CRLF line endings! Could we also add a test case with mixed line endings (both CRLF and LF in the same content) to ensure the function handles this edge case correctly? This would catch the potential issue I mentioned about line ending detection.