GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — side_diff: fix the column layout overflow and `--expand-tabs` by sami-daniel · Pull Request #270 · uutils/diffutils

Conversation

@sami-daniel

@sami-daniel sami-daniel commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #264.

Side-by-side sized its columns with signed arithmetic on the --tabsize value, which is parsed with no upper bound. A large tab size overflowed, and what happened next depended on the build: with -C overflow-checks=on the process aborted, and in release the value wrapped and the columns came out with bogus widths.

$ printf 'a\n' > f1; printf 'b\n' > f2
$ diff -y --tabsize=9223372036854775805 f1 f2
thread 'main' panicked at src/side_diff.rs:85:24:
attempt to add with overflow

A tab size past the signed range never aborted. It cast to a negative number and sized the columns as if the tab size were small, so the output looked ordinary and was wrong. The calculation is now unsigned throughout and holds for every input on its own, instead of relying on validation upstream.

--expand-tabs was accepted but not taken into account when sizing the columns. Column two has to start on a tab stop only so that tabs in the right column keep their position relative to the stops, and once tabs are expanded there are none left to keep aligned.

This changes the output of -y -t. At the default width the separator moves from column 62 to 64.

That wider half line exposed a third overflow, introduced and fixed in the same commit. A large tab size used to leave the half line empty, so nothing was ever drawn; with -t the line now fits, and the sums that walk to the next tab stop overflowed. They measure the step against the room that is left instead of adding absolute columns.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sami-daniel sami-daniel changed the title side_diff: fix the column layout overflow side_diff: fix the column layout overflow and --expand-tabs Aug 13, 2026
@sami-daniel
sami-daniel marked this pull request as ready for review August 13, 2026 03:43
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

GNU diffutils testsuite comparison:

Test results comparison:
  Current:   TOTAL: 33 / PASSED: 0 / FAILED: 33 / SKIPPED: 0
  Reference: TOTAL: 33 / PASSED: 8 / FAILED: 21 / SKIPPED: 4

Changes from main branch:
  TOTAL: +0
  PASSED: -8
  FAILED: +12

New test failures (12):
  - basic
  - bignum
  - brief-vs-stat-zero-kernel-lies
  - bug-64316
  - cmp
  - diff3
  - help-version
  - large-subopt
  - strcoll-0-names
  - strip-trailing-cr
  - timezone
  - y2038-vs-32bit
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread Cargo.toml Outdated
predicates = "3.1.0"
rand = "0.10.0"
tempfile = "3.26.0"
test-case = "3.3.1"

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need a new dependency for 3 tests? a simple loop would do

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1c1fb0c. But I'd keep the dependency because it looked really bad lol. We can revert the commit if need be. I've seen other places in the tests that could also make good use of this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread fuzz/fuzz_targets/fuzz_side.rs Outdated
// return;
// }
// We can't fuzz with width equals to usize, otherwise we
// would could have 2^64 - 1 of padding columns, which means

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this comment shorter
it also has typos ("would could", "nescessary", "perfomance"), cspell will complain

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 326eaee.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread src/side_diff.rs
}

// gnu sdiff do not tabulate the hole empty right line, instead, just keep the line empty
if !is_right {

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this comment (and the one in diff())? it isn't related to the fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the diff() one in 06445df and added the gnu sdiff one in 6a270f2. I thought it was a bad idea to mention gnu sdiff there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread src/side_diff.rs
}

#[test]
fn expanded_tabs_widen_the_half_line() {

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this duplicates default_width_and_tab_size and the test above it, no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just part of it. Removed the first half in 5b26b67.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread src/side_diff.rs
}

#[track_caller]
fn assert_renders(from: &[u8], to: &[u8], width: usize, tabsize: usize) {

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't assert anything, it only checks that it doesn't panic
could you please check the output, at least for a few cases?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the idea is exactly not to panic. There isn't much more to it than that, but it's fair. I've also added assertions. Done in 36b3d9b.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread src/side_diff.rs Outdated
}

#[test]
fn carriage_return_at_the_widest_line_and_tab_size() {

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no assertion here either

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was also just to confirm that I wouldn't panic. Done in 9b5ab11.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sylvestre
sylvestre reviewed Oct 4, 2026
View reviewed changes
Comment thread src/side_diff.rs Outdated
#[test_case(b"aaa\tbbb\n", b"aaa\tccc\n" ; "tabs on both sides")]
#[test_case(b"\t\t\n", b"\n" ; "tabs against an empty line")]
#[test_case("\u{4f60}\u{597d}\t\u{1f600}\n".as_bytes(), b"a\n" ; "wide and multibyte")]
fn every_small_width_and_tab_size_renders(from: &[u8], to: &[u8]) {

@sylvestre sylvestre Oct 4, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

96 * 96 * 2 * 3 diffs, how long does this take in debug?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sami-daniel sami-daniel Oct 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.43s, 0.38s, and 0.37s respectively

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sami-daniel
sami-daniel requested a review from sylvestre October 6, 2026 01:40
@sami-daniel sami-daniel mentioned this pull request Oct 6, 2026
side_diff: fix the layout of unchanged rows, blank lines and cut tabs #301
Draft
@sami-daniel

sami-daniel commented Oct 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Could I open a stack instead of several separate small PRs?

The main difficulty I'm facing with the remaining side-by-side work is the waiting time between PRs. For example, I already have the "|" marker ready, but I have to wait for #301 before I can send it, and the "/" and "" markers, "(" and ")", and options like "--suppress-common-lines" come after that. They all change the same two functions in "src/side_diff.rs", so each one has to wait for the one before it.

The code is mostly written, and most of the time goes into waiting between reviews. Using a stack would let me keep the changes small and reviewable while continuing to work on the next ones without having to wait for each PR to be merged.

I think this would be healthier for a small project like this one and would let me bring "diff -y" to the same output as gnu diff in one go, with everything except interactive mode. I’d really like to speed this up because the other diffs are well underway but this mode itself is still quite incomplete.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sylvestre

sylvestre commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

hi, this one can't be merged as it is:

  • it needs a rebase
    could you please have a look? otherwise, i will close it, thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sami-daniel added 16 commits November 6, 2026 19:58
@sami-daniel
build(deps): add test-case as a dev-dependency
4f1c66e
The side-by-side layout tests need parameterized cases so that each
width and tab size shows up as its own named test instead of a loop
that reports a single failure with no indication of which input broke.
@sami-daniel
fix(side-diff): compute the column layout without signed casts
94a4d38
`Config::new` sized the side-by-side columns with signed arithmetic on
the `--tabsize` value, which `params.rs` accepts with no upper bound.
`tab_size + GUTTER_WIDTH_MIN` overflowed for a large one: with
`-C overflow-checks=on` that aborted, and in release it wrapped and drew
bogus widths. A value above `isize::MAX` did not even abort, it cast to a
negative number and silently laid the columns out as if the tab size were
small. Closes uutils#264.

The calculation moves into `Config::layout`, in `usize` throughout, and
holds for every input on its own rather than relying on validation
upstream:

- An early return for `tab_size > full_width`. This is exact, not a
  clamp: the offset is a multiple of `tab_size`, so it is either zero or
  already past the right edge, and neither leaves room for a second
  column. It is also what keeps the sum below in range.
- `saturating_add` for the gutter sum, which covers the one case the
  strict `>` lets through, `full_width == tab_size == usize::MAX`.
- `saturating_sub` for the two bounds that could go negative.
  `full_width = 5, tab_size = 8` really does produce an offset past the
  right edge.
- `usize::midpoint` for the balance point, which cannot overflow.
- `saturating_sub` on `separator_pos`, which underflowed for a zero
  width. Unreachable from the CLI, reachable through the library.

A tab stop every zero columns has no meaning and every reader of
`tab_size` divides by it, so `Config::new` now normalizes it to one. The
layout alone was not enough: fixing it only moved the division by zero
from `layout` to `format_tabs_and_spaces`.

The new `mod layout` covers the boundaries, including the two that pin
the design down: `full_width = 10, tab_size = 8` still leaves two
columns, so no looser guard is correct, and `tab_size == full_width`
yields a non-zero offset with an empty half line.
@sami-daniel
fix(side-diff): honour --expand-tabs in the column layout
8a174b1
`Config::new` took `expanded` but ignored it when sizing the columns. The
manual's "Preserving Tab Stop Alignment" section explains why it matters:
column two has to start on a tab stop only so that tabs in the right
column keep their position relative to the stops. With `--expand-tabs`
there are no tabs left in the output, so there is no grid to preserve and
every column is a stop.

Compared against the output of `diff` from GNU diffutils 3.10. The
separator column now matches it for `-y -t` at widths 40, 80 and 130, and
for `-y -t --tabsize=4 --width=100`: columns 19, 39, 64 and 49. At the
default width it sat on column 62 before, two columns off.

The tab size still decides how far an expanded tab reaches, so it stays in
the field and only `layout` sees the 1. Feeding the 1 into the field would
shrink every expanded tab to a single space.

The widened half line makes a crash reachable. A tab size near
`usize::MAX` used to yield an empty half line, so `process_half_line`
returned before drawing anything; with `-t` the line now fits and three
expressions of the form
`current_width + tab_size - (current_width % tab_size)` overflowed.
`format_tabs_and_spaces` and the tab arm now measure the step to the next
stop against the room that is left instead of summing absolute columns,
which the surrounding `current_width <= max_width` already bounds.

`test_full_width_40_tab_8` used `expanded = true` and expected the widths
computed while ignoring it. The separator lands on column 19 either way,
so only the half width and the offset change.

The new tests were checked by mutation. Dropping the ternary breaks
`expanded_tabs_widen_the_half_line`,
`expanded_tabs_lay_out_as_a_stop_on_every_column` and
`test_full_width_40_tab_8`; applying it to the field instead breaks
`expanded_tabs_keep_the_real_tab_size_for_rendering` and
`expanded_tabs_reach_the_next_real_tab_stop`.
@sami-daniel
test(fuzz): fuzz width and tabsize in the side-by-side target
6166242
`width` and `tabsize` were commented out of `fuzz_side`, along with a
`width == 0 || tabsize == 0` early return that was commented out too,
because the column arithmetic could not take arbitrary values. It can
now, so both are fed to the target and neither guard is needed.

They are `u16` rather than `usize`: the layout handles the whole range,
but a width near `usize::MAX` asks the renderer for petabytes of
padding, which would only produce timeouts.
@sami-daniel
test(side-diff): drop the comment on the expanded layout expectation
93a7ac1
The assertions state the widths already, and the surrounding test name
carries the tab size.
@sami-daniel
test(side-diff): cover the carriage return padding at the widest line
ed163a4
A carriage return pads all the way to column two, the only caller that
reaches the far end of the line. That walk used to add absolute columns
and overflowed with `--width` and `--tabsize` at the top of the range,
which aborts under `-C overflow-checks=on`.

Both values sit at the maximum on purpose: a smaller tab size walks to
that end one stop at a time, and a smaller width never reaches the sum
that overflowed. Expansion stays off, since it pads with spaces one
column at a time and would not finish at this width.
@sami-daniel
test(fuzz): fuzz the whole tab size range in the side-by-side target
12feb6a
A tab size wider than the line collapses the layout to a single column
and draws nothing, so the whole `usize` range costs no more than a small
one. Capping it at `u16` left out the values above `isize::MAX`, which
are the ones the old signed arithmetic turned negative.

The width stays a `u16`, since the padding it asks for is written one
column at a time and the cost scales with it.
@sami-daniel
style(fuzz): write the exponent in the width comment as ASCII
88c35c8
Superscript digits do not survive every terminal or editor the file gets
read in.
@sami-daniel
test(side-diff): replace test-case with plain loops
bd0a9bc
Drop the test-case dev-dependency. Only two layout tests used it, so
`extreme_tab_size_renders` and `every_small_width_and_tab_size_renders`
now iterate over the same inputs in a loop inside a single `#[test]`.

The inputs are unchanged. The cases no longer show up as separately
named tests.
@sami-daniel
style(fuzz): shorten the width comment in the side-by-side target
3e17e59
Cut the comment from five lines to two. The rewrite also drops the
misspelled words that cspell would flag.
@sami-daniel
style(side-diff): restore the comment on the unpadded right line
4b0350b
fc18aa5 deleted the comment above the `if !is_right` block in
`process_half_line` without changing the code it describes. Put it back
verbatim, since it still explains why only the left half is padded.
@sami-daniel
style(side-diff): restore the left and right file notes on diff
25d888b
fc18aa5 deleted the comment under the `diff` signature that marked
`from_file` as the left file and `to_file` as the right one. Bring the
notes back as trailing comments on the two parameters, since the
original caret line no longer lines up with a signature that puts one
parameter per line.
@sami-daniel
test(side-diff): drop the duplicated width 130 layout assertions
1cc188a
`expanded_tabs_widen_the_half_line` repeated two assertions that other
tests already make: the plain layout for width 130 and tab size 8 is
`default_width_and_tab_size`, and the expanded one is in
`expanded_tabs_lay_out_as_a_stop_on_every_column`.

The width 40 pair stays. No other layout test covers it.
@sami-daniel
test(side-diff): assert on the rendered output
b538e92
The render sweeps only proved that `diff` does not panic. They now also
check that the output has no tab when `expand_tabs` is set, for every
width and tab size they cover.

Add `changed_line_output` and `unchanged_line_output`, which compare the
whole output for three cases at tab size 8:

- a changed line at width 130
- a changed line at width 40 with `expand_tabs`
- an unchanged line at width 130

The expected lines match what GNU diff 3.10 prints for a deleted, an
added and an unchanged line at the same widths.
@sami-daniel
test(side-diff): assert the output at the widest line and tab size
aa7a802
`carriage_return_at_the_widest_line_and_tab_size` only checked that
`diff` does not abort. It now also compares the rendered output.
@sami-daniel
test(side-diff): adapt two tests to the gutter marker fix from main
cafc698
The merge brought in the fix for uutils#269, which pads the gutter up to
`separator_pos` before the marker. Two tests no longer held.

- `carriage_return_at_the_widest_line_and_tab_size` ran `diff` with the
  width and the tab size both at `usize::MAX`. That padding is now 2^63
  spaces, so the test never finished. It becomes
  `padding_to_the_end_of_the_widest_line_and_tab_size` and calls
  `format_tabs_and_spaces(0, usize::MAX, ...)` directly, which is the
  walk that used to overflow, and expects a single tab.
- `sdiff_gutter_marker_column` expected the right column at 24 for
  `--width=40 --expand-tabs`, the offset of the layout that ignored
  `--expand-tabs`. This branch lays it out at column 22, which is where
  GNU diff 3.10 puts it.
@sami-daniel
Trigger CI
c5d7be4
@sami-daniel
sami-daniel force-pushed the fix/264-side-diff-layout-overflow branch from 7c69857 to c5d7be4 Compare November 6, 2026 23:00
@sami-daniel

sami-daniel commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@sylvestre Hi, sorry for the late reply. Rebase done!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sami-daniel added 2 commits November 9, 2026 03:20
@sami-daniel
Merge branch 'main' into fix/264-side-diff-layout-overflow
4444cc1
@sami-daniel
Merge branch 'main' into fix/264-side-diff-layout-overflow
bcb1883
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre Awaiting requested review from sylvestre

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

diff: -y --tabsize=<huge> overflows the column arithmetic (overflow-checks only)

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@sami-daniel @sylvestre
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.