Skip to content
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

fix: CURL detection, and shellcheck corrections #242

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

hucste
Copy link

@hucste hucste commented Jan 2, 2019

Hi....

Here are several corrections, by shellcheck.net and especially for CURL detection and using!

;)

@itoffshore
Copy link
Collaborator

itoffshore commented Jan 3, 2019

I'm rejecting some of the commits in this PR mainly for:

Continuing:

  • the helper scripts are compatible with BASH / ASH / DASH / MKSH shells & make extensive use of functions with local variables. POSIX may soon define local:

https://stackoverflow.com/questions/18597697/posix-compliant-way-to-scope-variables-to-a-function-in-a-shell-script

http://austingroupbugs.net/view.php?id=767

The only shell which does not support local is KSH which defines typeset but this is not POSIX compliant either.

  • Initialising variables with var= is not an error

  • Quoting variables unnecessarily gives them the same colour as strings in nano. Again I prefer readability.

  • $* versus $@ : The implementation of $* has always been a problem and realistically should have been replaced with the behaviour of $@. In almost every case where coders use $*, they mean $@. $* Can cause bugs and even security holes in your software.

  • get_options $@ - $@ is quoted inside the function itself

  • replacing curl with a path in $CURL will hide system configuration errors (curl not being in $PATH).

  • running cat on a file with multiple lines is not useless

  • awk instead of grep - this is less readable

  • adding $IS_CRON is not really needed. update-ngxblocker already has a -q option for quiet non-ANSI output designed for when run from cron.


Commits which are OK:

  • fix: shellcheck SC2059, use printf '..%s..' ""

  • fix: shellcheck SC2230, use 'command -v' instead of 'wich' => please fix the mispelling of which

  • fix: shellcheck SC2062, quote the grep pattern


Other comments:

some of your commits had alignment issues (tabs versus spaces) - before making a local commit you should always run git diff to check for alignment issues.

this PR has been made on your master branch. It is good practice to always create a repo branch for a PR to prevent issues merging the changes back into your master

@hucste
Copy link
Author

hucste commented Jan 13, 2019

Just few comment:

  • ksh supports local. it's an alias on typeset! (see: http://man.openbsd.org/ksh#Aliases )
  • about 'awk', I do not agree at all ... but I would not argue more than that: it seems to me preferable to use a single tool, rather than 36, especially if the action remains understandable... simple to understand, simple to execute.
  • ok, about your option -q ;)

I will create a new PR, in some days, with you notes.

@hucste
Copy link
Author

hucste commented Jan 13, 2019

Hi,

Is this previous fix d5ff386 about the management ot the printf messages, is ok, for you?!

Can i recreate it again on a new branch?

@itoffshore
Copy link
Collaborator

@hucste - I am happy to merge:


Commits which are OK:

  • fix: shellcheck SC2230, use 'command -v' instead of 'wich' => please fix the spelling of which

  • fix: shellcheck SC2062, quote the grep pattern

  • fix: shellcheck SC2059, use printf '..%s..' "" => please remove the following change

@@ -240,7 +240,7 @@ print_message() {
	msg="$@"

	if [ "$VERBOSE" != "N" ]; then
		printf "$msg"
		printf '%s\n' "$msg"
	fi
}

I think it is clearer for new line formatting to be where print_message() is called from.


  • As you have made the PR on your master branch I think the easy way to fix this is to do a hard reset of your fork:
git checkout master
git remote update
git reset --hard upstream/master --
git push origin +master
  • & create a branch to work on for the new PR

git checkout -b my-branch

  • & add the 3 commits above to the new PR.

@mmoya
Copy link

mmoya commented Nov 17, 2022

POSIX may soon define local:

http://austingroupbugs.net/view.php?id=767

FTR, the proposal was rejected on 2022-08-08.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants