Skip to content

stdin multiple file fixes#3222

Merged
yoniko merged 2 commits intofacebook:devfrom
yoniko:stdin-multiple-files-fix
Jul 29, 2022
Merged

stdin multiple file fixes#3222
yoniko merged 2 commits intofacebook:devfrom
yoniko:stdin-multiple-files-fix

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�%

terrelln
terrelln approved these changes Jul 29, 2022
View reviewed changes
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

tests/cli-tests/compression/multiple-files.sh Outdated
cat file2

echo "zstd -d ./file1.zst - file2.zst -c"
echo "stdin" | zstd | zstd -d ./file1.zst - file2.zst -c No newline at end of file
Copy link
Contributor

@terrelln terrelln Jul 29, 2022

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

@yoniko yoniko Jul 29, 2022

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
@yoniko yoniko mentioned this pull request Jul 29, 2022
Closed
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terrelln terrelln terrelln approved these changes

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoniko @terrelln @facebook-github-bot