-
Notifications
You must be signed in to change notification settings - Fork 567
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
WIP: Minor cleanup #4497
WIP: Minor cleanup #4497
Conversation
mkdeb.sh.in
Outdated
@@ -22,7 +22,7 @@ if [ -n "$HAVE_SELINUX" ]; then | |||
CONFIG_ARGS="$CONFIG_ARGS --enable-selinux" | |||
fi | |||
|
|||
TOP=`pwd` | |||
TOP=$(pwd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea, maybe with a fall-back substitution.
TOP=$(pwd) | |
TOP=$PWD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor cleanup
I wouldn't exactly call this minor; there are many unrelated changes in this
PR.
Some commits seem alright on their own, others I'm not so sure.
Anyway, I'd just like to leave a "please don't merge" in here for now until I
can review it properly.
Rebased to fix merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
- use license file from gnu.org
LGTM; bonus points for catching a violation of the GPL license itself (I hadn't
seen this one before).
- fix missing newlines at end of files
LGTM.
I'd personally avoid doing this until there is a commit hook/CI check to detect
such things, but it does not change too many files, so sure.
By the way, what commands have you used to detect/fix this?
- fix spelling
Please either wait for the merge of #4502 (which conflicts with this) so that
it can be a clean revert or make a separate PR for this.
As for the commit itself, there are some leftovers:
$ git grep temporarely
configure:# overlayfs features temporarely disabled pending fixes
$ git grep parrent
src/firejail/usage.c: "\tparrent interfaces.\n"
I have noticed the "parrent" one myself as well on #4502, though it was because
of the above file rather than the zsh completion.
Other than that, LGTM.
- fix shellcheck warnings
NAK; there are still many shellcheck warnings if you run it locally, including
for some common pitfalls, such as the lack of quotes.
Please make a separate PR for this.
- trim excess whitespace
I have some questions and I'd like to discuss this one separately.
Please make a separate PR for this.
Thank you for the review, I will rework the PRs. |
@a1346054 can we close this PR? |
I reworked this PR and split it into individual PRs for further discussion, as suggested in the review comments. |
No description provided.