batches: Fix Diffs render incorrectly#39377
Conversation
|
@mrnugget & @camdencheek I would appreciate a review on this since I see your names a lot when it comes to file diffs and this portion of the code. |
camdencheek
left a comment
There was a problem hiding this comment.
This section of code is a mess of off-by-one footguns. I've had this issue sitting around forever because this part of the code scares me. Thank you for the additional tests. I don't think I can provide a meaningful review because it's been too long since my head has been in this code, but I'm happy to stamp a fix that solves your issue, includes tests, and doesn't cause any other test failures.
eseliger
left a comment
There was a problem hiding this comment.
Nice, one more down in this nasty function. Thank you so much for picking this up, truly appreciated!
mrnugget
left a comment
There was a problem hiding this comment.
Similarily to @camdencheek I locked away all previous memories of working with this code in a vault hidden deeply in my mind, so I can't provide meaningful feedback on whether these 3 lines you changed are correct, but I can give you a huge 👍 on the test:code ratio for this part of the codebase.
Closes #35492.
I removed a function at the bottom of a Go file to replicate the rendering issue. What I found was when an empty
-line (not followed by unchanged lines) is removed, this causes the index ofhighlightedBaseandhunkLinesto be off (hunkLinesis behind by one). This is becausehighlightedBasestill includes the empty-line. So eachhunkLinecorresponds to the previous highlighted line.To fix this, I simply increment
baseLineby one if the empty-line is removed.Test plan
Added Go test
Before
After