Keep original file if -c or --stdout is given

[dirkmueller]

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.

[facebook-github-bot]

Hi @dirkmueller!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[Cyan4973]

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.
That's something we could also do ourselves if you don't feel comfortable to.

[dirkmueller]

@Cyan4973 Thank you for the quick review. I added a testcase to tests/cli-tests/compression/basic.sh. let me know if that works for you.

[Cyan4973]

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

[dirkmueller]

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]

actually I realize after 169f8c1 I can remove the gzip symlinking logic and just call gzip directly.

[terrelln]

@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 cli-tools/bin so that it is still possible to call the system gzip.

[dirkmueller]

@terrelln you're right. I've discovered $ZSTD_SYMLINK_DIR and changed it to use that. how about that?

[terrelln]

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!

[dirkmueller]

@terrelln tests updated accordingly. I think this behavior is no different than using --rm and -k . I was a bit hesitant to hardcode this behavior in tests as afaik no user is relying on this behavior. we can still change the semantics.

However now the tests cover that case as you suggested.

[dirkmueller]

hey y'all, I'd like to move this patch forward. anything I can do to get another round of review? @terrelln

[Cyan4973]

This looks good to me.
It's indeed proper behavior when combined with gzip symlink.

[dirkmueller]

@terrelln are you good with the change as well?

[terrelln]

Thanks for the PR @dirkmueller!