Fix first file in a diff never collapsing when generated

The first file of every Gitaly-streamed diff rendered fully expanded with no "generated" label, even when it matched a gitlab-generated pattern in .gitattributes. Later files collapsed correctly. Gitlab::GitalyClient::DiffStitcher#size returns @diff_count, a counter incremented as the stream is consumed, but Gitlab::Git::DiffCollection#expand_diff? read it as a total (@iterator.size == 1) to detect a single-file diff. For the first patch that value is always 1, so the file got expanded: true, which cancels collapse_generated_file? (generated? && !expanded). Two changes fix this. An explicit gitlab-generated marking now takes precedence over single-file auto-expansion, which is what resolves the report and ships ungated. Separately, DiffStitcher holds each patch until the next one arrives and exposes single_file?, so it answers the single-file question itself instead of DiffCollection misreading a running counter. That half is behind diff_stitcher_single_file_lookahead. Reported in #572763 (closed).

  • Every generated file in a diff now collapses, including the first one and a lone one. A merge request whose only changed file is gitlab-generated previously rendered expanded because single-file auto-expansion overrode the marking. That was a second, long-standing bug fixed here.
  • Precedence is now: an explicit expansion request, then a gitlab-generated marking, then single-file auto-expansion.
  • The generated-file precedence applies on both the Gitaly and persisted paths. The look-ahead affects every Gitaly-streamed diff, which is wider than merge requests: CommitService#call_commit_diff serves both diff and diff_from_parent, so commit and compare views stream through it unconditionally. Merge request diffs reach it only when ignore_whitespace_change is set or the persisted diffs were purged. DiffHelper#hide_whitespace? is true for ?w=1 and for every anonymous visitor, so anyone viewing a public merge request could hit it.
  • The first file of a multi-file Gitaly diff also stops escaping the viewer's 1MB collapse limit, since expanded? gates that too.
Detailed context for AI agents

Single-file detection (lib/gitlab/gitaly_client/diff_stitcher.rb)

  • size is a running count of patches read from the stream so far, not a total. It cannot answer "is this the only file?" until the stream ends.
  • The stitcher now keeps one patch in a look-ahead buffer. A patch is released only after the next one is read or the stream ends, so single_file? is known by the time the first patch is yielded.
  • The overflow marker still stops iteration.
  • If a consumer stops early and iteration resumes over a stream that cannot be replayed, the buffered patch is not lost.

Expansion precedence (lib/gitlab/git/diff_collection.rb)

  • DiffCollection#each_gitaly_patch keeps the shape it has on master. It asks the stitcher single_file? instead of reading @iterator.size == 1.
  • expand_diff? applies: explicit expansion request wins, then a gitlab-generated marking, then single-file auto-expansion.
  • After the charset-detection refactor, expand_diff? is a flag-gated fast path plus legacy_expand_diff?. Both carry the single-file test and both were updated.

Not in scope

  • detect_generated_files truncates to diff_max_files (1000) and returns an empty set on Gitlab::Git::CommandError or ResourceExhaustedError. A very large merge request can therefore still mark fewer files generated than expected. This is a separate failure mode and is not addressed here.

Verification

Specs cover:

  • every generated file collapsing in a multi-patch stream
  • a single-patch stream still auto-expanding
  • the first patch of a multi-patch stream no longer auto-expanding
  • a lone generated file collapsing
  • a lone generated file still expanding when expansion was requested
  • the same precedence on the persisted path
  • the overflow marker stopping iteration
  • no patch lost when a consumer stops early and iteration resumes over a stream that cannot be replayed
  • the flag-gated and legacy expand_diff? paths agreeing

spec/models/diff_viewer/base_spec.rb needed one change, which is not unrelated. Its "when diff is expanded" context never stubbed expanded? and was passing only because of the bug this fixes. It now stubs expanded? explicitly.

Rebased twice onto changes in the same method. The second rebase merged against the charset-detection refactor in !256379 (merged), which is why both expand_diff? paths were updated.

Rollout

  • The look-ahead is behind diff_stitcher_single_file_lookahead (gitlab_com_derisk, default off, actor is the repository container). Off is master's behaviour: single_file? counts patches seen so far, so the first patch of any stream reads as single. Turning it off does not reintroduce the reported bug, because expand_diff? short-circuits on generated before it consults single_file?. Verified by running the suite with the look-ahead disabled: the only failure is the non-generated first-file case. Watch list and steps: #631906
  • Rejected: using Enumerator#next and peek for the look-ahead. That moves the stream read into a Fiber, and RequestStore is fiber-local, so the ensure in Gitlab::GitalyClient::Call#instrument_stream would silently stop recording gitaly_duration_s and call details. Breaking out of external iteration also abandons a suspended Fiber, so gRPC cleanup never runs.
  • Rejected: asking Gitaly for a file count. find_changed_paths and diff_stats would give one, but Gitlab::Git::DiffCollection is deliberately RPC-free (line 3 of the file says so). Passing a count in from callers breaks down at Commit#diffs and Compare#diffs, which have no cheap count and are the callers that !180897 (merged) was about.
Edited by Kai Armstrong

Merge request reports

Loading
Loading