-
Notifications
You must be signed in to change notification settings - Fork 31
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
Shellcheck fixes #120
base: master
Are you sure you want to change the base?
Shellcheck fixes #120
Conversation
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
Signed-off-by: Alberto Planas <[email protected]>
I know that it's a draft, but in it's current form this PR doesn't make a lot of sense. It looks like an automated script just changed things without any understanding of the code. It's not that these changes would be incorrect per se, but after reading almost half of the patch I haven't seen a single line where this patch would actually fix something (except maybe the So in other words: Some commits are indeed useful (such as removing unused variables / functions), most of change simply don't change anything except for the syntax, and some of them are totally random... |
Signed-off-by: Alberto Planas <[email protected]>
I am using shellcheck to identify groups of issues, as I expect to use it later. Was extremely useful to find bugs in bash.
If you run shellcheck for bash on this code you will have 201 detected issues (errors and warnings). Removing those is helping to identify real problems:
There are more, like some missing quotes that can produce word splitting, and other stuff, but to reach the real issues you cannot navigate thru hundreds on elements in the report.
Those are shellcheck commands ...
Can be ... can you point me one? |
To extend on this, I know that I make some stylistics changes (! -z instead on -n), but others are real issues:
It is OK to drop this, but then I recommend you use shellcheck and navigate into the reported elements and evaluate then case by case: there are real bugs. |
Just look at the first 6 changed lines in https://github.com/openSUSE/transactional-update/pull/120/files: 5 of them are adding a comment without changing anything. These comments either seem to be a TODO, the other ones don't even explain what they want to say, so I wouldn't call that helpful for someone trying to understand the code. Or look at changes such as in line 340 - they simply won't do anything, it's not even a stylistic change. Some of them are also inconsistent, like the one in line 742 - shouldn't it include both variables into the quotation marks?
How are these issues in the context of this script?
You are implying that the code is currently full of bugs, but after a quick review the only potential issue I've seen is not catching the return value when changing directories. I guess your strategy is to make shellcheck happy now so that potential problems in the future will be more visible? In general I'm fine with this, but to be honest I'm not really happy about the fact that the tool is generating so many false positives / enforcing code that isn't changing anything. |
Indeed, using that parameter seems to have been lost somewhere.
I think this should have been changed to
It was reusing the pattern from the other
As I said: It's not that I'm opposing the changes, but it is a real problem that the script is generating so many false positives. |
Yes. It is a reminder for me that I need to convert those variables from strings to array, so
It is a shellcheck directive, they are explained here: https://www.shellcheck.net/wiki/Directive
Not sure why you say that: it is adding quotes to
No. Of course it is also OK to move all between quotes, but because shellcheck proves that only one part varies, it is recommended to do that explicitly.
They are issues in all bash scripts. Please, read the links. For example, for SC2155 will make some SC2166 is more clear: using -a or -o is even advised not to do by POSIX.
No, not at all. I am implying that there are real bugs that can automatically detected but we cannot see that easily: dead code, or live variables that seems to be not initialized maybe breaking a long boolean expression.
And in the present. Please, review the comment #120 (comment) I pointed three that I did not fixed here.
I am sorry, but they are not false positives. You know very well that bash is a tricky language that is sadly full of corner cases. All those elements appointed by the tool are pointing to hidden bombs that can trigger later. For example, you yourself quoted "${SNAPSHOT_ID}", but you expect that will always be a number. You quote that just because is this changes, or a bug is later introduced that make it a string with spaces, something dangerous can happen. Those changes are on the same line. |
There is no single false positive in the report. |
Note: last POSIX 2024 removed -a -o: https://sortix.org/blog/posix-2024/ |
No description provided.