ci: enforce consistent shell formatting with shfmt#514
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds editorconfig formatting settings, a GitHub Actions format job using shfmt, documents shfmt in CONTRIBUTING, and applies widespread shell whitespace/quoting/indentation normalizations across scripts, completions, plugin code, and tests. No public API signatures were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Runner as "ubuntu-latest runner"
participant Setup as "mfinelli/setup-shfmt@v4"
participant Shfmt as "shfmt v3.13.1"
participant Repo as "Repository"
GH->>Runner: trigger "format" job
Runner->>Repo: checkout repository
Runner->>Setup: install shfmt
Setup->>Shfmt: provide shfmt binary
Runner->>Shfmt: run `shfmt --diff .`
Shfmt-->>Runner: return formatting diffs
Runner-->>GH: report job status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
57-66:⚠️ Potential issue | 🟡 MinorUse CI-equivalent
shfmtcheck command in Local Validation.This section says commands should match CI, but CI uses
shfmt --list .while docs currently instructshfmt --write ..💡 Suggested documentation tweak
-shellcheck forgit.plugin.sh bin/git-forgit -lib/bashunit . -bash forgit.plugin.sh -zsh forgit.plugin.zsh -fish conf.d/forgit.plugin.fish -shfmt --write . +shellcheck forgit.plugin.sh bin/git-forgit +lib/bashunit . +bash forgit.plugin.sh +zsh forgit.plugin.zsh +fish conf.d/forgit.plugin.fish +shfmt --list . +# Optional auto-fix before rerunning checks: +# shfmt --write .Based on learnings: Align local validation with CI workflow (which runs on macOS and Ubuntu) before opening a PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 57 - 66, Replace the local validation shfmt command to match CI by changing the occurrence of "shfmt --write ." to "shfmt --list ." in the CONTRIBUTING.md instructions; ensure the example command list now shows "shfmt --list ." so local pre-PR checks mirror the CI workflow (which uses shfmt --list).
🧹 Nitpick comments (2)
.editorconfig (1)
9-11: Scope the shfmt options to shell files instead of[*].
indent_size = 4now becomes the default for every file type, even though this PR is only enforcing shell formatting. I’d move the shell-specific formatter knobs into shell/completion sections (plusbin/git-forgit) so unrelated files don’t inherit shell defaults.♻️ Suggested config split
[*] charset = utf-8 end_of_line = lf indent_style = space insert_final_newline = true tab_width = 4 -indent_size = 4 -simplify = true -switch_case_indent = true trim_trailing_whitespace = true + +[bin/git-forgit] +indent_size = 4 +simplify = true +switch_case_indent = true + +[*.{sh,bash,zsh,fish}] +indent_size = 4 +simplify = true +switch_case_indent = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.editorconfig around lines 9 - 11, The global .editorconfig currently sets shfmt options (indent_size, simplify, switch_case_indent) at the root which applies to all files; move these keys into shell-specific sections so only shell files get them—create/modify a [*.sh] (and add a specific section for bin/git-forgit if it lacks a .sh extension, e.g., [bin/git-forgit]) and place indent_size = 4, simplify = true, and switch_case_indent = true there so unrelated files do not inherit shell formatting defaults.tests/helper-functions.test.sh (1)
15-17: Useprintffor exact fixture round-tripping.This test is comparing raw parser output.
echo -n/echoare shell-option dependent, so future cases like-nfooor escape sequences can get distorted before the assertion runs.♻️ Safer test harness
- done < <(echo -n "$input" | _forgit_get_files_from_diff_line | xargs -0 -n 1 echo) + done < <(printf '%s' "$input" | _forgit_get_files_from_diff_line | xargs -0 -n 1 printf '%s\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helper-functions.test.sh` around lines 15 - 17, The test uses echo -n to feed raw fixture data into _forgit_get_files_from_diff_line which can mangle inputs (leading -n, backslashes, etc.); replace the echo invocation with printf '%s' "$input" so the exact bytes of the fixture are passed through for _forgit_get_files_from_diff_line and subsequent xargs handling, ensuring exact round-tripping in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 92-96: The workflow's Check format step uses an invalid list under
the with key for the mfinelli/setup-shfmt@v3 action; change the with input to a
key/value mapping (e.g., set shfmt-version: "3.13.1") and ensure the subsequent
run step remains as run: shfmt --list . so the action receives the correct
shfmt-version parameter.
In `@bin/git-forgit`:
- Line 177: The git invocation in bin/git-forgit uses git ls-files "$1"
--error-unmatch which treats filenames beginning with '-' as options; change the
command to pass the filename after the option terminator by invoking git
ls-files -- "$1" --error-unmatch (update the invocation where git ls-files is
called in the script) so paths like "-foo" are handled correctly.
---
Outside diff comments:
In `@CONTRIBUTING.md`:
- Around line 57-66: Replace the local validation shfmt command to match CI by
changing the occurrence of "shfmt --write ." to "shfmt --list ." in the
CONTRIBUTING.md instructions; ensure the example command list now shows "shfmt
--list ." so local pre-PR checks mirror the CI workflow (which uses shfmt
--list).
---
Nitpick comments:
In @.editorconfig:
- Around line 9-11: The global .editorconfig currently sets shfmt options
(indent_size, simplify, switch_case_indent) at the root which applies to all
files; move these keys into shell-specific sections so only shell files get
them—create/modify a [*.sh] (and add a specific section for bin/git-forgit if it
lacks a .sh extension, e.g., [bin/git-forgit]) and place indent_size = 4,
simplify = true, and switch_case_indent = true there so unrelated files do not
inherit shell formatting defaults.
In `@tests/helper-functions.test.sh`:
- Around line 15-17: The test uses echo -n to feed raw fixture data into
_forgit_get_files_from_diff_line which can mangle inputs (leading -n,
backslashes, etc.); replace the echo invocation with printf '%s' "$input" so the
exact bytes of the fixture are passed through for
_forgit_get_files_from_diff_line and subsequent xargs handling, ensuring exact
round-tripping in tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cea9a1d-5fb2-4abe-946d-5a85c4380d1d
📒 Files selected for processing (11)
.editorconfig.github/workflows/ci.yamlCONTRIBUTING.mdbin/git-forgitcompletions/git-forgit.bashforgit.plugin.zshtests/clean-select-files.test.shtests/empty-state.test.shtests/helper-functions.test.shtests/preview-context.test.shtests/working-tree-changes.test.sh
sandr01d
force-pushed
the
shfmt
branch
5 times, most recently
from
f5d35e1 to
9d01fe9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/git-forgit (1)
176-178:⚠️ Potential issue | 🟡 MinorPass the filename after
--to handle edge cases.The argument order issue persists: a tracked file named
-foowill be treated as an option. The--separator should precede the filename.🐛 Minimal fix
_forgit_is_file_tracked() { - git ls-files "$1" --error-unmatch &>/dev/null + git ls-files --error-unmatch -- "$1" &>/dev/null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` around lines 176 - 178, The helper _forgit_is_file_tracked uses `git ls-files "$1" --error-unmatch` which treats filenames starting with `-` as options; update the call inside _forgit_is_file_tracked to pass the filename after the `--` separator (i.e., `git ls-files --error-unmatch -- "$1"`), so filenames like `-foo` are handled correctly and not parsed as options.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
82-91: Useshfmt --diff .for better CI feedback on formatting issues.The format job configuration is correct. However,
shfmt --list .only prints filenames that differ. Usingshfmt --diff .(or-d) would additionally display the actual formatting changes, making it easier for contributors to understand what needs fixing without running shfmt locally.💡 Suggested improvement
- name: Check format uses: mfinelli/setup-shfmt@v4 with: shfmt-version: 3.13.1 - - run: shfmt --list . + - run: shfmt --diff .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 82 - 91, Update the format job to use shfmt's diff output so contributors see exact formatting changes: in the "format" job where the step runs `shfmt --list .`, replace that invocation with `shfmt --diff .` (or `shfmt -d .`) so the CI prints unified diffs; ensure the step name ("Check format") and the mfinelli/setup-shfmt@v4 setup remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/git-forgit`:
- Around line 176-178: The helper _forgit_is_file_tracked uses `git ls-files
"$1" --error-unmatch` which treats filenames starting with `-` as options;
update the call inside _forgit_is_file_tracked to pass the filename after the
`--` separator (i.e., `git ls-files --error-unmatch -- "$1"`), so filenames like
`-foo` are handled correctly and not parsed as options.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 82-91: Update the format job to use shfmt's diff output so
contributors see exact formatting changes: in the "format" job where the step
runs `shfmt --list .`, replace that invocation with `shfmt --diff .` (or `shfmt
-d .`) so the CI prints unified diffs; ensure the step name ("Check format") and
the mfinelli/setup-shfmt@v4 setup remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69fa0010-3a20-4f4b-aabf-36b149cb1c07
📒 Files selected for processing (11)
.editorconfig.github/workflows/ci.yamlCONTRIBUTING.mdbin/git-forgitcompletions/git-forgit.bashforgit.plugin.zshtests/clean-select-files.test.shtests/empty-state.test.shtests/helper-functions.test.shtests/preview-context.test.shtests/working-tree-changes.test.sh
✅ Files skipped from review due to trivial changes (9)
- tests/empty-state.test.sh
- CONTRIBUTING.md
- .editorconfig
- tests/helper-functions.test.sh
- tests/clean-select-files.test.sh
- completions/git-forgit.bash
- tests/preview-context.test.sh
- forgit.plugin.zsh
- tests/working-tree-changes.test.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/git-forgit (1)
110-129:⚠️ Potential issue | 🟠 MajorUse shell escaping here instead of wrapping paths in single quotes.
files+=("'$1'")breaks as soon as a path contains'. That output is later interpolated into fzf execute/preview command strings, where unescaped quotes produce syntactically invalid shell commands.Suggested fix
_forgit_quote_files() { - local files add + local add escaped files files=() add=false while (("$#")); do case "$1" in --) add=true shift ;; *) - if [ $add == true ]; then - files+=("'$1'") + if [[ $add == true ]]; then + printf -v escaped '%q' "$1" + files+=("$escaped") fi shift ;; esac done echo "${files[*]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` around lines 110 - 129, _in_forgit_quote_files, file paths are incorrectly wrapped with literal single quotes via files+=("'$1'"), which breaks on paths containing single quotes; replace that naive quoting with proper shell escaping (e.g. use printf '%q' on "$1" or otherwise escape single quotes to '\'' before appending) so each entry in files is a safely escaped token, update the append site (files+=(...)) accordingly and keep the final echo as echo "${files[*]}" to emit the escaped tokens for later fzf command interpolation.
♻️ Duplicate comments (1)
bin/git-forgit (1)
177-177:⚠️ Potential issue | 🟡 MinorPass the path after
--ingit ls-files.A tracked file named like
-foois still treated as an option here, so this helper returns the wrong answer for exactly the edge case it is meant to handle.Minimal fix
- git ls-files "$1" --error-unmatch &>/dev/null + git ls-files --error-unmatch -- "$1" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-forgit` at line 177, The git ls-files invocation treats filenames beginning with '-' as options; update the call that currently reads git ls-files "$1" --error-unmatch &>/dev/null to pass the path after the option terminator so options are separated from the filename (i.e., use the -- separator and supply "$1" after it), leaving the rest of the logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/git-forgit`:
- Around line 110-129: _in_forgit_quote_files, file paths are incorrectly
wrapped with literal single quotes via files+=("'$1'"), which breaks on paths
containing single quotes; replace that naive quoting with proper shell escaping
(e.g. use printf '%q' on "$1" or otherwise escape single quotes to '\'' before
appending) so each entry in files is a safely escaped token, update the append
site (files+=(...)) accordingly and keep the final echo as echo "${files[*]}" to
emit the escaped tokens for later fzf command interpolation.
---
Duplicate comments:
In `@bin/git-forgit`:
- Line 177: The git ls-files invocation treats filenames beginning with '-' as
options; update the call that currently reads git ls-files "$1" --error-unmatch
&>/dev/null to pass the path after the option terminator so options are
separated from the filename (i.e., use the -- separator and supply "$1" after
it), leaving the rest of the logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b5c111c-79b4-4ddf-8378-f56ee94ebdc6
📒 Files selected for processing (11)
.editorconfig.github/workflows/ci.yamlCONTRIBUTING.mdbin/git-forgitcompletions/git-forgit.bashforgit.plugin.zshtests/clean-select-files.test.shtests/empty-state.test.shtests/helper-functions.test.shtests/preview-context.test.shtests/working-tree-changes.test.sh
✅ Files skipped from review due to trivial changes (6)
- tests/empty-state.test.sh
- CONTRIBUTING.md
- tests/clean-select-files.test.sh
- tests/preview-context.test.sh
- tests/working-tree-changes.test.sh
- .editorconfig
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yaml
- tests/helper-functions.test.sh
- completions/git-forgit.bash
carlfriedrich
approved these changes
Apr 9, 2026
Collaborator
carlfriedrich
left a comment
carlfriedrich
left a comment
There was a problem hiding this comment.
@sandr01d Thanks a lot for your work! I had this on my wishlist for forgit for some time as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check list
Description
Enforce a consistent shell script file using shfmt. This requires
shfmtv3.13.1 to be able to correctly parse the zsh specific syntax in forgit.plugin.zsh. I kept the formatting as close as possible to what we currently have. Of course I am open for discussion should anyone want to have a different configuration.Type of change
Test environment
Summary by CodeRabbit
Chores
Documentation
Style