Skip to content
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

stdin multiple file fixes #3222

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Conversation

yoniko
Copy link
Contributor

@yoniko yoniko commented Jul 29, 2022

Fixes for #3206 - bugs when handling stdin as part of multiple files.

@yoniko
Copy link
Contributor Author

yoniko commented Jul 29, 2022

I haven't added tests for the cases where we fail on stdin/stdout being a console as our test suite actually doesn't use the console.
Here are the outputs of the manual checks:

❯ ./zstd ./file1 - -c
stdin is a console, aborting
❯ ./zstd - -c
stdin is a console, aborting
❯ zstd - -c
stdin is a console, aborting
❯ ./zstd ./file1 -
stdin is a console, aborting
❯ ./zstd ./file1 - -c
stdin is a console, aborting
❯ ./zstd - -c
stdin is a console, aborting
❯ ./zstd -
stdin is a console, aborting
❯ ./zstd - -f
aaaaa
(�/�X1aaaaa
B�%
❯ ./zstd - ./file1 -f
dwadawd
Compress: 1/2 files. Current: /*stdin*\ Read:     0   B  ==>  0%(�/�XAdwadawd
  2 files compressed :285.71%   (    14   B =>     40   B)
❯ echo "abc" | ./zstd -
stdout is a console, aborting
❯ echo "abc" | ./zstd - -c
(�/�X!abc
-nL�%
❯ echo "abc" | ./zstd ./file1 -
stdout is a console, aborting
❯ echo "abc" | ./zstd ./file1 - -c
(�/�$1file1
C0W(�/�X!abc
-nL�%

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

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

LGTM

cat file2

echo "zstd -d ./file1.zst - file2.zst -c"
echo "stdin" | zstd | zstd -d ./file1.zst - file2.zst -c
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline at end of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@yoniko yoniko merged commit ae46704 into facebook:dev Jul 29, 2022
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants