Skip to content

fold: do not fold a bytewise line that is exactly the width - #14189

Open
Socialpranker wants to merge 1 commit into
uutils:mainfrom
Socialpranker:fold-bytewise-exact-width
Open

fold: do not fold a bytewise line that is exactly the width#14189
Socialpranker wants to merge 1 commit into
uutils:mainfrom
Socialpranker:fold-bytewise-exact-width

Conversation

@Socialpranker

Copy link
Copy Markdown

In byte mode (-b), a line that is exactly width bytes long is wrapped
anyway when -s is given:

$ printf 'aaa bbb\n' | fold -bs -w 7   # GNU
aaa bbb
$ printf 'aaa bbb\n' | ./fold -bs -w 7  # this build
aaa 
bbb

fold_file_bytewise buffers width + 1 bytes and looks for a natural line
end inside line[..width]. For a line of exactly width bytes the
terminating newline sits at index width — one byte past that slice — so it is
never found, and the code falls through to the fold path. Without -s the
if line[end] != NL guard happens to cover it, because end == width; with
-s the break moves back to the last blank and the guard looks at the wrong
byte, so the line is split early and the blank is left dangling at the end of
the output line.

Extending the newline search by that one lookahead byte fixes it. The guard on
the fold path then becomes unreachable — end <= width, and a newline in
line[..=width] has just been ruled out — so it is dropped and the newline is
written unconditionally.

Real-world instance, /usr/share/doc/apt/copyright on a Debian system, where
this line is exactly 60 bytes:

$ diff <(fold -bs -w 60 copyright) <(./fold -bs -w 60 copyright)
90c90,91
<  1. Redistributions of source code must retain the copyright
---
>  1. Redistributions of source code must retain the 
>  copyright

Testing: three new tests in tests/by-util/test_fold.rs cover a whole line
of exactly the width, the remainder of a folded line landing on exactly the
width, and — as a control — a line one byte longer that must still fold. The
first two fail on current main and pass here; the third passes either way.
The full fold suite is green (94 passed), cargo fmt --all --check and
cargo clippy -p uu_fold --all-targets are clean.

Release builds of main and of this branch were also run against the system
fold inside debian:stable-slim over 60 files from /usr/share/doc and
/etc, with flags -b, -bs, -s and none, at widths 1, 2, 3, 5, 7, 8, 11,
20, 40, 60 and 80 — 2640 invocations. 468 of them go from differing to
byte-identical, and none regress. Every remaining difference is in the
character/column path, which this PR does not touch; byte mode (-b, -bs,
1320 invocations) now matches GNU everywhere in that sweep.

Behaviour was established by observing GNU fold's output on a Debian system —
no GNU source was consulted.

In byte mode the newline that terminates a line of exactly `width` bytes
sits one byte past the chunk being inspected, so it was never seen as a
natural line end. With -s the line was then broken at its last blank,
producing an early wrap and a trailing space. Include the lookahead byte
in the newline search; the "next byte is a newline" guard on the fold
path becomes unreachable and is dropped.
@sylvestre

Copy link
Copy Markdown
Contributor

Please avoid writing such longs comment 0.
It does not provide much value

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/misc/io-errors (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/retry (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/tail-n0f (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/rm/many-dir-entries-vs-OOM is now being skipped but was previously passing.
Note: The gnu test tests/unexpand/bounded-memory is now being skipped but was previously passing.
Congrats! The gnu test tests/csplit/csplit-heap is now passing!
Congrats! The gnu test tests/seq/seq-epipe is now passing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants