Bail from unfolding when the old blob no longer resolves

What does this MR do and why?

LinesUnfolder takes its blob from diff_file.old_blob. That resolves to nil when the path recorded in a diff note's position no longer exists at the recorded base_sha, which most often happens after the file has been renamed or moved. unfold_required? then reached @position.old_line > @blob.lines.size and raised NoMethodError: undefined method 'lines' for nil.

The failure surfaced a long way from its cause. The unfold happens while serializing a single note, so one stale note took down the entire discussions.json response and the merge request could load none of its discussions, rather than losing diff context on one comment.

Counting the lines through @blob&.lines&.size.to_i treats a missing blob as empty, so the bounds check immediately below bails exactly as it already does for a line past the end of the blob. Nothing else needed to change: the other branch of that check reads blob_data, which already goes through @blob&.data, so it was nil safe. This brings the two branches in line with each other.

The issue proposes a second layer, marking the note outdated and surfacing a message in the UI. That needs a product and UX decision and frontend work, so it is deliberately left out here. This change is scoped to stopping the 500.

References

Differences

Before: loading discussions on a merge request that carried a diff note on a since-renamed file returned HTTP 500. NoMethodError: undefined method 'lines' for nil was raised in lines_unfolder.rb, and because it happened during serialization, no discussion on the merge request rendered at all.

After: the note's blob is treated as empty, unfolding bails the same way it does for a position beyond the end of a blob, and the discussions endpoint responds normally. The stale note loses its unfolded diff context; every other discussion on the merge request is unaffected.

How to set up and validate locally

bundle exec rspec spec/lib/gitlab/diff/lines_unfolder_spec.rb

The new context is "when the old blob no longer resolves at the recorded base_sha", which stubs old_blob to nil and expects unfold_required? to be false and unfolded_diff_lines to be nil rather than an exception. It covers both states of unfold_diff_note_large_blob: the flag defaults to disabled, which is the branch that raised, and a nested context pins the enabled branch as well.

End to end, add a diff note to a file in a merge request, rename that file so the recorded old_path no longer resolves at base_sha, then load GET /:namespace/:project/-/merge_requests/:iid/discussions.json. It returns 200 rather than 500.

Edited by Jeston Singh

Merge request reports

Loading
Loading