Merge pull request #12072 from XuehaiPan/refactor-shell-formatter

style: add shfmt exit status to `brew style`

Library/Homebrew/style.rb

@@ -65,12 +65,16 @@ module Homebrew
         run_shellcheck(shell_files, output_type)
       end
 
-      run_shfmt(shell_files, fix: fix) if ruby_files.none? || shell_files.any?
+      shfmt_result = if ruby_files.any? && shell_files.none?
+        true
+      else
+        run_shfmt(shell_files, fix: fix)
+      end
 
       if output_type == :json
         Offenses.new(rubocop_result + shellcheck_result)
       else
-        rubocop_result && shellcheck_result
+        rubocop_result && shellcheck_result && shfmt_result
       end
     end
 

Library/Homebrew/utils/shfmt.sh

@@ -49,6 +49,7 @@ do
   SHFMT_ARGS+=("${arg}")
   shift
 done
+unset arg
 
 FILES=()
 for file in "$@"
@@ -66,6 +67,7 @@ do
     exit 1
   fi
 done
+unset file
 
 if [[ "${#FILES[@]}" == 0 ]]
 then
@@ -76,6 +78,32 @@ fi
 ### Custom shell script styling
 ###
 
+# Check for specific patterns and prompt messages if detected
+no_forbidden_patten() {
+  local file="$1"
+  local tempfile="$2"
+  local subject="$3"
+  local message="$4"
+  local regex_pos="$5"
+  local regex_neg="${6:-}"
+  local line
+  local num=0
+  local retcode=0
+
+  while IFS='' read -r line
+  do
+    num="$((num + 1))"
+    if [[ "${line}" =~ ${regex_pos} ]] &&
+       [[ -z "${regex_neg}" || ! "${line}" =~ ${regex_neg} ]]
+    then
+      onoe "${subject} detected at \"${file}\", line ${num}."
+      [[ -n "${message}" ]] && onoe "${message}"
+      retcode=1
+    fi
+  done <"${file}"
+  return "${retcode}"
+}
+
 # Check pattern:
 # '^\t+'
 #
@@ -84,22 +112,12 @@ fi
 no_tabs() {
   local file="$1"
   local tempfile="$2"
-  local line
-  local num=0
-  local retcode=0
-  local regex_pos='^[[:space:]]+'
-  local regex_neg='^ +'
 
-  while IFS='' read -r line
+  no_forbidden_patten "${file}" "${tempfile}" \
-  do
+    "Indent with tab" \
-    num="$((num + 1))"
+    'Replace tabs with 2 spaces instead.' \
-    if [[ "${line}" =~ ${regex_pos} && ! "${line}" =~ ${regex_neg} ]]
+    '^[[:space:]]+' \
-    then
+    '^ +'
-      onoe "Indent by tab detected at \"${file}\", line ${num}."
-      retcode=1
-    fi
-  done <"${file}"
-  return "${retcode}"
 }
 
 # Check pattern:
@@ -116,18 +134,10 @@ no_tabs() {
 no_multiline_for_statements() {
   local file="$1"
   local tempfile="$2"
-  local line
-  local num=0
-  local retcode=0
   local regex='^ *for [_[:alnum:]]+ in .*\\$'
-
+  local message
-  while IFS='' read -r line
+  message="$(
-  do
+    cat <<EOMSG
-    num="$((num + 1))"
-    if [[ "${line}" =~ ${regex} ]]
-    then
-      onoe "Multiline for statement detected at \"${file}\", line ${num}."
-      cat >&2 <<EOMSG
 Use the followings instead (keep for statements only one line):
   ARRAY=(
     ...
@@ -137,10 +147,12 @@ Use the followings instead (keep for statements only one line):
     ...
   done
 EOMSG
-      retcode=1
+  )"
-    fi
+
-  done <"${file}"
+  no_forbidden_patten "${file}" "${tempfile}" \
-  return "${retcode}"
+    "Multiline for statement" \
+    "${message}" \
+    "${regex}"
 }
 
 # Check pattern:
@@ -155,32 +167,26 @@ EOMSG
 no_IFS_newline() {
   local file="$1"
   local tempfile="$2"
-  local line
-  local num=0
-  local retcode=0
   local regex="^[^#]*IFS=\\\$'\\\\n'"
-
+  local message
-  while IFS='' read -r line
+  message="$(
-  do
+    cat <<EOMSG
-    num="$((num + 1))"
-    if [[ "${line}" =~ ${regex} ]]
-    then
-      onoe "Pattern \`IFS=\$'\\n'\` detected at \"${file}\", line ${num}."
-      cat >&2 <<EOMSG
 Use the followings instead:
   while IFS='' read -r line
   do
     ...
   done < <(command)
 EOMSG
-      retcode=1
+  )"
-    fi
+
-  done <"${file}"
+  no_forbidden_patten "${file}" "${tempfile}" \
-  return "${retcode}"
+    "Pattern \`IFS=\$'\\n'\`" \
+    "${message}" \
+    "${regex}"
 }
 
 # Combine all forbidden styles
-no_forbiddens() {
+no_forbidden_styles() {
   local file="$1"
   local tempfile="$2"
 
@@ -196,18 +202,23 @@ no_forbiddens() {
 #   then                      then
 #
 # before:                   after:
-#   if [[ -n ... || \         if [[ -n ... || \
+#   if [[ -n ... ||           if [[ -n ... ||
 #     -n ... ]]                     -n ... ]]
 #   then                      then
 #
 align_multiline_if_condition() {
-  local multiline_if_begin_regex='^( *)(el)?if '
-  local multiline_then_end_regex='^(.*)\; (then( *#.*)?)$'
-  local within_test_regex='^( *)(((! )?-[fdrwxes])|([^\[]+ == ))'
-  local base_indent=''
-  local extra_indent=''
   local line
   local lastline=''
+  local base_indent=''  # indents before `if`
+  local extra_indent='' # 2 extra spaces for `elif`
+  local multiline_if_begin_regex='^( *)(el)?if '
+  local multiline_then_end_regex='^(.*)\; (then( *#.*)?)$'
+  local within_test_regex='^( *)(((! )?-[fdLrwxeszn] )|([^\[]+ == ))'
+
+  trim() {
+    [[ "$1" =~ [^[:space:]](.*[^[:space:]])? ]]
+    printf "%s" "${BASH_REMATCH[0]}"
+  }
 
   if [[ "$1" =~ ${multiline_if_begin_regex} ]]
   then
@@ -228,9 +239,9 @@ align_multiline_if_condition() {
     fi
     if [[ "${line}" =~ ${within_test_regex} ]]
     then
-      echo "  ${extra_indent}${line}"
+      echo "${base_indent}${extra_indent}      $(trim "${line}")"
     else
-      echo " ${extra_indent}${line}"
+      echo "${base_indent}${extra_indent}   $(trim "${line}")"
     fi
   done
 
@@ -244,7 +255,7 @@ align_multiline_if_condition() {
 #
 # before:                   after:
 #   if [[ ... ]] ||           if [[ ... ]] ||
-#     [[ ... ]]; then           [[ ... ]]
+#     [[ ... ]]; then            [[ ... ]]
 #                             then
 #
 # before:                   after:
@@ -255,13 +266,13 @@ wrap_then_do() {
   local file="$1"
   local tempfile="$2"
 
-  local -a processed
+  local -a processed=()
+  local -a buffer=()
   local line
   local singleline_then_regex='^( *)(el)?if (.+)\; (then( *#.*)?)$'
   local singleline_do_regex='^( *)(for|while) (.+)\; (do( *#.*)?)$'
   local multiline_if_begin_regex='^( *)(el)?if '
   local multiline_then_end_regex='^(.*)\; (then( *#.*)?)$'
-  local -a buffer=()
 
   while IFS='' read -r line
   do
@@ -297,22 +308,21 @@ wrap_then_do() {
   printf "%s\n" "${processed[@]}" >"${tempfile}"
 }
 
-# TODO: it's hard to align multiline switch cases
+# TODO: It's hard to align multiline switch cases
 align_multiline_switch_cases() {
   true
 }
 
 format() {
   local file="$1"
+  local tempfile
   if [[ ! -f "${file}" || ! -r "${file}" ]]
   then
     onoe "File \"${file}\" is not readable."
     return 1
   fi
 
-  # shellcheck disable=SC2155
+  tempfile="$(dirname "${file}")/.${file##*/}.temp"
-  local tempfile="$(dirname "${file}")/.${file##*/}.temp"
-
   trap 'rm -f "${tempfile}" 2>/dev/null' RETURN
   cp -af "${file}" "${tempfile}"
 
@@ -325,15 +335,12 @@ format() {
   # Format with `shfmt` first
   if ! "${SHFMT}" -w "${SHFMT_ARGS[@]}" "${tempfile}"
   then
-    onoe "Failed to run \`shfmt\`"
+    onoe "Failed to run \`shfmt\` for file \"${file}\"."
     return 1
   fi
 
-  # Fail fast when forbidden patterns detected
+  # Fail fast when forbidden styles detected
-  if ! no_forbiddens "${file}" "${tempfile}"
+  ! no_forbidden_styles "${file}" "${tempfile}" && return 2
-  then
-    return 2
-  fi
 
   # Tweak it with custom shell script styles
   wrap_then_do "${file}" "${tempfile}"
@@ -357,13 +364,15 @@ format() {
 RETCODE=0
 for file in "${FILES[@]}"
 do
-  if ! format "${file}"
+  format "${file}"
+  retcode="$?"
+  if [[ "${retcode}" != 0 ]]
   then
-    if [[ "$?" == 1 ]]
+    if [[ "${retcode}" == 1 ]]
     then
-      onoe "${0##*/}: Failed to format file \"${file}\". Function exited with code 1."
+      onoe "${0##*/}: Failed to format file \"${file}\". Formatter exited with code 1."
     else
-      onoe "${0##*/}: Bad style for file \"${file}\". Function exited with code 2."
+      onoe "${0##*/}: Bad style for file \"${file}\". Formatter exited with code 2."
     fi
     onoe
     RETCODE=1