refactor: split utility.sh into per-verb modules in lib/#44
Conversation
|
@greptile |
|
/gemini review |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Greptile SummaryThis PR refactors the monolithic 1480-line Confidence Score: 4/5Safe to merge structurally, but open P1-level completion bugs from prior review threads should be addressed first. The refactoring itself is clean and well-executed — module boundaries are clear, the loader correctly guards every lib/completions.sh requires the most attention due to unresolved issues from prior review threads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["source utility.sh"] --> B["Compute _U7_DIR"]
B --> C{"for each module in load order"}
C -->|core| D["lib/core.sh"]
C -->|show| E["lib/show.sh"]
C -->|make| F["lib/make.sh"]
C -->|drop| G["lib/drop.sh"]
C -->|convert| H["lib/convert.sh"]
C -->|move| I["lib/move.sh"]
C -->|set| J["lib/set.sh"]
C -->|run| K["lib/run.sh"]
C -->|completions| L["lib/completions.sh"]
C -->|load failure| M["echo error >&2 / unset / return 1"]
D & E & F & G & H & I & J & K & L --> N["unset _u7_mod _U7_DIR / define u7"]
N --> O["u7 verb entity ..."]
O --> P{"verb dispatch"}
P -->|sh| E
P -->|mk| F
P -->|dr| G
P -->|cv| H
P -->|mv| I
P -->|st| J
P -->|rn| K
Reviews (8): Last reviewed commit: "fix: address Greptile review feedback fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully refactors the utility.sh script by splitting its functionality into several smaller, verb-specific modules within the lib/ directory. This significantly improves modularity and maintainability, making the codebase easier to navigate and extend. The .gitignore update correctly reflects the new file structure. While the refactoring is well-executed, several areas require attention regarding security, portability, and best practices in shell scripting. Specifically, direct use of sudo within functions, command injection vulnerabilities due to eval and unquoted variables, and reliance on Bash 4+ features and GNU sed syntax introduce critical and high-severity issues that should be addressed.
| local value="${time//[^0-9]/}" | ||
|
|
||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| case "$unit" in |
There was a problem hiding this comment.
Using eval "$cmd" is a critical security vulnerability. If $cmd contains any user-controlled input, it allows for arbitrary code execution. This should be avoided. Instead, consider using bash -c "$cmd" or carefully sanitizing the input if eval is absolutely necessary, though it's rarely the safest option.
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] $cmd &" | ||
| else | ||
| eval "$cmd" & |
There was a problem hiding this comment.
Using eval "$cmd" & is another critical security vulnerability. Similar to the job entity, if $cmd contains user-controlled input, it allows for arbitrary code execution. This should be avoided. If running a command in the background is needed, ensure the command is constructed safely without eval.
| _u7_exec tar -cJvf "$output" "$@" | ||
| else | ||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] xz -c $* > $output" |
There was a problem hiding this comment.
Using unquoted $* can lead to word splitting and globbing issues if filenames contain spaces or special characters. It's safer to use "$@" to ensure each argument is passed as a separate, properly quoted word to the command.
| echo "[dry-run] xz -c $* > $output" | |
| echo "[dry-run] xz -c \"$@\" > $output" |
| fi | ||
| ;; | ||
| *.iso) | ||
| _u7_exec sudo mount -o loop "$archive" "$dest" ;; |
There was a problem hiding this comment.
Directly calling sudo mount within the script can lead to unexpected behavior or security prompts if the user does not have NOPASSWD configured for this command. It's generally better to let the user invoke the entire script with sudo if elevated privileges are required for its operation, or to provide clear instructions for setting up sudoers entries if specific commands must be run with sudo non-interactively.
| echo "Usage: u7 dr user <username>" | ||
| return 1 | ||
| fi | ||
| _u7_exec sudo deluser "$1" |
There was a problem hiding this comment.
Similar to lib/convert.sh, directly calling sudo deluser within the script can lead to unexpected behavior or security prompts. It's generally better to let the user invoke the entire script with sudo if elevated privileges are required for its operation, or to provide clear instructions for setting up sudoers entries if specific commands must be run with sudo non-interactively.
| fi | ||
|
|
||
| for file in "${matched_files[@]}"; do | ||
| sed -i'' "s/$old_escaped/$new_escaped/g" "$file" |
There was a problem hiding this comment.
The sed -i'' syntax for in-place editing without a backup file is specific to GNU sed. On systems using BSD sed (like macOS), this syntax will treat '' as the backup file extension, potentially leading to unexpected behavior or errors. For cross-platform compatibility, consider using sed -i.bak (which creates a backup) or sed -i '' (with a space, for BSD sed to mean no backup) and handling the backup file if necessary.
| sed -i'' "s/$old_escaped/$new_escaped/g" "$file" | |
| sed -i '' "s/$old_escaped/$new_escaped/g" "$file" |
| sed -i'' "s/$old_escaped/$new_escaped/g" "$file" | ||
| done | ||
| else | ||
| sed -i'' "s/$old_escaped/$new_escaped/g" "$target" |
There was a problem hiding this comment.
The sed -i'' syntax for in-place editing without a backup file is specific to GNU sed. On systems using BSD sed (like macOS), this syntax will treat '' as the backup file extension, potentially leading to unexpected behavior or errors. For cross-platform compatibility, consider using sed -i.bak (which creates a backup) or sed -i '' (with a space, for BSD sed to mean no backup) and handling the backup file if necessary.
| sed -i'' "s/$old_escaped/$new_escaped/g" "$target" | |
| sed -i '' "s/$old_escaped/$new_escaped/g" "$target" |
| count="$2" | ||
| fi | ||
| local term_cmd | ||
| term_cmd=$(ps -o comm= -p "$(($(ps -o ppid= -p "$(($(ps -o sid= -p "$$")))")))") |
There was a problem hiding this comment.
The command ps -o comm= -p "$(($(ps -o ppid= -p "$(($(ps -o sid= -p "$$")))")))" is overly complex and relies heavily on specific ps output formats and process tree traversal, which can be fragile and non-portable across different Unix-like systems or ps versions. This could lead to unexpected failures or incorrect terminal detection.
| return 1 | ||
| fi | ||
| # Convert only basename to lowercase, preserve extension case | ||
| _u7_exec rename 's/^(.*)(\.\w+)$/lc($1) . $2/e' "${@:5}" |
There was a problem hiding this comment.
The rename command with the /e flag executes Perl code. While the current usage appears to construct the regex and replacement string internally, using eval-like behavior with potentially user-controlled input (even if indirectly through arguments) can be a security risk. Ensure that all inputs to the rename command are thoroughly sanitized to prevent arbitrary code execution.
lib/convert.sh
Outdated
| return 1 | ||
| fi | ||
|
|
||
| local lowercase=$(echo "$archive" | tr '[:upper:]' '[:lower:]') |
There was a problem hiding this comment.
Using echo "$archive" | tr '[:upper:]' '[:lower:]' creates an unnecessary subshell and external process for a simple string transformation. Bash has built-in parameter expansion for case conversion which is more efficient and idiomatic.
Consider using lowercase=${archive,,} for Bash 4+ or lowercase=$(echo "$archive" | tr '[:upper:]' '[:lower:]') if Bash 3 compatibility is strictly required, but the current approach is less efficient.
| local lowercase=$(echo "$archive" | tr '[:upper:]' '[:lower:]') | |
| local lowercase=${archive,,} |
There was a problem hiding this comment.
Code Review
This pull request is an excellent refactoring effort, splitting a large shell script into smaller, more manageable modules. This greatly improves the code's readability and maintainability. The changes are well-structured, and the modularization is logical. I've identified a few areas for improvement, primarily related to shell scripting best practices. These include enhancing the robustness of dry-run outputs by correctly quoting variables, preventing potential issues with filenames containing spaces, and replacing a potentially unsafe command with a more secure alternative. Addressing these points will make the script more robust and reliable.
| if [[ -n "$4" ]]; then | ||
| _u7_exec rename 'y/ /_/' "$4" | ||
| else | ||
| _u7_exec rename 'y/ /_/' -- * | ||
| fi |
There was a problem hiding this comment.
The current implementation for converting spaces to underscores in filenames defaults to operating on all files in the current directory (*) if no file is specified. This is potentially destructive and contradicts the usage string u7 cv spaces to underscores on <file>, which implies a file argument is mandatory. It would be safer to enforce that a file path is provided.
| if [[ -n "$4" ]]; then | |
| _u7_exec rename 'y/ /_/' "$4" | |
| else | |
| _u7_exec rename 'y/ /_/' -- * | |
| fi | |
| if [[ -z "$4" ]]; then | |
| echo "Usage: u7 cv spaces to underscores on <file>" | |
| return 1 | |
| fi | |
| _u7_exec rename 'y/ /_/' "$4" |
| if [[ "$1" == "but" ]]; then | ||
| local pattern="$2" | ||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] find . -type f ! -name $pattern -delete" |
There was a problem hiding this comment.
The $pattern variable is not quoted in the dry-run output. If the pattern contains spaces or other special shell characters, the output will be misleading or incorrect. Quoting the variable ensures the dry-run log accurately reflects the command that would be run. This issue is present in several dry-run commands throughout the scripts.
| echo "[dry-run] find . -type f ! -name $pattern -delete" | |
| echo "[dry-run] find . -type f ! -name '$pattern' -delete" |
| _u7_exec tar -czvf "$output" "$@" | ||
| else | ||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] gzip -c $* > $output" |
There was a problem hiding this comment.
The dry-run output for creating an archive uses $* which will not handle filenames with spaces correctly. Using $(printf '%q ' "$@") will ensure that all file arguments are properly quoted and displayed, giving an accurate representation of the command.
| echo "[dry-run] gzip -c $* > $output" | |
| echo "[dry-run] gzip -c $(printf '%q ' "$@") > $output" |
|
|
||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| if [[ -d "$target" ]]; then | ||
| echo "[dry-run] grep -rlF '$old' $target | xargs sed -i'' 's/$old_escaped/$new_escaped/g'" |
There was a problem hiding this comment.
The $target variable is unquoted in the dry-run output. This can be misleading if the path contains spaces. Quoting it will ensure the dry-run output is accurate.
| echo "[dry-run] grep -rlF '$old' $target | xargs sed -i'' 's/$old_escaped/$new_escaped/g'" | |
| echo "[dry-run] grep -rlF '$old' '$target' | xargs sed -i'' 's/$old_escaped/$new_escaped/g'" |
| for k in $(git branch | sed 's/^..//'); do | ||
| echo -e "$(git show --pretty=format:"%ci %cr" "$k" -- | head -n 1)\t$k" | ||
| done | sort -r |
There was a problem hiding this comment.
The for k in $(...) construct will break on branch names that contain spaces due to word splitting. Using a while read loop is a more robust way to process command output line by line and will correctly handle such cases.
| for k in $(git branch | sed 's/^..//'); do | |
| echo -e "$(git show --pretty=format:"%ci %cr" "$k" -- | head -n 1)\t$k" | |
| done | sort -r | |
| git branch | sed 's/^..//' | while IFS= read -r k; do | |
| echo -e "$(git show --pretty=format:\"%ci %cr\" \"$k\" -- | head -n 1)\t$k" | |
| done | sort -r |
lib/convert.sh
Outdated
| return 1 | ||
| fi | ||
|
|
||
| local lowercase=$(echo "$archive" | tr '[:upper:]' '[:lower:]') |
There was a problem hiding this comment.
Using echo and tr to convert a string to lowercase creates an unnecessary subshell. Bash 4.0+ provides a more performant and idiomatic way to do this with parameter expansion.
| local lowercase=$(echo "$archive" | tr '[:upper:]' '[:lower:]') | |
| local lowercase="${archive,,}" |
|
|
||
| _u7_exec() { | ||
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] $*" |
There was a problem hiding this comment.
The dry-run output using echo "[dry-run] $*" doesn't correctly represent arguments with spaces or special characters, which can be misleading. Using printf with the %q format specifier will produce a shell-quoted, unambiguous representation of the command and its arguments, making the dry-run output much more reliable.
| echo "[dry-run] $*" | |
| echo "[dry-run] $(printf '%q ' "$@")" |
| s) sleep "$value" && eval "$cmd" & ;; | ||
| m) sleep "$((value * 60))" && eval "$cmd" & ;; | ||
| h) sleep "$((value * 3600))" && eval "$cmd" & ;; |
There was a problem hiding this comment.
Using eval can be risky, especially with user-provided input. For consistency with other parts of the script (like the with priority case) and for better security, it's safer to execute the command within a new shell using bash -c.
| s) sleep "$value" && eval "$cmd" & ;; | |
| m) sleep "$((value * 60))" && eval "$cmd" & ;; | |
| h) sleep "$((value * 3600))" && eval "$cmd" & ;; | |
| s) sleep "$value" && bash -c "$cmd" & ;; | |
| m) sleep "$((value * 60))" && bash -c "$cmd" & ;; | |
| h) sleep "$((value * 3600))" && bash -c "$cmd" & ;; |
| if [[ "$_U7_DRY_RUN" == "1" ]]; then | ||
| echo "[dry-run] $cmd &" | ||
| else | ||
| eval "$cmd" & |
There was a problem hiding this comment.
Using eval can be risky. To improve security and maintain consistency with other commands in this script, consider using bash -c to execute the command in a separate shell process.
| eval "$cmd" & | |
| bash -c "$cmd" & |
|
@greptile |
|
@greptile |
| case "$entity" in | ||
| check) COMPREPLY=($(compgen -W "syntax" -- "$cur")) ; _filedir ;; | ||
| script) _filedir ;; | ||
| esac |
There was a problem hiding this comment.
image/video/json/csv entities never matched by completion rules
The png|jpg|jpeg|gif pattern matches the entity word (the second argument after the verb), but _u7_convert uses image as the entity name for image conversion — not the file extension. So u7 cv image foo.png <tab> falls through to *) _filedir ;; and never offers to as a completion.
The same gap applies to video, json, and csv which are also real entities in lib/convert.sh but are absent from this case statement.
| esac | |
| archive|files|image|video|json|csv) COMPREPLY=($(compgen -W "to" -- "$cur")) ;; |
The old png|jpg|jpeg|gif pattern can be removed — those are never used as the entity word.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 80
Comment:
**`image`/`video`/`json`/`csv` entities never matched by completion rules**
The `png|jpg|jpeg|gif` pattern matches the *entity* word (the second argument after the verb), but `_u7_convert` uses `image` as the entity name for image conversion — not the file extension. So `u7 cv image foo.png <tab>` falls through to `*) _filedir ;;` and never offers `to` as a completion.
The same gap applies to `video`, `json`, and `csv` which are also real entities in `lib/convert.sh` but are absent from this case statement.
```suggestion
archive|files|image|video|json|csv) COMPREPLY=($(compgen -W "to" -- "$cur")) ;;
```
The old `png|jpg|jpeg|gif` pattern can be removed — those are never used as the entity word.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| local verbs="show sh make mk drop dr convert cv move mv set st run rn --help" | ||
| local opts="-n --dry-run" | ||
|
|
There was a problem hiding this comment.
_filedir overwrites the syntax completion
_filedir replaces COMPREPLY entirely, so the syntax suggestion set on the same line is always discarded. A user typing u7 rn check <tab> will see only filenames, never syntax.
| check) COMPREPLY=($(compgen -W "syntax" -- "$cur")) ;; |
File-path completion is only needed at a deeper position (after syntax in file), and it already falls through to the *) _filedir ;; fallback there.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 96
Comment:
**`_filedir` overwrites the `syntax` completion**
`_filedir` replaces `COMPREPLY` entirely, so the `syntax` suggestion set on the same line is always discarded. A user typing `u7 rn check <tab>` will see only filenames, never `syntax`.
```suggestion
check) COMPREPLY=($(compgen -W "syntax" -- "$cur")) ;;
```
File-path completion is only needed at a deeper position (after `syntax in file`), and it already falls through to the `*) _filedir ;;` fallback there.
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile |
| ;; | ||
| run|rn) | ||
| COMPREPLY=($(compgen -W "job script check terminal --help" -- "$cur")) | ||
| ;; | ||
| esac | ||
| } | ||
|
|
||
| _u7_complete_args() { | ||
| local verb="$1" entity="$2" cur="$3" | ||
| case "$verb" in | ||
| show|sh) | ||
| case "$entity" in | ||
| ip) COMPREPLY=($(compgen -W "external internal connected" -- "$cur")) ;; | ||
| processes) COMPREPLY=($(compgen -W "running by" -- "$cur")) ;; | ||
| files) COMPREPLY=($(compgen -W "match by" -- "$cur")) ;; | ||
| usage) COMPREPLY=($(compgen -W "disk directories" -- "$cur")) ;; | ||
| git) COMPREPLY=($(compgen -W "authors branches tags log status diff remotes" -- "$cur")) ;; | ||
| env) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; | ||
| http) COMPREPLY=($(compgen -W "get head headers" -- "$cur")) ;; | ||
| docker) COMPREPLY=($(compgen -W "containers images volumes networks all" -- "$cur")) ;; | ||
| *) _filedir ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
COMPREPLY word-splitting on special characters in $cur
Every COMPREPLY=($(compgen -W "..." -- "$cur")) assignment is subject to word-splitting and glob expansion on the result. If $cur contains spaces, tabs, or glob metacharacters the array can silently grow extra elements or collapse. The safe idiom is to use mapfile:
mapfile -t COMPREPLY < <(compgen -W "ip csv json line ssl ..." -- "$cur")This applies throughout both _u7_complete_entities (every compgen call on lines 23, 26, 29, 32, 35, 38, 41) and _u7_complete_args (lines 51–98).
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 21-43
Comment:
**`COMPREPLY` word-splitting on special characters in `$cur`**
Every `COMPREPLY=($(compgen -W "..." -- "$cur"))` assignment is subject to word-splitting and glob expansion on the result. If `$cur` contains spaces, tabs, or glob metacharacters the array can silently grow extra elements or collapse. The safe idiom is to use `mapfile`:
```bash
mapfile -t COMPREPLY < <(compgen -W "ip csv json line ssl ..." -- "$cur")
```
This applies throughout both `_u7_complete_entities` (every `compgen` call on lines 23, 26, 29, 32, 35, 38, 41) and `_u7_complete_args` (lines 51–98).
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile |
2 similar comments
|
@greptile |
|
@greptile |
|
@greptile |
1 participant
Summary
lib/from .gitignore (was a Python template artifact)File structure
Test plan
u7 --helpworks