8000 head: fix bug reading back through files by karlmcdowall · Pull Request #7248 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

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

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

karlmcdowall
Copy link
Contributor

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
@sylvestre
Copy link
Contributor

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

@karlmcdowall
Copy link
Contributor Author

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

@karlmcdowall
Copy link
Contributor Author

Sounds reasonable :)

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)

@@ -392,6 +392,29 @@ fn test_presume_input_pipe_5_chars() {
.stdout_is_fixture("lorem_ipsum_5_chars.expected");
}

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

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

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)

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)

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)

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.
Copy link
github-actions bot commented Feb 3, 2025

GNU t 8000 estsuite 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
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
0