Repository navigation
side_diff: fix the layout of unchanged rows, blank lines and cut tabs - #301
Draft
sami-daniel wants to merge 19 commits into
Draft
sami-daniel wants to merge 19 commits into
sami-daniel wants to merge 19 commits into
Conversation
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.
`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.
`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`.
`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.
The assertions state the widths already, and the surrounding test name carries the tab size.
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.
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.
Superscript digits do not survive every terminal or editor the file gets read in.
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.
Cut the comment from five lines to two. The rewrite also drops the misspelled words that cspell would flag.
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.
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.
`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.
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.
`carriage_return_at_the_widest_line_and_tab_size` only checked that `diff` does not abort. It now also compares the rendered output.
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.
The left line was padded up to a fixed column and the right one written after it. That caused three faults: - the right half of an unchanged row started at half_width + 3 instead of column two, so it was misplaced at any width other than 130 and with --expand-tabs - a blank line common to both files was padded instead of left empty - a tab reaching the end of a half was dropped and the text after it was still printed, instead of the row being cut there Replace process_half_line with print_half_line, which prints only the text of a half and returns the column where the output stopped, and make push_output pad from that column to the marker and to column two. print_half_line keeps two positions, the column the text has reached and the column the output has, so that what follows a cut tab is cut too. A carriage return goes back to the start of the half, a backspace moves one column back, and control characters and bytes that are not a character take no column. Closes uutils#300.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #300.
Depends on #270.
Side-by-side padded the left half up to a fixed column and wrote the right half after it. That fixed column was
half_width + 3for an unchanged row, which is column two only at the default--width=130. At any other width, and with--expand-tabs, the right half started in the wrong place. A blank line common to both files was padded the same way instead of left empty.process_half_lineis replaced byprint_half_line, which prints only the text of a half and returns the column where the output stopped.push_outputpads from that column to the marker and to column two, so the padding no longer depends on which kind of row it is.A tab that reached the end of the half was dropped and the text after it was still printed.
print_half_linetracks the column the text has reached apart from the column the output has, so what follows a cut tab is cut too.This changes the output of
-y. With the example above the rows becomea^I^I^Ia$, an empty line, andc^I^I^Ic$. The default width without-tis the one case where unchanged rows were already in place.Three other things follow from the rewrite. A carriage return goes back to the start of its half, a backspace moves one column back, and control characters and bytes that are not a character take no column. An invalid byte used to count as one column.
The
process_half_lineunit tests are replaced by ones forprint_half_line.