Format comparisons, functions, and redirections to be consistent (#1201)

* Format comparisons and functions to be consistent

* Add `checkstyle.py` script and add check to CI

* Fix Ruff lints

.github/workflows/ci.yml

@@ -36,6 +36,8 @@ jobs:
         env:
           # NOTE: use env to pass the output in order to avoid possible injection attacks
           FILES: "${{ steps.files.outputs.added_modified }}"
+      - name: checkstyle
+        run: ./scripts/checkstyle.py
       - name: Shellcheck
         run: shellcheck --severity=error bin/* ./*.sh
       - name: Lint and format Python with Ruff

bin/git-abort

@@ -2,7 +2,7 @@
 
 set -euo pipefail
 
-function discover_op() {
+discover_op() {
   local gitdir
   # git rev-parse emits an error if not in a git repo so only need to bail out
   gitdir="$(git rev-parse --git-dir)" || exit
@@ -14,7 +14,7 @@ function discover_op() {
   done
 }
 
-function validate_op() {
+validate_op() {
   local op="$1"
   if [ -z "$op" ]; then
     echo "No active operation found" >&2
@@ -26,7 +26,7 @@ function validate_op() {
   fi
 }
 
-function discover_action() {
+discover_action() {
     local action=${1/git-/}
     if [ "$action" != "abort" ] && [ "$action" != "continue" ]; then
         echo "Invalid action: $1" >&2

bin/git-bulk

@@ -30,7 +30,7 @@ cdfail() {
 }
 
 # add another workspace to global git config
-function addworkspace {
+addworkspace() {
   git config --global bulkworkspaces."$wsname" "$wsdir";
   if [ -n "$source" ]; then
         if [ ! -d "$wsdir" ]; then echo 1>&2 "Path of workspace doesn't exist, make it first."; exit 1; fi
@@ -59,19 +59,19 @@ function addworkspace {
 }
 
 # add current directory
-function addcurrent { git config --global bulkworkspaces."$wsname" "$PWD"; }
+addcurrent() { git config --global bulkworkspaces."$wsname" "$PWD"; }
 
 # remove workspace from global git config
-function removeworkspace { checkWSName && git config --global --unset bulkworkspaces."$wsname"; }
+removeworkspace() { checkWSName && git config --global --unset bulkworkspaces."$wsname"; }
 
 # remove workspace from global git config
-function purge { git config --global --remove-section bulkworkspaces; }
+purge() { git config --global --remove-section bulkworkspaces; }
 
 # list all current workspace locations defined
-function listall { git config --global --get-regexp bulkworkspaces; }
+listall() { git config --global --get-regexp bulkworkspaces; }
 
 # guarded execution of a git command in one specific repository
-function guardedExecution () {
+guardedExecution () {
   if [ "${quiet?}" != "true" ] || $guardedmode; then
     echo 1>&2 "${bldred}->${reset} executing ${inverse}git $gitcommand${reset} in repository ${leadingpath%/*}/${bldred}${curdir##*/}${reset}"
   fi
@@ -88,7 +88,7 @@ function guardedExecution () {
 }
 
 # check if the passed command is known as a core git command
-function checkGitCommand () {
+checkGitCommand () {
   if git help -a | grep -o -q "\b${corecommand}\b"; then
     echo 1>&2 "Core command \"$corecommand\" accepted."
   else
@@ -101,7 +101,7 @@ function checkGitCommand () {
 }
 
 # check if workspace name is registered
-function checkWSName () {
+checkWSName () {
   while read -r workspace; do
     parseWsName "$workspace"
     if [[ $rwsname == "$wsname" ]]; then return; fi
@@ -111,7 +111,7 @@ function checkWSName () {
 }
 
 # parse out wsname from workspacespec
-function parseWsName () {
+parseWsName () {
   local wsspec="$1"
   # Get the workspace value from its specification in the `.gitconfig`.
   # May be an absolute path or a variable name of the form: `$VARNAME`
@@ -128,7 +128,7 @@ function parseWsName () {
 }
 
 # detects the wsname of the current directory
-function wsnameToCurrent () {
+wsnameToCurrent () {
   while read -r workspace; do
     if [ -z "$workspace" ]; then continue; fi
     parseWsName "$workspace"
@@ -140,7 +140,7 @@ function wsnameToCurrent () {
 }
 
 # helper to check number of arguments.
-function allowedargcount () {
+allowedargcount () {
 	if [ "$paramcount" -ne "${1:-0}"  ] && [ "$paramcount" -ne "${2:-0}" ]; then
 		echo 1>&2 "error: wrong number of arguments" && usage;
 		exit 1;
@@ -148,7 +148,7 @@ function allowedargcount () {
 }
 
 # execute the bulk operation
-function executBulkOp () {
+executBulkOp () {
   checkGitCommand
   if ! $allwsmode && ! $singlemode; then wsnameToCurrent; fi # by default git bulk works within the 'current' workspace
   listall | while read -r workspacespec; do
@@ -194,7 +194,7 @@ while [ "${#}" -ge 1 ] ; do
     --listall|--purge)
       butilcommand="${1:2}" && break ;;
     --removeworkspace|--addcurrent|--addworkspace)
-      butilcommand="${1:2}" && wsname="$2" && wsdir="$3" && if [ "$4" == "--from" ]; then source="$5"; fi && break ;;
+      butilcommand="${1:2}" && wsname="$2" && wsdir="$3" && if [ "$4" = "--from" ]; then source="$5"; fi && break ;;
     --no-follow-symlinks)
       no_follow_symlinks=true ;;
     --no-follow-hidden)

bin/git-clear

@@ -2,7 +2,7 @@
 PROGNAME="git-clear"
 FORCE=0
 
-function _usage() {
+_usage() {
 cat << EOF
 usage: $PROGNAME options
 usage: $PROGNAME -h|help|?
@@ -38,6 +38,6 @@ else
     clean=y
 fi
 
-if [ "$clean" == "y" ]; then
+if [ "$clean" = "y" ]; then
     git clean -d -f -x && git reset --hard
 fi

bin/git-clear-soft

@@ -2,6 +2,6 @@
 
 echo -n "Sure? - This command may delete files that cannot be recovered. Files and directories in .gitignore will be preserved [y/N]: "
 read -r answer
-if [ "$answer" == "y" ]
+if [ "$answer" = "y" ]
     then git clean -d -f && git reset --hard
 fi

bin/git-ignore

@@ -2,7 +2,7 @@
 
 GIT_DIR=$(git rev-parse --git-dir 2>/dev/null)
 
-function show_contents {
+show_contents() {
   local file="${2/#~/$HOME}"
   if [ -f "$file" ]; then
     echo "$1 gitignore: $2" && cat "$file"
@@ -11,7 +11,7 @@ function show_contents {
   fi
 }
 
-function cd_to_git_root {
+cd_to_git_root() {
   local error_level="$1"
 
   if ! git rev-parse --git-dir &>/dev/null; then
@@ -29,7 +29,7 @@ function cd_to_git_root {
   fi
 }
 
-function global_ignore() {
+global_ignore() {
   if ! git config --global core.excludesFile 2>/dev/null; then
     if [ -f "$HOME/.gitignore" ]; then
       echo "$HOME/.gitignore"
@@ -39,11 +39,11 @@ function global_ignore() {
   fi
 }
 
-function show_global {
+show_global() {
   show_contents Global "$(global_ignore)"
 }
 
-function add_global {
+add_global() {
   local global_gitignore
   global_gitignore="$(global_ignore)"
   if [ -z "$global_gitignore" ]; then
@@ -56,28 +56,28 @@ function add_global {
   fi
 }
 
-function show_local {
+show_local() {
   cd_to_git_root --warn
   show_contents Local .gitignore
 }
 
-function add_local {
+add_local() {
   cd_to_git_root --warn
   add_patterns .gitignore "$@"
 }
 
-function show_private {
+show_private() {
   cd_to_git_root --error
   show_contents Private "${GIT_DIR}/info/exclude"
 }
 
-function add_private {
+add_private() {
   cd_to_git_root --error
   test -d "${GIT_DIR}/info" || mkdir -p "${GIT_DIR}/info"
   add_patterns "${GIT_DIR}/info/exclude" "$@"
 }
 
-function add_patterns {
+add_patterns() {
   echo "Adding pattern(s) to: $1"
   local file="${1/#~/$HOME}"
   dir_name=$(dirname "$file")

bin/git-merge-into

@@ -18,7 +18,7 @@ then
     git stash
 fi
 
-if [ "${!#}" == '--ff-only' ]; then
+if [ "${!#}" = '--ff-only' ]; then
     case $# in
         2 ) # dest --ff
             git push "$(git rev-parse --show-toplevel)" "$cur_branch":"$1";;

bin/git-paste

@@ -2,7 +2,7 @@
 set -e
 set -o pipefail
 
-if ! command -v pastebinit >/dev/null 2>&1; then
+if ! command -v pastebinit &>/dev/null; then
     echo >&2 "To run 'git paste', you need to install pastebinit in your system"
     exit 1
 fi

bin/git-psykorebase

@@ -5,7 +5,7 @@ SECONDARY_BRANCH=""
 FF="--ff"
 CONTINUE="no"
 
-function current_branch() {
+current_branch() {
     git rev-parse --abbrev-ref HEAD
 }
 

bin/git-pull-request

@@ -43,7 +43,7 @@ if [ -z "$remote" ]; then
     echo 'no upstream found, push to origin as default'
     remote="origin"
 fi
-[ "$remote" == "." ] && abort "the upstream should be a remote branch."
+[ "$remote" = "." ] && abort "the upstream should be a remote branch."
 
 # make sure it's pushed
 

bin/git-rebase-patch

@@ -30,7 +30,7 @@ do
         GIT_INDEX_FILE=$index git read-tree "$rev"
 
         # Try to apply the patch.
-        GIT_INDEX_FILE=$index git apply --cached "$1" >/dev/null 2>&1
+        GIT_INDEX_FILE=$index git apply --cached "$1" &>/dev/null
         patch_failed=$?
 
         # Do it again, but show the error, if the problem is the patch itself.

bin/git-release

@@ -119,7 +119,7 @@ if test $# -gt 0; then
   fi
 
   declare -a sign_args
-  if [ "$sign" == true ]; then
+  if [ "$sign" = true ]; then
     sign_args=("-s")
   fi
 

bin/git-setup

@@ -2,7 +2,7 @@
 
 COMMIT_MESSAGE='Initial commit'
 
-if [ "$1" == "-m" ]; then
+if [ "$1" = "-m" ]; then
     COMMIT_MESSAGE=$2
     shift; shift
 fi

bin/git-standup

@@ -35,7 +35,7 @@ in_git_repo=$?
 
 # Use colors, but only if connected to a terminal, and that terminal
 # supports them.
-if command -v tput >/dev/null 2>&1; then
+if command -v tput &>/dev/null; then
     ncolors=$(tput colors)
 fi
 if [[ -t 1 ]] && [[ -n "$ncolors" ]] && [[ "$ncolors" -ge 8 ]] ; then

bin/git-summary

@@ -206,10 +206,10 @@ COLUMN_CMD_DELIMTER="¬" # Hopefully, this symbol is not used in branch names...
 SP="$COLUMN_CMD_DELIMTER|"
 
 print_summary_by_line() {
-  if [ "$OUTPUT_STYLE" == "tabular" ]; then
+  if [ "$OUTPUT_STYLE" = "tabular" ]; then
     tabular_headers="# Repo $SP Lines"
     echo -e "$tabular_headers\n$project $SP $(line_count "${paths[@]}")" | column -t -s "$COLUMN_CMD_DELIMTER"
-  elif [ "$OUTPUT_STYLE" == "oneline" ]; then
+  elif [ "$OUTPUT_STYLE" = "oneline" ]; then
     echo "$project / lines: $(line_count "${paths[@]}")"
   elif [ -n "$SUMMARY_BY_LINE" ]; then
     echo
@@ -221,10 +221,10 @@ print_summary_by_line() {
 }
 
 print_summary() {
-  if [ "$OUTPUT_STYLE" == "tabular" ]; then
+  if [ "$OUTPUT_STYLE" = "tabular" ]; then
     tabular_headers="# Repo $SP Age $SP Last active $SP Active on $SP Commits $SP Uncommitted $SP Branch"
     echo -e "$tabular_headers\n$project $SP $(repository_age) $SP $(last_active) $SP $(active_days "$commit") days $SP $(commit_count "$commit") $SP $(uncommitted_changes_count) $SP $(current_branch_name)" | column -t -s "$COLUMN_CMD_DELIMTER"
-  elif [ "$OUTPUT_STYLE" == "oneline" ]; then
+  elif [ "$OUTPUT_STYLE" = "oneline" ]; then
     echo "$project / age: $(repository_age) / last active: $(last_active) / active on $(active_days "$commit") days / commits: $(commit_count "$commit") / uncommitted: $(uncommitted_changes_count) / branch: $(current_branch_name)"
   else
     echo
@@ -236,7 +236,7 @@ print_summary() {
     echo " commits     : $(commit_count "$commit")"
 
     # The file count doesn't support passing a git ref so ignore it if a ref is given
-    if [ "$commit" == "HEAD" ]; then
+    if [ "$commit" = "HEAD" ]; then
       echo " files       : $(file_count)"
     fi
     echo " uncommitted : $(uncommitted_changes_count)"

scripts/checkstyle.py

@@ -0,0 +1,262 @@
+#!/usr/bin/env python3
+import re
+import os
+import argparse
+from pathlib import Path
+from typing import Callable, List, Dict, Any # compat
+
+# This file checks Bash and Shell scripts for violations not found with
+# shellcheck or existing methods. You can use it in several ways:
+#
+# Lint all .bash, .sh, .bats files along with 'bin/asdf' and print out violations:
+# $ ./scripts/checkstyle.py
+#
+# The former, but also fix all violations. This must be ran until there
+# are zero violations since any line can have more than one violation:
+# $ ./scripts/checkstyle.py --fix
+#
+# Lint a particular file:
+# $ ./scripts/checkstyle.py ./lib/functions/installs.bash
+#
+# Check to ensure all regular expressions are working as intended:
+# $ ./scripts/checkstyle.py --internal-test-regex
+
+Rule = Dict[str, Any]
+
+class c:
+    RED = '\033[91m'
+    GREEN = '\033[92m'
+    YELLOW = '\033[93m'
+    BLUE = '\033[94m'
+    MAGENTA = '\033[95m'
+    CYAN = '\033[96m'
+    RESET = '\033[0m'
+    BOLD = '\033[1m'
+    UNDERLINE = '\033[4m'
+    LINK: Callable[[str, str], str] = lambda href, text: f'\033]8;;{href}\a{text}\033]8;;\a'
+
+def utilGetStrs(line: Any, m: Any):
+    return (
+        line[0:m.start('match')],
+        line[m.start('match'):m.end('match')],
+        line[m.end('match'):]
+    )
+
+# Before: printf '%s\\n' '^w^'
+# After: printf '%s\n' '^w^'
+def noDoubleBackslashFixer(line: str, m: Any) -> str:
+    prestr, midstr, poststr = utilGetStrs(line, m)
+
+    return f'{prestr}{midstr[1:]}{poststr}'
+
+# Before: $(pwd)
+# After: $PWD
+def noPwdCaptureFixer(line: str, m: Any) -> str:
+    prestr, _, poststr = utilGetStrs(line, m)
+
+    return f'{prestr}$PWD{poststr}'
+
+# Before: [ a == b ]
+# After: [ a = b ]
+def noTestDoubleEqualsFixer(line: str, m: Any) -> str:
+    prestr, _, poststr = utilGetStrs(line, m)
+
+    return f'{prestr}={poststr}'
+
+# Before: function fn() { ...
+# After: fn() { ...
+# ---
+# Before: function fn { ...
+# After fn() { ...
+def noFunctionKeywordFixer(line: str, m: Any) -> str:
+    prestr, midstr, poststr = utilGetStrs(line, m)
+
+    midstr = midstr.strip()
+    midstr = midstr[len('function'):]
+    midstr = midstr.strip()
+
+    parenIdx = midstr.find('(')
+    if parenIdx != -1:
+        midstr = midstr[:parenIdx]
+
+    return f'{prestr}{midstr}() {poststr}'
+
+# Before: >/dev/null 2>&1
+# After: &>/dev/null
+# ---
+# Before: 2>/dev/null 1>&2
+# After: &>/dev/null
+def noVerboseRedirectionFixer(line: str, m: Any) -> str:
+    prestr, _, poststr = utilGetStrs(line, m)
+
+    return f'{prestr}&>/dev/null{poststr}'
+
+def lintfile(file: Path, rules: List[Rule], options: Dict[str, Any]):
+    content_arr = file.read_text().split('\n')
+
+    for line_i, line in enumerate(content_arr):
+        if 'checkstyle-ignore' in line:
+            continue
+
+        for rule in rules:
+            should_run = False
+            if 'sh' in rule['fileTypes']:
+                if file.name.endswith('.sh'):
+                    should_run = True
+            if 'bash' in rule['fileTypes']:
+                if file.name.endswith('.bash') or file.name.endswith('.bats') or file.name.startswith('git-'):
+                    should_run = True
+
+            if options['verbose']:
+                print(f'{str(file)}: {should_run}')
+
+            if not should_run:
+                continue
+
+            m = re.search(rule['regex'], line)
+            if m is not None and m.group('match') is not None:
+                dir = os.path.relpath(file.resolve(), Path.cwd())
+                prestr = line[0:m.start('match')]
+                midstr = line[m.start('match'):m.end('match')]
+                poststr = line[m.end('match'):]
+
+                print(f'{c.CYAN}{dir}{c.RESET}:{line_i + 1}')
+                print(f'{c.MAGENTA}{rule["name"]}{c.RESET}: {rule["reason"]}')
+                print(f'{prestr}{c.RED}{midstr}{c.RESET}{poststr}')
+                print()
+
+                if options['fix']:
+                    content_arr[line_i] = rule['fixerFn'](line, m)
+
+                rule['found'] += 1
+
+    if options['fix']:
+        file.write_text('\n'.join(content_arr))
+
+def main():
+    rules: List[Rule] = [
+        {
+            'name': 'no-pwd-capture',
+            'regex': '(?P<match>\\$\\(pwd\\))',
+            'reason': '$PWD is essentially equivalent to $(pwd) without the overhead of a subshell',
+            'fileTypes': ['bash', 'sh'],
+            'fixerFn': noPwdCaptureFixer,
+            'testPositiveMatches': [
+                '$(pwd)'
+            ],
+            'testNegativeMatches': [
+                '$PWD'
+            ],
+        },
+        {
+            'name': 'no-test-double-equals',
+            'regex': '(?<!\\[)\\[ (?:[^]]|](?=}))*?(?P<match>==).*?]',
+            'reason': 'Disallow double equals in places where they are not necessary for consistency',
+            'fileTypes': ['bash', 'sh'],
+            'fixerFn': noTestDoubleEqualsFixer,
+            'testPositiveMatches': [
+                '[ a == b ]',
+                '[ "${lines[0]}" == blah ]',
+            ],
+            'testNegativeMatches': [
+                '[ a = b ]',
+                '[[ a = b ]]',
+                '[[ a == b ]]',
+                '[ a = b ] || [[ a == b ]]',
+                '[[ a = b ]] || [[ a == b ]]',
+                '[[ "${lines[0]}" == \'usage: \'* ]]',
+                '[ "${lines[0]}" = blah ]',
+            ],
+        },
+        {
+            'name': 'no-function-keyword',
+            'regex': '^[ \\t]*(?P<match>function .*?(?:\\([ \\t]*\\))?[ \\t]*){',
+            'reason': 'Only allow functions declared like `fn_name() {{ :; }}` for consistency (see ' + c.LINK('https://www.shellcheck.net/wiki/SC2113', 'ShellCheck SC2113') + ')',
+            'fileTypes': ['bash', 'sh'],
+            'fixerFn': noFunctionKeywordFixer,
+            'testPositiveMatches': [
+                'function fn() { :; }',
+                'function fn { :; }',
+            ],
+            'testNegativeMatches': [
+                'fn() { :; }',
+            ],
+        },
+        {
+            'name': 'no-verbose-redirection',
+            'regex': '(?P<match>(>/dev/null 2>&1|2>/dev/null 1>&2))',
+            'reason': 'Use `&>/dev/null` instead of `>/dev/null 2>&1` or `2>/dev/null 1>&2` for consistency',
+            'fileTypes': ['bash'],
+            'fixerFn': noVerboseRedirectionFixer,
+            'testPositiveMatches': [
+                'echo woof >/dev/null 2>&1',
+                'echo woof 2>/dev/null 1>&2',
+            ],
+            'testNegativeMatches': [
+                'echo woof &>/dev/null',
+                'echo woof >&/dev/null',
+            ],
+        },
+    ]
+    [rule.update({ 'found': 0 }) for rule in rules]
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument('files', metavar='FILES', nargs='*')
+    parser.add_argument('--fix', action='store_true')
+    parser.add_argument('--verbose', action='store_true')
+    parser.add_argument('--internal-test-regex', action='store_true')
+    args = parser.parse_args()
+
+    if args.internal_test_regex:
+        for rule in rules:
+            for positiveMatch in rule['testPositiveMatches']:
+                m: Any = re.search(rule['regex'], positiveMatch)
+                if m is None or m.group('match') is None:
+                    print(f'{c.MAGENTA}{rule["name"]}{c.RESET}: Failed {c.CYAN}positive{c.RESET} test:')
+                    print(f'=> {positiveMatch}')
+                    print()
+
+            for negativeMatch in rule['testNegativeMatches']:
+                m: Any = re.search(rule['regex'], negativeMatch)
+                if m is not None and m.group('match') is not None:
+                    print(f'{c.MAGENTA}{rule["name"]}{c.RESET}: Failed {c.YELLOW}negative{c.RESET} test:')
+                    print(f'=> {negativeMatch}')
+                    print()
+        print('Done.')
+        return
+
+    options = {
+        'fix': args.fix,
+        'verbose': args.verbose,
+    }
+
+    # parse files and print matched lints
+    if len(args.files) > 0:
+        for file in args.files:
+            p = Path(file)
+            if p.is_file():
+                lintfile(p, rules, options)
+    else:
+        for file in Path.cwd().glob('**/*'):
+            if '.git' in str(file.absolute()):
+                continue
+
+            if file.is_file():
+                lintfile(file, rules, options)
+
+    # print final results
+    print(f'{c.UNDERLINE}TOTAL ISSUES{c.RESET}')
+    for rule in rules:
+        print(f'{c.MAGENTA}{rule["name"]}{c.RESET}: {rule["found"]}')
+
+    grand_total = sum([rule['found'] for rule in rules])
+    print(f'GRAND TOTAL: {grand_total}')
+    print(f'{c.BOLD}{c.YELLOW}NOTE:{c.RESET} Run "./scripts/checkstyle.py --fix" to automatically fix all issues (may need to run multiple times)')
+
+    # exit
+    if grand_total == 0:
+        exit(0)
+    else:
+        exit(2)
+
+main()