Support glob expansion in trash() - #46287
Conversation
Enhance the trash() implementation to detect glob patterns in the target path, expand them, and move each matched file into the user's .Trash with a timestamp and random suffix. If no matches are found the function logs that the pattern doesn't exist.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46287 +/- ##
==========================================
- Coverage 66.78% 66.74% -0.04%
==========================================
Files 2803 2773 -30
Lines 223565 222726 -839
Branches 11345 11047 -298
==========================================
- Hits 149309 148662 -647
+ Misses 60693 60501 -192
Partials 13563 13563
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR updates the Homebrew uninstall script generator’s trash() helper to recognize glob patterns in the target path, expand them, and move matched files into the logged-in user’s ~/.Trash using a timestamped name.
Changes:
- Detect glob metacharacters in
target_fileand iterate over expanded matches. - Move each match to
~/.Trashand log a message when no matches exist.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR extends the embedded trash() shell function to detect glob characters in target paths. If present, it expands the pattern with compgen -G, iterates matched paths, skips missing entries, and moves each match into ~/.Trash with a unique basename suffixed by timestamp, rand, and an index while logging each removal. If no matches are found it logs that the target doesn't exist and returns. Non-glob targets retain the original single-path behavior. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/maintained-apps/ingesters/homebrew/scripts.go (1)
674-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-match destination names are not unique and can overwrite files.
timestampandrandare computed once, so multiple matches with the same basename can collide at the same Trash path (mv -fwill overwrite). Generate uniqueness per file move.Suggested fix
- local timestamp="$(date +%Y-%m-%d-%s)" - local rand="$(jot -r 1 0 99999)" + local timestamp="$(date +%Y-%m-%d-%s)" @@ - local file_name="$(basename "$f")" + local file_name="$(basename "$f")" + local rand="$(jot -r 1 0 99999)" echo "removing $f." mv -f "$f" "$trash/${file_name}_${timestamp}_${rand}"Also applies to: 692-695
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/maintained-apps/ingesters/homebrew/scripts.go` around lines 674 - 675, The per-match destination name uses shared `timestamp` and `rand` variables so multiple moves can collide and `mv -f` may overwrite; change the logic so `timestamp` and/or `rand` (or a unique id like `$(date +%s%N)` or `mktemp` UUID) are generated inside the per-file handling block right before performing the `mv -f` to the Trash path, ensuring each move computes a fresh unique suffix for the destination name (update both the occurrences where `timestamp`/`rand` are used, e.g., the block around the variables `timestamp` and `rand` and the later moves at the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 687-690: The for-loop over $target_file is subject to
word-splitting before globbing which breaks paths with spaces (variable:
target_file, loop: for f in $target_file); change the loop so the glob is
expanded as a single pattern rather than split words — for example, replace the
current loop with a safe expansion using eval (e.g. eval "for f in $target_file;
do ...; done") or explicitly expand into an array via a null-delimited expansion
before iterating, ensuring you reference target_file and the for f loop so
globbing occurs correctly for paths containing spaces.
---
Outside diff comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 674-675: The per-match destination name uses shared `timestamp`
and `rand` variables so multiple moves can collide and `mv -f` may overwrite;
change the logic so `timestamp` and/or `rand` (or a unique id like `$(date
+%s%N)` or `mktemp` UUID) are generated inside the per-file handling block right
before performing the `mv -f` to the Trash path, ensuring each move computes a
fresh unique suffix for the destination name (update both the occurrences where
`timestamp`/`rand` are used, e.g., the block around the variables `timestamp`
and `rand` and the later moves at the other occurrence).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f54a4d7e-56a9-4fbd-8da2-786e75cf44ee
📒 Files selected for processing (1)
ee/maintained-apps/ingesters/homebrew/scripts.go
Fix glob expansion and safe handling in trash(): use compgen -G piped into a read -r loop so paths with spaces are preserved, add checks for existing files or symlinks, and introduce a per-match counter to avoid basename collisions when moving files to the trash. Also declare local variables and provide clearer comments explaining the approach and rationale.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Enhance the trash() implementation to detect glob patterns in the target path, expand them, and move each matched file into the user's .Trash with a timestamp and random suffix. If no matches are found the function logs that the pattern doesn't exist.
Summary by CodeRabbit
*,?,[]) to remove multiple matching files at once.