Skip to content

head: fix bug reading back through files #7248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
sylvestre merged 2 commits into from
Feb 3, 2025
Merged

head: fix bug reading back through files #7248

sylvestre merged 2 commits into from
Feb 3, 2025

Conversation

@karlmcdowall
Copy link
Contributor

@karlmcdowall karlmcdowall commented Feb 1, 2025

Fix issue #7247.
Rework logic for reading/seeking backwards through files. Bug was seen when reading back through large files. Added test case to validate fix.

@karlmcdowall karlmcdowall marked this pull request as draft February 1, 2025 13:50
@karlmcdowall karlmcdowall marked this pull request as ready for review February 1, 2025 18:06
@karlmcdowall karlmcdowall mentioned this pull request Feb 1, 2025
Merged
@sylvestre
Copy link
Contributor

sylvestre commented Feb 2, 2025

how can we reproduce the issue at the level of the command? thanks :)

@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Feb 2, 2025

If you run

seq 70000 > log
head -n-65000 log

you should see the issue.

With GNU head you'll get the correct output...

<snip>
4998
4999
5000

With the Rust implementation you'll see

<snip>
4001
4002
4

@sylvestre
Copy link
Contributor

sylvestre commented Feb 2, 2025

maybe add such test here https://github.com/uutils/coreutils/blob/main/tests/by-util/test_head.rs ? :)

@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Feb 2, 2025

Sounds reasonable :)

@github-actions
Copy link

github-actions bot commented Feb 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/usage_vs_getopt (passes in this run but fails in the 'main' branch)

sylvestre
sylvestre reviewed Feb 3, 2025
View reviewed changes
tests/by-util/test_head.rs
}

#[test]
fn test_read_backwards_lines_large_file() {
Copy link
Contributor

@sylvestre sylvestre Feb 3, 2025

Choose a reason for hiding this comment

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

sorry, I would prefer to create these files on the fly, no need to add big files in the tree :)
and I guess you can reproduce it with only 1 => 50 ?

Copy link
Contributor Author

@karlmcdowall karlmcdowall Feb 3, 2025

Choose a reason for hiding this comment

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

Updated the test code to generate the files on the fly. Files need to be pretty big since the bug is only seen for backward searches >128K (i.e. double the BUF_SIZE value in head.rs)

@github-actions
Copy link

github-actions bot commented Feb 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Feb 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@karlmcdowall
head: fix bug reading back through files
be071d5 
Fix issue uutils#7247.
Rework logic for reading/seeking backwards through files.
Bug was seen when reading back through large files.
Added test case to validate fix.
@karlmcdowall
Added additional test case
3ceb338
@github-actions
Copy link

github-actions bot commented Feb 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@sylvestre sylvestre merged commit f94ff78 into uutils:main Feb 3, 2025
63 of 65 checks passed
@karlmcdowall karlmcdowall mentioned this pull request Feb 24, 2025
Closed
@moonfruit moonfruit mentioned this pull request Mar 10, 2025
Closed
@karlmcdowall karlmcdowall deleted the head_nth_line_bug branch July 5, 2025 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre sylvestre left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@karlmcdowall @sylvestre