fix for -r on empty directory #3027
Conversation
-r on empty directory resulted in zstd waiting input from stdin. now zstd exits without error and prints a warning message explaining why no processing happened (no files or directories to process).
fix for error message in recursive mode for an empty folder
|
Thanks for the PR @brailovich! I definitely agree with the behavior you want. We definitely don't want to use stdin when it is a TTY. There is a small question of whether we want to use stdin when it isn't a TTY. However, gzip doesn't. So I think that this behavior is fine. |
programs/zstdcli.c
Outdated
| stdin and stdout should not be used */ | ||
| if (nbInputFileNames > 0 ){ | ||
| DISPLAYLEVEL(2, "please provide correct input file(s) or non-empty directories -- ignored \n"); | ||
| CLEAN_RETURN(2); |
There was a problem hiding this comment.
This should also be CLEAN_RETURN(1);
There was a problem hiding this comment.
Actually, if we want the CLI to return without erroring in this scenario, it should be CLEAN_RETURN(0).
|
Agreed with @terrelln comments, Consider adding a test, to ensure this behavior will be preserved across future changes by future committers. |
|
tests added, return values changed. |
tests/playTests.sh
Outdated
| # combination of -r with empty folder | ||
| mkdir -p tmpEmptyDir | ||
| zstd -r tmpEmptyDir 2>tmplog2 | ||
| if [grep "aborting" tmplog2]; then |
There was a problem hiding this comment.
minor sh error here with [ and ]
There was a problem hiding this comment.
thanks, updated!
tests/playTests.sh
Outdated
| # combination of -r with empty folder | ||
| mkdir -p tmpEmptyDir | ||
| zstd -r tmpEmptyDir 2>tmplog2 | ||
| if [ grep "aborting" tmplog2 ]; then |
There was a problem hiding this comment.
Seems this construction is still problematic for sh compatibility.
I'm no sh expert, and this specific construction is not used in our test script,
but I can recommend an existing idiom that has worked well so far :
zstd -r tmpEmptydir 2>&1 | grep "aborting" && die "Should not abort on empty directory"
This would be equivalent, but you could go for something even simpler :
zstd -r tmpEmptydir
this second test only relies on return code being successful, which, I believe, is the goal of the test ?
There was a problem hiding this comment.
thanks! my initial thought was to go with just zstd -r tmpEmptydir but I thought that the test should be more verbose. if this is acceptable, that's the best solution, thanks!
There was a problem hiding this comment.
btw, "if then" sh syntax allows cleaning tmp dirs/files before exiting.
combination of -r with empty folder simplified to comply with sh compatibility tests
Cyan4973
left a comment
Cyan4973
left a comment
There was a problem hiding this comment.
Looks good to me ! Thanks @brailovich for your contribution !
4 participants
fix for -r on empty directory resulted in zstd waiting for input from stdin. this fix makes zstd exit without erroring
but printing a warning message explaining that there were no files or non-empty directories to process.