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-generatedpreviously 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-generatedmarking, 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_diffserves bothdiffanddiff_from_parent, so commit and compare views stream through it unconditionally. Merge request diffs reach it only whenignore_whitespace_changeis set or the persisted diffs were purged.DiffHelper#hide_whitespace?is true for?w=1and 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)
sizeis 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_patchkeeps the shape it has on master. It asks the stitchersingle_file?instead of reading@iterator.size == 1.expand_diff?applies: explicit expansion request wins, then agitlab-generatedmarking, then single-file auto-expansion.- After the charset-detection refactor,
expand_diff?is a flag-gated fast path pluslegacy_expand_diff?. Both carry the single-file test and both were updated.
Not in scope
detect_generated_filestruncates todiff_max_files(1000) and returns an empty set onGitlab::Git::CommandErrororResourceExhaustedError. 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, becauseexpand_diff?short-circuits ongeneratedbefore it consultssingle_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#nextandpeekfor the look-ahead. That moves the stream read into a Fiber, andRequestStoreis fiber-local, so theensureinGitlab::GitalyClient::Call#instrument_streamwould silently stop recordinggitaly_duration_sand 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_pathsanddiff_statswould give one, butGitlab::Git::DiffCollectionis deliberately RPC-free (line 3 of the file says so). Passing a count in from callers breaks down atCommit#diffsandCompare#diffs, which have no cheap count and are the callers that !180897 (merged) was about.