Keep original file if -c or --stdout is given#3052
Conversation
|
Hi @dirkmueller! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thanks @dirkmueller, I believe that's a good suggestion. One missing aspect in this PR could be to add a test validating the new behavior, and ensuring it remains respected in future changes. |
dirkmueller
force-pushed
the
gzip_keep
branch
2 times, most recently
from
70dd10d to
ad8fc64
Compare
|
@Cyan4973 Thank you for the quick review. I added a testcase to |
dirkmueller
force-pushed
the
gzip_keep
branch
from
ad8fc64 to
296f274
Compare
tests/cli-tests/compression/basic.sh
Outdated
|
|
||
| # Test keeping input file when compressing to stdout in gzip mode | ||
| ln -s "$(type -p zstd)" ./gzip | ||
| ./gzip -c file | zstd -t ; test -f file |
There was a problem hiding this comment.
minor :
if you want to test that both sides of the test are completing fine, prefer && instead of ; separator.
> false ; echo this is successful
this is successful
> false ; true
> echo $?
0
> false && echo this is successful
> false && true
> echo $?
1
There was a problem hiding this comment.
Please test with set -e like these test scripts are using. In my experience the behavior of that with && is odd - && similar to if and while will temporarily turn off errexit mode and simply hide the error. try this example script:
$ cat x.sh
set -e
false | false && true && echo it detected the failure
true && echo "did it really find the false?"
$ sh x.sh ; echo $?
did it really find the false?
0
The behavior we want that the first false return aborts the script (because if zstd -c fails for whatever reason, test -f will also remain trivially true)
dirkmueller
force-pushed
the
gzip_keep
branch
2 times, most recently
from
d2ea3b5 to
15ff5ad
Compare
|
actually I realize after 169f8c1 I can remove the gzip symlinking logic and just call |
dirkmueller
force-pushed
the
gzip_keep
branch
from
15ff5ad to
5ab91d5
Compare
|
@dirkmueller You still need the symlink logic. I believe that this is calling the system gzip. I've avoided putting the zstd gzip symlink in |
dirkmueller
force-pushed
the
gzip_keep
branch
from
5ab91d5 to
1ba2d32
Compare
|
@terrelln you're right. I've discovered |
terrelln
left a comment
terrelln
left a comment
There was a problem hiding this comment.
Sorry for the delay, and thanks for the ping!
@Cyan4973 are you happy if zstd --rm -c keeps the file, and zstd -c --rm removes the file?
If Yann is okay with that, can you also add a test to codify that behavior? E.g.
# Test -c overrides --rm
zstd --rm -c file | zstd -t; test -f file
# Test --rm overrides -c
cp file file-rm
zstd -c --rm file-rm | zstd -t; test ! -f file-rm
Then this PR looks good to me!
Set removeSrcFile back to false when -c or --stdout is used to improve compatibility with gzip(1) behavior. gzip(1) is removing the original file on compression unless --stdout or /-c is used. zstd is defaulting to keep the file unless --rm is used or when it is called via a gzip symlink, in which it is removing by default. Specifying -c/--stdout turns this behavior off.
|
@terrelln tests updated accordingly. I think this behavior is no different than using However now the tests cover that case as you suggested. |
|
hey y'all, I'd like to move this patch forward. anything I can do to get another round of review? @terrelln |
Cyan4973
left a comment
Cyan4973
left a comment
There was a problem hiding this comment.
This looks good to me.
It's indeed proper behavior when combined with gzip symlink.
|
@terrelln are you good with the change as well? |
|
Thanks for the PR @dirkmueller! |
4 participants
Set removeSrcFile back to false when -c or --stdout is used to improve
compatibility with gzip(1) behavior.
gzip(1) is removing the original file on compression unless --stdout or
/-c is used. zstd is defaulting to keep the file unless --rm is used or
when it is called via a gzip symlink, in which it is removing by
default. Specifying -c/--stdout turns this behavior off.