Tcrypt/diff parity fixes - #2987
Conversation
Before running Myers, a frequent line is discarded only when it sits inside a
run of lines that match nothing. Git decides that in xdl_clean_mmatch
(xdiff/xprepare.c), where the backward and forward scans each start their
frequent-line counter at 1, so the line under test contributes 2 to the total:
for (r = 1, rdis0 = 0, rpdis0 = 1; (i - r) >= s; r++) { ... }
for (r = 1, rdis1 = 0, rpdis1 = 1; (i + r) <= e; r++) { ... }
rdis1 += rdis0; rpdis1 += rpdis0;
return rpdis1 * XDL_KPDIS_RUN < (rpdis1 + rdis1);
should_prune_common_line started both counters at 0 and started its forward
loop beginning at the line itself, so it contributed 1. The threshold for
discarding was therefore an unmatched run longer than 3 rather than longer
than 6, and a discarded line can never be matched, costing one removal and
one insertion against git's answer each time.
…osqrt
sqrt() rounds the halved bit count down, where git's xdl_bogosqrt rounds it up:
for (i = 1; n > 0; n >>= 2) i <<= 1;
For every odd bit length it halves the result. A 450-line file gets a limit
of 16 instead of git's 32, so a line occurring 27 times is treated as too
frequent to be worth matching and becomes a candidate for discarding, while git
treats it as ordinary and matches it. The same value is the cost ceiling the
Myers search gives up at, which was likewise half of git's.
…t does Two bugs that were propping each other up. git tracks end of file positionally. measure_split sets end_of_file when `split >= nrec`, meaning the split sits past the last line, and that flag is the only thing END_OF_FILE_PENALTY keys off. Indents had no such flag, so score() inferred it from content instead: `next_indent == BLANK && trailing_blanks == 0`.
There was a problem hiding this comment.
🟡 Changes recommended
There are still a couple of confirmed edge-case/parity correctness issues (overflow risk in window bound calculation and a MAX_BLANKS boundary mismatch) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves parity between gix-imara-diff’s diff behavior and Git’s xdiff implementation by aligning a few heuristic details that influence matching/discard decisions and split scoring.
Changes:
- Adjust frequent-line pruning (
should_prune_common_line) to count the candidate line consistently and to scan a full forward window. - Update the
sqrtapproximation to match Git’sxdl_bogosqrtrounding behavior and avoid overflow by clamping. - Refine slider-heuristic split scoring by tracking EOF explicitly and by fixing how trailing blanks are measured; add regression tests for the parity cases.
File summaries
| File | Description |
|---|---|
gix-imara-diff/src/util.rs |
Updates sqrt() to round like Git’s xdl_bogosqrt and clamp instead of overflowing. |
gix-imara-diff/src/tests.rs |
Adds parity-focused regression tests for frequent-line handling and sqrt() behavior (including large inputs). |
gix-imara-diff/src/slider_heuristic.rs |
Improves split-context accounting (EOF flag + trailing blank counting) and adds a focused test for trailing blanks. |
gix-imara-diff/src/myers/preprocess.rs |
Tweaks frequent-line pruning candidate counting and forward-scan window size; adds targeted tests. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f48f94d38f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
|
That's great, thank you! CC Christoph Rüßler (@cruessler) as it might help getting more |
I admit that I am completely out of my depth here and I just assume this makes things better. We'd really need an easy way to run conformance tests. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <[email protected]>
|
Christoph Rüßler (@cruessler) While I am out of my depth here, could you do a before/after run of the blame-with-slider tests? They should be better now, after all, and not worse, or at least unchanged to be able to merge this. |
|
Great, thanks for the contribution! That’s an area I’m working on myself as well. In fact, my next PR would have addressed the |
|
Thanks, I will definitely wait for your review 🙏. |
Fix 3 parity issues between gitoxide and git diffs
I found three places where gitoxide's Myers implementation differs from Git's C implementation:
should_prune_common_lineundercounts candidate lines and scans one less following line.sqrtrounds bit-counts down while git'sxdl_bogosqrtrounds them up.sqrteither panics (in debug) or returns 0 (in release).at_token.I've put them into a single PR assuming that would be easiest for review, but if you'd prefer them to be split just let me know.
More complete explanations
(Samples taken from git commit
3cb9185f65)1. Candidate count and forward window in
should_prune_common_lineFrom
xdiff/xprepare.c:Both
rpdiscounters start at 1 and neither loop visits indexi, so the candidate contributes exactly 2. The current gitoxide implementation only counts it in the forward scan, leading to a count of 1.Git also caps
eati + XDL_SIMSCAN_WINDOW, then scans fromi + 1whilei + r <= e. That covers 100 lines after the candidate. The Rust scan starts at the candidate itself, but its exclusive end waspos + WINDOW_SIZE, so it only covered 99 lines after it. Extending the end by one makes the windows match.2. The sqrt approximation rounds down instead of up as in
xdl_bogosqrtFrom
xdiff/xutils.c:24-34:idoubles once per iteration and the loop runsceil(bit_length(n) / 2)times, so the result is1 << ceil(bit_length(n)/2).gix-imara-diff'ssqrtfunction does a single integer division ofbit_length(n)by 2, so its result is1 << floor(bit_length(n) / 2).2b. Overflow starting at 2^63 (2^62 with the
floor->ceilchange from above)This is unrealistic to hit in practice, but it doesn't cost much to improve. If it's not wanted and the other changes are I'd be happy to remove it.
With a 64-bit input the result of
sqrt/bogosqrtis1 << 32; a 33-bit integer thatbogosqrtcan safely represent because it returnsuint64_ts. Currentlysqrtoverflows which panics in debug mode and returns 0 in release. There are no great options for handling this, but I think the one that's most pragmatic is to saturate to u32::MAX which avoids changing to a u64 return type, avoids panics in debug, and makes the difference because the two implementations 1 instead of (2^32)+1 in release.3. Inaccurate tracking of the end of file position and trailing blanks in
at_tokenTwo pieces of
xdiff/xdiffi.c.measure_split:end_of_fileis set from the position alone, andpost_blankis only ever incremented when walking blank lines; it's never assigned the index. When the loop runs off the end,post_blankisnrec - split - 1.And
score_add_split:The flag is the only input to
END_OF_FILE_PENALTYand nothing is inferred from indent content.MAX_BLANKSis 20 at line 404, matching the Rust constant. Theat_eofpath in Rust returns(0, BLANK), which is what git's loop ends at whensplit >= nrec.