Updates#1
Conversation
📝 WalkthroughWalkthroughAdds v0.3.0: configuration file support (~/.macgnu.conf) exposing Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as macgnu CLI
participant Config as Config Loader
participant Filter as Formula Filter
participant Installer as Installer
participant Brew as Homebrew
User->>CLI: run "macgnu install [--dry-run] [--force] [pkg]"
CLI->>CLI: parse flags & command
CLI->>Config: read CONFIG_FILE / env
Config-->>CLI: return MACGNU_SKIP_PACKAGES
CLI->>Filter: apply skips to formula list
Filter-->>CLI: return filtered formulas
CLI->>Installer: perform install/preview with progress
Installer->>Brew: invoke brew install/upgrade/uninstall
Brew-->>Installer: return status
Installer-->>CLI: progress updates
CLI-->>User: summary & exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
47-174: Remove the broken IMPROVEMENTS.md reference or create the missing file.The README references
IMPROVEMENTS.mdat line 173 as a "Quick Start Guide", but this file does not exist in the repository. Either remove this documentation link or create the file with appropriate content.
🤖 Fix all issues with AI agents
In `@docs/FEATURES.md`:
- Around line 74-78: The fenced code block showing the install progress lacks a
language identifier; update the opening fence for that block in FEATURES.md from
``` to ```text so it becomes a labeled "text" code block (the block containing
"Installing 36 GNU packages...", "Progress: [...]", "✓ Installation complete")
to enable proper syntax highlighting and match other blocks in the document.
In `@docs/INSTALLATION.md`:
- Around line 136-140: Update the Homebrew permission fix so it works on both
Intel and Apple Silicon by replacing the hardcoded /usr/local paths with the
dynamic brew prefix (using brew --prefix) and ensure required directories exist;
specifically, change the chown/chmod commands from targeting /usr/local/bin and
/usr/local/sbin to operate on "$(brew --prefix)" and create the common
directories (bin, share, etc, var, Frameworks, Cellar) before changing ownership
and permissions as shown in the suggested fix so both architectures are
supported.
- Around line 7-10: The INSTALLATION.md snippet lacks prerequisites for the
"awesome" package manager; update the docs to first instruct how to install
Awesome (e.g., fetch the installer from its GitHub repo using curl or wget and
run it) and to ensure the user's PATH includes the install directory (e.g., add
~/bin to $PATH), then keep the existing usage example "awesome install
shinokada/macgnu" as the final step; mention both curl and wget as options and a
one-line reminder to reload the shell or source the profile after updating PATH.
In `@macgnu`:
- Around line 41-48: The macgnu_load_config function currently sources
CONFIG_FILE which can overwrite an existing environment value for
MACGNU_SKIP_PACKAGES; modify macgnu_load_config so it preserves any pre-existing
environment value by saving MACGNU_SKIP_PACKAGES before sourcing (or by only
assigning it from the config if it is unset/empty) and restoring or keeping the
original if it was exported in the environment; reference the
MACGNU_SKIP_PACKAGES variable and the macgnu_load_config function when
implementing this change.
- Around line 382-443: The argument parsing in macgnu_main allows later args to
override -h/--help and -v/--version; fix by introducing boolean flags (e.g.,
HELP_REQUESTED and VERSION_REQUESTED) set when -h/--help or -v/--version are
seen in the parsing loop, avoid changing cmd when those flags are set, and after
the for-loop check those booleans first—if HELP_REQUESTED call macgnu_help and
exit, if VERSION_REQUESTED echo "$version" and exit—so help/version always take
precedence over commands like install/uninstall; update references in
macgnu_main and the parsing case to use these new booleans.
- Around line 51-77: macgnu_filter_formulas uses Bash namerefs (local -n arr=$1)
which fails on macOS Bash 3.2; refactor the function to accept the formulas as
positional parameters (iterate over "$@"), parse MACGNU_SKIP_PACKAGES into a
local array, build a filtered list, and emit the filtered values (e.g., print
them so callers can capture via command substitution into their arrays); then
update all four call sites that currently call macgnu_filter_formulas formulas
to instead capture the function output into the formulas array (apply this
pattern to each call site that passes an array name).
In `@tests/README.md`:
- Around line 53-81: Update the three fenced code blocks that contain test
output in tests/README.md to specify the language "text" (i.e., change ``` to
```text) so markdownlint recognizes them as text blocks; specifically update the
blocks showing the checkmarks list, the failed-test example (the block starting
with "✗ Some test name"), and the Test Summary block (the block starting with
"================================") to use ```text instead of ``` .
In `@updates/README.md`:
- Around line 112-116: Replace the bold inline section title "**File: macgnu**"
with a proper Markdown heading to satisfy MD036; locate the "🔄 Script
Enhancements" block and change the bold "File: macgnu" token to a heading (e.g.,
"### File: macgnu" or another appropriate heading level consistent with
surrounding headings) so the section uses a heading instead of emphasis.
- Around line 49-66: Update the code fence for the directory tree block in the
README (the triple-backtick block that begins with "macgnu/") to declare a
language by changing the opening ``` to ```text so the tree is rendered as plain
text (addresses MD040 and improves rendering).
🧹 Nitpick comments (2)
updates/IMPLEMENTATION_SUMMARY.md (1)
128-150: Add a language tag to the fenced directory tree.
Improves Markdown rendering consistency and satisfies MD040.🧩 Suggested tweak
-``` +```text macgnu/ ├── macgnu # Updated main script (v0.3.0) ├── .macgnu.conf.example # NEW: Config file template ├── README.md # Updated with new sections │ ├── docs/ # NEW: Documentation directory │ ├── INSTALLATION.md # Setup guide │ ├── FEATURES.md # Feature descriptions │ └── CONFIGURATION.md # Config guide │ ├── updates/ # NEW: Update documentation │ ├── CHANGELOG.md # What changed in v0.3.0 │ └── MIGRATION.md # Upgrade guide │ └── tests/ # NEW: Test suite ├── run_tests.sh # Main test suite ├── unit_tests.sh # Unit tests ├── test.macgnu.conf # Test config └── README.md # Testing guide -``` +```tests/unit_tests.sh (1)
8-9: Remove unusedPROJECT_ROOTto keep lint clean.It’s never referenced in this script, so it only adds shellcheck noise.
♻️ Proposed change
- PROJECT_ROOT="$(dirname "$TEST_DIR")"
| Using Awesome package manager: | ||
| ```bash | ||
| awesome install shinokada/macgnu | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Awesome package manager awesome install command
💡 Result:
Do you mean the Awesome window manager (awesomewm) or a package named "awesome" (or something like Font Awesome)? Which one do you want install instructions for — and what OS/distribution are you using?
🌐 Web query:
macgnu shinokada awesome install
💡 Result:
You can install Shinichi Okada’s "awesome" script package manager and use it to install packages (e.g. shinokada/manop) like this:
- Install awesome:
- curl:
curl -s https://raw.githubusercontent.com/shinokada/awesome/main/install | bash -s install - or wget:
wget -qO - https://raw.githubusercontent.com/shinokada/awesome/main/install | bash -s install. [1][2]
-
Ensure ~/bin is in your PATH (add to ~/.zshrc or ~/.bashrc if needed):
echo 'export PATH=$HOME/bin:$PATH' >> ~/.zshrc
source ~/.zshrc. [1] -
Install a package (example):
awesome install shinokada/manop
You can also install by full repo URL (https://github.com/shinokada/manop) or specify subpath/main script per the tool’s syntax. To uninstall, run the uninstall command from the installer or remove the installed files. [1][2]
Sources: Shinichi Okada’s Qiita post and article describing the tool and installer. [1][2]
References:
[1] Qiita: シンプルなスクリプトパッケージマネージャーを作ってみた (includes install commands and usage).
[2] Medium: A New Simple Package Manager for Script Languages (install instructions).
🌐 Web query:
Awesome package manager documentation
💡 Result:
Do you mean:
- the Awesome (awesomewm) window manager documentation, or
- an “Awesome” (curated) list of package-manager documentation/resources (npm, pip, apt, Homebrew, etc.)?
Tell me which and I’ll fetch the best docs/lists with links.
Add context explaining the Awesome package manager.
While Awesome is a legitimate script package manager (by Shinichi Okada), the INSTALLATION.md snippet lacks necessary setup instructions. Readers unfamiliar with it won't know how to install Awesome itself or configure their PATH. Consider adding prerequisite steps: fetching the installer via curl or wget from the GitHub repository and ensuring ~/bin is in $PATH, then the awesome install command.
🤖 Prompt for AI Agents
In `@docs/INSTALLATION.md` around lines 7 - 10, The INSTALLATION.md snippet lacks
prerequisites for the "awesome" package manager; update the docs to first
instruct how to install Awesome (e.g., fetch the installer from its GitHub repo
using curl or wget and run it) and to ensure the user's PATH includes the
install directory (e.g., add ~/bin to $PATH), then keep the existing usage
example "awesome install shinokada/macgnu" as the final step; mention both curl
and wget as options and a one-line reminder to reload the shell or source the
profile after updating PATH.
| Fix Homebrew permissions: | ||
| ```bash | ||
| sudo chown -R $(whoami) /usr/local/bin /usr/local/sbin | ||
| chmod u+w /usr/local/bin /usr/local/sbin | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Homebrew default installation prefix Apple Silicon Intel macOS /opt/homebrew /usr/local
💡 Result:
Short answer:
- Apple Silicon (ARM/M1/M2): default prefix is /opt/homebrew. [1]
- Intel macOS (x86_64): default prefix is /usr/local. [1]
Notes:
- The official installer uses those prefixes so bottles (prebuilt binaries) work and so ARM and Intel installs can coexist on Apple Silicon (ARM in /opt/homebrew, Intel/Rosetta in /usr/local). [1][2]
Sources:
[1] Homebrew installation docs.
[2] Discussion/FAQ explaining /opt/homebrew choice (coexistence and bottles).
🌐 Web query:
Homebrew "directory must exist and be writable" permission fix current recommendation
💡 Result:
Short answer — follow Homebrew's own guidance: create any missing Homebrew directories, then change ownership to your user and ensure they are writable.
Commands (run in Terminal; use /opt/homebrew on Apple Silicon or /usr/local on Intel):
- Create missing directories:
sudo mkdir -p "$(brew --prefix)"/{bin,share,etc,var,Frameworks,Cellar} - Make you the owner and give write permission:
sudo chown -R "$(whoami)" "$(brew --prefix)"
sudo chmod u+w "$(brew --prefix)"
If brew reports specific directories (e.g. via brew doctor), run:
sudo mkdir -p
sudo chown -R "$(whoami)"
chmod u+w
Sources: Homebrew documentation showing the recommended create/chown/chmod workflow and the diagnostic checks that print those exact commands. (docs.brew.sh)
Citations:
Update Homebrew permission fix to support both Intel and Apple Silicon architectures.
The troubleshooting steps hardcode /usr/local, but Homebrew defaults to /opt/homebrew on Apple Silicon (ARM) and /usr/local on Intel macOS. Update the fix to use $(brew --prefix) to dynamically reference the correct prefix for both architectures:
Suggested fix
sudo mkdir -p "$(brew --prefix)"/{bin,share,etc,var,Frameworks,Cellar}
sudo chown -R "$(whoami)" "$(brew --prefix)"
chmod u+w "$(brew --prefix)"🤖 Prompt for AI Agents
In `@docs/INSTALLATION.md` around lines 136 - 140, Update the Homebrew permission
fix so it works on both Intel and Apple Silicon by replacing the hardcoded
/usr/local paths with the dynamic brew prefix (using brew --prefix) and ensure
required directories exist; specifically, change the chown/chmod commands from
targeting /usr/local/bin and /usr/local/sbin to operate on "$(brew --prefix)"
and create the common directories (bin, share, etc, var, Frameworks, Cellar)
before changing ownership and permissions as shown in the suggested fix so both
architectures are supported.
| source_macgnu() { | ||
| # Create a copy without the main call for testing | ||
| sed '$ d' "$PROJECT_ROOT/macgnu" >/tmp/macgnu_test.sh | ||
| } |
There was a problem hiding this comment.
Sourcing harness can execute main; make the strip and temp file robust.
sed '$ d' only removes the last line. If the script ends with a blank line or adds a footer, the main invocation can remain and execute during source, which is risky for install/uninstall behavior. Also, a fixed /tmp path can clash in parallel runs.
🔧 Safer temp file + pattern-based strip
source_macgnu() {
# Create a copy without the main call for testing
- sed '$ d' "$PROJECT_ROOT/macgnu" >/tmp/macgnu_test.sh
+ local tmp
+ tmp="$(mktemp "${TMPDIR:-/tmp}/macgnu_test.XXXXXX")"
+ MACGNU_TEST_FILE="$tmp"
+ # Remove only the main invocation line
+ sed -E '/^[[:space:]]*(macgnu_)?main[[:space:]]+("?\$@\"?|\$@).*$/d' "$PROJECT_ROOT/macgnu" >"$tmp"
}
@@
-trap 'rm -f /tmp/macgnu_test.sh' EXIT
+trap 'rm -f "${MACGNU_TEST_FILE:-}"' EXITThen replace all occurrences of:
source /tmp/macgnu_test.shwith:
source "$MACGNU_TEST_FILE"Also applies to: 371-372
| ### 🔄 Script Enhancements | ||
|
|
||
| **File: macgnu** | ||
|
|
||
| New Functions: |
There was a problem hiding this comment.
Use a heading instead of bold text for the section title.
Markdownlint flags emphasis-as-heading here (MD036).
📝 Suggested fix
-**File: macgnu**
+### File: macgnu📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 🔄 Script Enhancements | |
| **File: macgnu** | |
| New Functions: | |
| ### 🔄 Script Enhancements | |
| ### File: macgnu | |
| New Functions: |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
114-114: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@updates/README.md` around lines 112 - 116, Replace the bold inline section
title "**File: macgnu**" with a proper Markdown heading to satisfy MD036; locate
the "🔄 Script Enhancements" block and change the bold "File: macgnu" token to a
heading (e.g., "### File: macgnu" or another appropriate heading level
consistent with surrounding headings) so the section uses a heading instead of
emphasis.
There was a problem hiding this comment.
@shinokada, understood! I'll skip this suggestion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 158-166: The conditional that checks FORCE_INSTALL inside the "not
installed" branch is redundant because both branches run the same brew install
command; simplify the block around the "$formula" install logic by removing the
inner FORCE_INSTALL check so that if ! brew ls --versions "$formula" then run
brew install "$formula" (with the existing redirection/|| true behavior), and
keep the existing elif [[ "$FORCE_INSTALL" == true ]] branch to run brew upgrade
"$formula" only when the package is already installed; update the block that
references the "$formula" and "$FORCE_INSTALL" variables accordingly.
In `@tests/run_tests.sh`:
- Around line 231-239: Add a SKIPPED counter (e.g., TESTS_SKIPPED) and increment
it in the skip branch where brew isn't installed (the block that currently
echoes "Skipping - brew not installed" and increments TESTS_RUN); then update
the test summary calculation and output to include TESTS_SKIPPED and validate
that TESTS_RUN == TESTS_PASSED + TESTS_FAILED + TESTS_SKIPPED (or adjust tests
run derivation to sum those three) so the pass/fail ratio correctly accounts for
skipped tests.
🧹 Nitpick comments (2)
tests/unit_tests.sh (2)
9-9: Unused variableTEST_DIR.The
TEST_DIRvariable is defined but never used in the script. Either remove it or use it (e.g., for locating test fixtures).🧹 Suggested fix
-TEST_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# TEST_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # Uncomment if needed for fixturesOr simply remove the line if not needed.
19-34: Consider renamingtestfunction to avoid shadowing the shell builtin.The function name
testshadows the shell builtin (used for conditionals liketest -f file). While this works in this script, it can cause confusion and subtle issues.♻️ Suggested rename
-test() { +run_test() { local name="$1" local expected="$2" local actual="$3"Then update all call sites (lines 58-62, 82-86) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 36: Update the CI job matrix entry that defines the operating systems so
it no longer includes the deprecated "macos-13" runner: in the workflow matrix
key where you currently have os: [macos-13, macos-14, macos-15], remove
"macos-13" so the matrix becomes os: [macos-14, macos-15]; ensure the change is
applied to the matrix definition in the .github/workflows/ci.yml file.
In `@macgnu`:
- Around line 83-88: The macgnu_print_progress function can divide by zero when
total is 0; update it to guard the calculation by checking total before
computing percent (e.g., if total is zero set percent=0 or return early), and
ensure the progress-bar generation (the inner seq/printf that uses percent) also
handles percent==0 safely so no negative/invalid seq arguments are passed;
modify macgnu_print_progress to perform this check and then compute/print the
progress only when safe.
- Around line 262-263: The combined declaration and assignment "local
version=$(brew list --versions \"$formula\" ...)" hides the brew command's exit
status; change to declare the variable separately (local version) then assign
with version="$(brew list --versions \"$formula\" 2>/dev/null | awk '{print
$NF}')" and immediately check the command's exit status (or test if version is
empty) before echoing "✓ $formula ($version)" so brew failures are detected and
handled; refer to the variable name version and the command substitution that
invokes brew list --versions and awk to locate the code to change.
- Around line 169-170: The current install step uses a relative path ("cp
.macgnu ~/.macgnu") which can fail if the script is not run from the repo root;
update the command to copy from an absolute path instead by resolving the
script/repo directory and using that resolved path when invoking cp (i.e.,
derive the full path to .macgnu from the script location or $PWD and use that
instead of ".macgnu"); keep the user-facing echo line as-is.
In `@tests/run_tests.sh`:
- Around line 318-324: Remove the duplicated brew check by deleting the outer
if/else block in main() and call test_status_command unconditionally;
test_status_command already performs the `command -v brew` check and prints the
“Note: Some tests skipped - Homebrew not installed” message internally, so keep
the internal logic in test_status_command and simplify main() to a single call
to test_status_command.
🧹 Nitpick comments (4)
macgnu (3)
137-137: Usemapfileto avoid word-splitting issues (SC2207).The pipeline flagged this pattern at lines 137, 186, 226, and 251. While brew formula names don't contain spaces, using
mapfileis more robust and silences the shellcheck warning.♻️ Suggested fix using mapfile
- formulas=($(macgnu_filter_formulas "${formulas[@]}")) + mapfile -t formulas < <(macgnu_filter_formulas "${formulas[@]}")Apply this pattern to all four occurrences (lines 137, 186, 226, 251).
199-199: Consider explicit conditional for clarity (SC2015).The
A && B && C || Dchain can have unexpected behavior if intermediate commands fail. While the|| truemakes this safe, an explicitifblock would be clearer.♻️ Optional: Explicit conditional
- [ -f ~/.gdbinit ] && sed -i.bak '/set startup-with-shell off/d' ~/.gdbinit && rm ~/.gdbinit.bak || true + if [[ -f ~/.gdbinit ]]; then + sed -i.bak '/set startup-with-shell off/d' ~/.gdbinit && rm -f ~/.gdbinit.bak || true + fi
406-413: Unknown arguments silently treated as triggers for help.Unrecognized flags (e.g.,
--dryruntypo) silently fall through to help display. Consider warning the user about unrecognized arguments.♻️ Optional: Warn on unrecognized arguments
*) # Check if it's a package name for install command if [[ -z "$cmd" ]]; then - cmd="help" + echo "Warning: Unrecognized argument '$arg'" >&2 + cmd="${cmd:-help}" elif [[ "$cmd" == "install" ]] && [[ -z "$pkg" ]]; then pkg="$arg" + else + echo "Warning: Unrecognized argument '$arg'" >&2 fi ;;CODERABBIT_FIXES.md (1)
50-55: Add language specifier to fenced code blocks (MD040).The code blocks on lines 50 and 67 lack language specifiers. Adding them improves rendering and accessibility.
♻️ Suggested fix
Line 50:
-``` +```text Total: XLine 67:
-``` +```text fix: apply CodeRabbitAI suggestions
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CODERABBIT_FIXES_ROUND2.md`:
- Around line 79-88: The fenced commit-message block in
CODERABBIT_FIXES_ROUND2.md lacks a language identifier; update the opening
triple-backtick for the commit message code fence (the block containing "fix:
apply CodeRabbitAI round 2 suggestions ...") to include a language token (e.g.,
```text) so markdownlint stops flagging it and rendering is consistent.
In `@macgnu`:
- Around line 84-90: In macgnu_print_progress(), avoid calling seq with a zero
length by computing filled=$((percent/2)) and only invoking seq when filled is
greater than 0; replace the subshell "$(printf '#%.0s' $(seq 1 $((percent /
2))))" with logic that returns an empty string when filled==0 (e.g., conditional
branch or using printf with a width/count) so the function never runs seq 1 0
and thus won't trigger set -e failures.
- Around line 419-451: Replace the current warning-only handling for
unrecognized flags/commands in the argument parsing case blocks with a fail-fast
behavior: inside the --* branch and the default branch where it currently echoes
"Warning: Unrecognized ...", check whether show_help or show_version is set and
if not, print a clear error to stderr and exit with non-zero (e.g., exit 1)
immediately so unknown args cannot fall through and trigger a real install;
update references to cmd and pkg handling to only reach assignment paths after
argument validation so constructs like "--dry-run install" cannot proceed when
an unknown token was seen earlier.
| ## Commit Message Suggestion | ||
| ``` | ||
| fix: apply CodeRabbitAI round 2 suggestions | ||
|
|
||
| - Remove deprecated macos-13 from CI matrix | ||
| - Guard against division by zero in progress function | ||
| - Use absolute path for .macgnu file (fixes bug when run outside repo) | ||
| - Add fallback for unknown version in status display | ||
| - Remove duplicate brew check in test suite | ||
| - Add warnings for unrecognized arguments and flags |
There was a problem hiding this comment.
Specify a language for the commit-message code fence.
markdownlint flags the block without a language identifier; adding one improves rendering consistency.
🛠️ Proposed fix
-```
+```text
fix: apply CodeRabbitAI round 2 suggestions
- Remove deprecated macos-13 from CI matrix
- Guard against division by zero in progress function
- Use absolute path for .macgnu file (fixes bug when run outside repo)
- Add fallback for unknown version in status display
- Remove duplicate brew check in test suite
- Add warnings for unrecognized arguments and flags</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
## Commit Message Suggestion
🤖 Prompt for AI Agents
In `@CODERABBIT_FIXES_ROUND2.md` around lines 79 - 88, The fenced commit-message
block in CODERABBIT_FIXES_ROUND2.md lacks a language identifier; update the
opening triple-backtick for the commit message code fence (the block containing
"fix: apply CodeRabbitAI round 2 suggestions ...") to include a language token
(e.g., ```text) so markdownlint stops flagging it and rendering is consistent.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 299-302: The arithmetic increment "((installed++))" can return a
non-zero exit status under set -e when installed is zero; update the increment
in the loop that updates the installed/not_installed counters (the "installed"
and "not_installed" variables used alongside the echo branches) to use a safe
form such as a pre-increment "((++installed))" (and similarly for
"not_installed" if post-increment is used there) or append "|| true" to the
existing "((installed++))" to prevent the shell from treating the increment as a
failure.
♻️ Duplicate comments (1)
updates/CODERABBIT_FIXES_ROUND2.md (1)
79-89: Specify a language for the commit-message code fence.The fenced code block lacks a language identifier.
🧹 Nitpick comments (1)
updates/BASH32_FIX_SUMMARY.md (1)
57-67: Specify a language for the commit-message code fence.The fenced code block lacks a language identifier, which affects rendering consistency and triggers markdownlint MD040.
🛠️ Proposed fix
## Commit Message -``` +```text fix: replace mapfile with Bash 3.2-compatible while-read loops
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
macgnu (2)
15-19: Exit with non-zero status on error.
exitwithout an argument returns the last command's exit status. Afterechosucceeds (status 0), the script exits successfully even though it detected an error condition.🐛 Suggested fix
macgnu_check_os() { if ! [[ "$OSTYPE" =~ darwin* ]]; then echo "This is meant to be run on macOS only" - exit + exit 1 fi }
22-28: Exit with non-zero status on error.Same issue as above—
exitdefaults to status 0 afterechosucceeds.🐛 Suggested fix
macgnu_check_brew() { if ! command -v brew >/dev/null; then echo "Homebrew not installed!" echo "In order to use this script please install homebrew from https://brew.sh" - exit + exit 1 fi }
🧹 Nitpick comments (1)
macgnu (1)
30-39: Declareresultas local to avoid polluting global namespace.The
resultvariable leaks into the global scope. This is a minor hygiene issue.🧹 Suggested fix
macgnu_check_dirs() { - result=0 + local result=0 for dir in $(brew --prefix)/bin $(brew --prefix)/sbin; do if [[ ! -d $dir || ! -w $dir ]]; then echo "$dir must exist and be writeable" result=1 fi done return $result }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 313-323: The function macgnu_install_package currently assigns
local package=$1 which under set -u can cause an unbound-variable abort before
your validation runs; change the assignment in macgnu_install_package to use
parameter expansion (e.g., set package from ${1-}) so a missing argument yields
an empty string and the subsequent if [[ -z "$package" ]] check can show the
friendly error message; update any similar patterns in this function if present.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 252-256: The uninitialized array filtered_formulas can trigger an
"unbound variable" error under set -u; initialize it before use. In the block
that reads macgnu_filter_formulas into filtered_formulas (the while loop using
macgnu_filter_formulas and formulas), change the declaration to initialize the
array (filtered_formulas=()) so filtered_formulas is always defined before
appending and then assign formulas=("${filtered_formulas[@]}") as before.
- Around line 205-209: The filtered_formulas array may remain uninitialized when
macgnu_filter_formulas returns no entries, causing an unbound variable under set
-u; initialize it before use (e.g., change the declaration to
filtered_formulas=() or declare -a filtered_formulas) in the block that reads
from macgnu_filter_formulas so that the subsequent assignment
formulas=("${filtered_formulas[@]}") is safe; apply the same initialization
pattern as used in macgnu_install to ensure consistency.
- Around line 281-285: The local array filtered_formulas is declared but not
initialized, which triggers an unbound variable error under set -u; change the
declaration of filtered_formulas in this function so it is initialized to an
empty array before the while loop (replace the current local -a
filtered_formulas with an initialized local filtered_formulas), keeping the rest
of the loop that reads from macgnu_filter_formulas and assigns back to formulas
unchanged.
- Around line 78-81: The array variable filtered is not initialized, so under
set -u the `${filtered[@]}` expansion is unbound; initialize filtered as an
empty array before it’s ever used (e.g., add an explicit initialization for
filtered before the loop that appends to it and before the `[[ ${`#filtered`[@]}
-gt 0 ]]` check) so subsequent `filtered+=("$formula")` and `printf '%s\n'
"${filtered[@]}"` work safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 179-189: The user-facing echo currently claims the script edited
shell RC files but the code only copies the .macgnu file; update the message in
the block that references script_dir and the copied ~/.macgnu to accurately
reflect the action (e.g., "✓ Copied .macgnu to ~/.macgnu" or similar), or
implement explicit logic to append "source ~/.macgnu" to common RC files
(~/.bashrc, ~/.zshrc) if you want the original claim to be true; change the echo
line that mentions ~/.bashrc and ~/.zshrc accordingly and keep references to
script_dir and .macgnu consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@macgnu`:
- Around line 243-244: The uninstall message is misleading: the code only runs
"rm -f ~/.macgnu" but the echo claims it removed "'. ~/.macgnu' from your
~/.bashrc, ~/.zshrc...". Update the echo to accurately reflect what the script
does (e.g., "✓ Removed ~/.macgnu") or, if you intend to remove sourcing lines,
add logic to edit shell RC files to remove the " . ~/.macgnu" line; locate the
"rm -f ~/.macgnu" invocation and the following echo statement and make the
message and behavior consistent.
| rm -f ~/.macgnu | ||
| echo "Remove '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc or your shell's equivalent config file" | ||
| echo "✓ Removed '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc or your shell's equivalent config file" |
There was a problem hiding this comment.
Misleading uninstall message - same issue as the install message that was fixed.
Line 244 states the script "Removed '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc..." but the code at line 243 only removes ~/.macgnu itself. The script never modifies shell RC files. This mirrors the issue that was corrected in the install function (lines 186-187).
💡 Suggested fix for consistency with install message
# Remove changes to PATH/MANPATH/INFOPATH/LDFLAGS/CPPFLAGS
rm -f ~/.macgnu
- echo "✓ Removed '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc or your shell's equivalent config file"
+ echo "✓ Removed ~/.macgnu"
+ echo "Remove '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc, or equivalent config file if added"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -f ~/.macgnu | |
| echo "Remove '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc or your shell's equivalent config file" | |
| echo "✓ Removed '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc or your shell's equivalent config file" | |
| rm -f ~/.macgnu | |
| echo "✓ Removed ~/.macgnu" | |
| echo "Remove '. ~/.macgnu' from your ~/.bashrc, ~/.zshrc, or equivalent config file if added" |
🤖 Prompt for AI Agents
In `@macgnu` around lines 243 - 244, The uninstall message is misleading: the code
only runs "rm -f ~/.macgnu" but the echo claims it removed "'. ~/.macgnu' from
your ~/.bashrc, ~/.zshrc...". Update the echo to accurately reflect what the
script does (e.g., "✓ Removed ~/.macgnu") or, if you intend to remove sourcing
lines, add logic to edit shell RC files to remove the " . ~/.macgnu" line;
locate the "rm -f ~/.macgnu" invocation and the following echo statement and
make the message and behavior consistent.
1 participant
Closes #
📑 Description
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.