Skip to content

Conversation

@Misakait
Copy link
Contributor

@Misakait Misakait commented Oct 5, 2025

Summary

This PR fixes a layout bug in ptx's traditional mode (-G). When references are enabled, the available line width was not being correctly adjusted, causing text wrapping to be inconsistent with the GNU ptx reference implementation.

The Bug

In traditional mode with references (-G -A), GNU ptx performs text wrapping on long lines. The uutils implementation failed to replicate this behavior because it was not correctly reducing the available line width to account for the space occupied by the reference column.

This resulted in an overly large layout budget, which prevented wrapping and produced output that was inconsistent with GNU ptx.

Changes Made

  • The configuration logic is updated to subtract the maximum reference length from config.line_width when references are enabled in traditional mode. This provides the correct layout budget to the text chunking algorithm.
  • Add related regression test

GNU (Correct edition)

charlotte@Misakait ~ at 21:14:45
❯ echo "Hello World Rust is good language" | ptx -G -A
.xx "language" "" "Hello World Rust is good" "" ":1"
.xx "" "Hello World" "Rust is good language" "" ":1"
.xx "" "Hello" "World Rust is good language" "" ":1"
.xx "" "Hello World Rust is" "good language" "" ":1"
.xx "" "Hello World Rust" "is good language" "" ":1"
.xx "" "Hello World Rust is good" "language" "" ":1"
image

Before fix

wanan@Misakait coreutils/src/uu/ptx on  fix/reference-calculate [$?] is 📦 v0.2.2 via 🦀 v1.90.0 at 21:41:00
❯ echo "Hello World Rust is good language" | cargo run -- -G -A
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
     Running `/home/wanan/coreutils/target/debug/ptx -G -A`
.xx "" "" "Hello World Rust is good language" "" ":1"
.xx "" "Hello World" "Rust is good language" "" ":1"
.xx "" "Hello" "World Rust is good language" "" ":1"
.xx "" "Hello World Rust is" "good language" "" ":1"
.xx "" "Hello World Rust" "is good language" "" ":1"
.xx "" "Hello World Rust is good" "language" "" ":1"
image

After fix

wanan@Misakait coreutils/src/uu/ptx on  fix/reference-calculate [$!?] is 📦 v0.2.2 via 🦀 v1.90.0 took 2s at 21:37:36
❯ echo "Hello World Rust is good language" | cargo run -- -G -A
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `/home/wanan/coreutils/target/debug/ptx -G -A`
.xx "language" "" "Hello World Rust is good" "" ":1"
.xx "" "Hello World" "Rust is good language" "" ":1"
.xx "" "Hello" "World Rust is good language" "" ":1"
.xx "" "Hello World Rust is" "good language" "" ":1"
.xx "" "Hello World Rust" "is good language" "" ":1"
.xx "" "Hello World Rust is good" "language" "" ":1"
image

@github-actions
Copy link

github-actions bot commented Oct 5, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 5, 2025

CodSpeed Performance Report

Merging #8820 will not alter performance

Comparing Misakait:fix/reference-calculate (0dc061f) with main (788bf92)

Summary

✅ 79 untouched
⏩ 73 skipped1

Footnotes

  1. 73 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

context_regex: String,
line_width: usize,
gap_size: usize,
max_ref_len: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding max_ref_len as a property of Config? It's used in a single place in write_traditional_output and it looks like you could use a local var instead. Maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and good question!
The reason I added max_ref_len as a Config property is that I’m preparing for the upcoming GNU-extension mode — I plan to use this max_ref_len in the code where I implement the GNU-extension logic later. I wanted to set it up in advance to keep the code structure consistent once that mode is added.
But I’m not entirely sure if this is the right approach. Should I instead remove max_ref_len from Config for now, and just use a local variable in write_traditional_output until the GNU-extension mode is actually implemented? I’d appreciate your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback—I have changed it to use a local variable as you suggested.

@sylvestre
Copy link
Contributor

sorry but why did you add screenshots ? they duplicate the information no?

@Misakait
Copy link
Contributor Author

Misakait commented Oct 5, 2025

sorry but why did you add screenshots ? they duplicate the information no?

No worries at all—great point! I added the screenshots mainly because I thought visualizing the output (like the aligned text or diff results) would make the context more intuitive and easier to follow at a glance, especially for showing formatting details that might feel abstract in plain text.
But I totally get the concern about duplication! Since screenshots won’t render in command-line environments (where this code might be used or reviewed), I made sure to also include the text descriptions—so folks checking in terminals still have clear, accessible information, while the screenshots act as a quick visual aid for those viewing the docs in a graphical interface.
I can adjust this if the duplication feels unnecessary, though! Would it work better to keep just the text (with clear formatting notes) or maybe link the screenshots separately instead?

…nd add related test

In traditional mode (-G) with references enabled, `uutils/ptx` failed
to wrap long lines in the same way as the GNU `ptx` reference
implementation.

This was due to the layout algorithm operating on an incorrectly large
line width, as the space for the reference column was not being
subtracted from the total width budget.

This commit implements the correct line width adjustment by subtracting
the reference width. This aligns the wrapping behavior and
makes the output identical to GNU `ptx` for the tested cases.
@Misakait Misakait force-pushed the fix/reference-calculate branch from 0dc061f to 4b11638 Compare October 5, 2025 16:18
@github-actions
Copy link

github-actions bot commented Oct 5, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Misakait Misakait requested a review from cakebaker October 6, 2025 14:53
@cakebaker cakebaker merged commit cd2e64d into uutils:main Oct 7, 2025
51 checks passed
@cakebaker
Copy link
Contributor

Thanks!

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.

3 participants