-
Notifications
You must be signed in to change notification settings - Fork 136
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
check_for_equality uses numeric vs. string compare #599
Comments
Numeric comparison isn't a problem here. Could you find a simple example that causes the problem you are seeing (ideally run rmlint on only the files that the message is complaining about), and provide the resulting script and the complete output of |
Below are two tgz files demonstrating the issue. There is a ReadMe file outlining what I did, and a typescript log file output by the script command as I did it. These demonstrate the behavior of the unmodified rmlint 2.10.1. You will see the shell complaining that the arguments for -ne are expected to be integers. I did not run it with my substituting != for -ne, but in this case the shell will find that 0 != 0 is true. There are two tar files because I discovered that the generated rmlint .sh file specifies /bin/sh for an interpreter (which is the right thing to do) and that debian links /bin/sh to /bin/dash. Essentially this happened in 2009, as explained here: https://askubuntu.com/questions/976485/what-is-the-point-of-sh-being-linked-to-dash. I was skeptical, but the reference explains that dash is at least as good if not better adherence to POSIX, which was the goal, and it is alot faster. The second tar archive modifies the generated script to specify the /bin/bash interpreter, which seems to be pretty ubiquitous except for the debian boot shell of dash. The results are the same, except for a second error message that is a little more readable. All software has bugs, but what version of rmlint would you be using if you were in a corner and had to do a series of trimings without error to pull the size down to where you can back it up? I am shooting in the dark. While you cannot make any promises, your guess is much better than my shots in the dark! Thanks. And thanks for what you do. The few troubles I have had not withstanding, this is a great package, incredibly clearly architected and written, and great documentation. |
Blast! I forgot the tgz files! Here they are: |
I fixed the bug. I am attaching: fix_-xv_SlowSteps2.2.log (must be renamed .tgz) which demonstrates the fix exactly as the previous test_with_bash.log demonstrated the bug with bash for the rmlint.sh shell. I have also tested it with /bin/sh which is linked to /bin/dash on debian and ubuntu systems. Solution.png which is a screenshot of a WinMerge showing the three small changes made. I changed check_for_equality from an echo of the cmp or rmlint return value to stdout to a "return" of the cmp or rmlint return value. I don't see the value in converting it into a string to flow through stdout. This makes check_for_equality have the same semantics as any UNIX command: 0 means success, anything else failure, with the result in $?. Also as I expect you know, the original code redirected all output to stdout from the statements grouped by check_for_equality's { }, which in the case of a directory comparison included all stdout from rmlint. This is why it did not match a simple "0". The simple "0" was the last character, but there was a ton of other stuff in that string also! I changed original_check to run check_for_equality prior to the conditional that will check its return value, then had the condition reference $? to expose the return value to compare to a zero. I left in one "set -xv" statement and two "set xv" statements so you can see it happen. I am assuming that I can simply edit lib/sh.sh with these changes and recompile. Is there anything else I should know? Thanks. |
Yeah, you should be able to edit |
As @rnsc5jjjjj (thanks!) already noticed the issue is in the stdout where rmlint sends all output in case of a directory compare. |
If the paranoid mode is selected (-p) original_check calls check_for_equality which runs cmp if they are simple files and rmlint if they are directorys. Both seem to work correctly, but on returning to original check, original check performs a numeric comparison of the two strings: The quoted return value from check_for_equality and a quoted "0":
if [ "$check_for_equality "$1" "$2")" -ne "0" ] ; then
This should use a string compare not equal:
if [ "$check_for_equality "$1" "$2")" != "0" ] ; then
Easy enough to fix. I put "set -xv" and set "xv" around the section) to watch what it did.
It ran everything correctly, coming down to the following test:
[ "0" != "0" ]
however it finds that this expression is true, and executes the "then" clause which prints a message that the files are no longer identical, cancelling the deletion and returning 1 to the caller, where the deletion is not performed.
It looks like everything works correctly except for declaring "0" != "0" is true!
I cannot figure out what is wrong. The round circles are zeros, not ohs.
Any suggestions would be very welcome.
The text was updated successfully, but these errors were encountered: