feat: add sh log entity for viewing/searching log files#50
Conversation
|
/gemini review |
![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
This pull request introduces a new log entity to the u7 sh command, enabling users to view, limit, match, or follow log files. The changes include shell completion support in lib/completions.sh, the core implementation in lib/show.sh, and a suite of new tests in test.sh. Review feedback suggests refactoring the usage string into a variable for better maintainability, making the numeric argument mandatory for the limit subcommand to ensure consistency with the match subcommand, and improving test isolation by using dedicated log files for specific test cases.
| local file="$1" | ||
| if [[ -z "$file" ]]; then | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
To improve maintainability and avoid repeating the usage string, you can store it in a variable at the beginning of the log block. This is consistent with how other entities like http are handled in this file. You can then reuse this variable in the default case of the case statement as well (line 284).
| local file="$1" | |
| if [[ -z "$file" ]]; then | |
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | |
| return 1 | |
| fi | |
| local file="$1" | |
| local usage="Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | |
| if [[ -z "$file" ]]; then | |
| echo "$usage" | |
| return 1 | |
| fi |
| local n="${3:-20}" | ||
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: limit must be a positive integer, got '$n'" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
When limit is used without a number (e.g., u7 sh log file.log limit), it currently defaults to 20 lines, which is the same as not providing any subcommand. This can be confusing. It's better to require a number when limit is specified and show an error if it's missing, for consistency with how match requires a pattern.
| local n="${3:-20}" | |
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | |
| echo "Error: limit must be a positive integer, got '$n'" | |
| return 1 | |
| fi | |
| if [[ -z "$3" ]]; then | |
| echo "Error: The 'limit' subcommand requires a number." | |
| echo "Usage: u7 sh log <file> limit <N>" | |
| return 1 | |
| fi | |
| local n="$3" | |
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | |
| echo "Error: limit must be a positive integer, got '$n'" | |
| return 1 | |
| fi |
| echo "ERROR something broke" >> "$LOG_FILE" | ||
| echo "INFO all good" >> "$LOG_FILE" | ||
| echo "ERROR another failure" >> "$LOG_FILE" | ||
| result=$(u7 sh log "$LOG_FILE" match ERROR 2>&1) |
There was a problem hiding this comment.
Modifying the shared $LOG_FILE in the middle of tests can lead to test inter-dependencies and make them harder to reason about. It's a good practice to ensure tests are independent. For this match test, consider creating a separate, dedicated log file instead of appending to the existing one.
| echo "ERROR something broke" >> "$LOG_FILE" | |
| echo "INFO all good" >> "$LOG_FILE" | |
| echo "ERROR another failure" >> "$LOG_FILE" | |
| result=$(u7 sh log "$LOG_FILE" match ERROR 2>&1) | |
| MATCH_LOG_FILE="$TEST_DIR/match_test.log" | |
| echo "ERROR something broke" > "$MATCH_LOG_FILE" | |
| echo "INFO all good" >> "$MATCH_LOG_FILE" | |
| echo "ERROR another failure" >> "$MATCH_LOG_FILE" | |
| result=$(u7 sh log "$MATCH_LOG_FILE" match ERROR 2>&1) |
Greptile SummaryThis PR adds a Key changes:
Two minor issues found: the Confidence Score: 5/5Safe to merge — all findings are minor style/validation P2 items that do not affect correctness of the primary user path. All three issues identified are P2: a validation edge-case (limit 0), a grep exit-code style concern, and a cosmetic completion ordering note. None block correct behaviour for the documented use cases, and the test suite is comprehensive for all interactive paths. lib/show.sh — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["u7 sh log <file> [subcommand]"] --> B{file provided?}
B -- No --> C["echo Usage; return 1"]
B -- Yes --> D{file exists?}
D -- No --> E["echo File not found; return 1"]
D -- Yes --> F{subcommand}
F -- empty --> G["tail -n 20 <file>"]
F -- limit --> H{n provided?}
H -- No --> I["n=20 (default)"]
H -- Yes --> J{valid integer?}
J -- No --> K["echo Error; return 1"]
J -- Yes --> L["tail -n n <file>"]
I --> L
F -- match --> M{pattern provided?}
M -- No --> N["echo Usage; return 1"]
M -- Yes --> O["grep -- pattern <file>"]
F -- follow --> P["tail -f <file>"]
F -- unknown --> Q["echo Usage; return 1"]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: lib/show.sh
Line: 264
Comment:
**`limit 0` bypasses the "positive integer" contract**
The regex `^[0-9]+$` matches `0`, so `u7 sh log <file> limit 0` silently runs `tail -n 0` and returns nothing with exit 0. The error message on the following line explicitly says "positive integer", but `0` is accepted. Users seeing silent empty output for `limit 0` are likely to be confused.
```suggestion
if ! [[ "$n" =~ ^[1-9][0-9]*$ ]]; then
```
This changes the regex to require at least one digit 1-9 optionally followed by more digits, which correctly excludes `0`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/show.sh
Line: 270-276
Comment:
**`grep` exit code propagates as function failure on no-match**
When `grep` finds no matching lines it exits with code `1`, which becomes the return value of `_u7_show`. A caller checking `$?` (or a script using `set -e`) will see this as a command failure even though "no results" is not an error. Consider suppressing or distinguishing the no-match exit code:
```suggestion
match)
if [[ -z "$3" ]]; then
echo "Usage: u7 sh log <file> match <pattern>"
return 1
fi
grep -- "$3" "$file" || true
;;
```
Appending `|| true` keeps the function exit code at `0` when there are simply no matches, while still showing any matched lines.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/completions.sh
Line: 41
Comment:
**Completion offers keywords at every argument position**
`_u7_complete_args` is called for any argument beyond the entity, so this line runs for both position 3 (where the user should type a `<file>`) and position 4+ (where a keyword is expected). The result is that `limit`, `match`, and `follow` appear alongside file-name completions even when typing the file path.
This is consistent with the existing `check` pattern at line 79, so it may be intentional. If you want position-aware completion you would need to inspect `$cword` relative to `$entity_idx`, but that is a larger refactor. As-is, it is a minor UX roughness rather than a functional bug.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: add sh log entity for viewing/sear..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "$2" in | ||
| limit) | ||
| local n="${3:-20}" | ||
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then |
There was a problem hiding this comment.
limit 0 bypasses the "positive integer" contract
The regex ^[0-9]+$ matches 0, so u7 sh log <file> limit 0 silently runs tail -n 0 and returns nothing with exit 0. The error message on the following line explicitly says "positive integer", but 0 is accepted. Users seeing silent empty output for limit 0 are likely to be confused.
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | |
| if ! [[ "$n" =~ ^[1-9][0-9]*$ ]]; then |
This changes the regex to require at least one digit 1-9 optionally followed by more digits, which correctly excludes 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/show.sh
Line: 264
Comment:
**`limit 0` bypasses the "positive integer" contract**
The regex `^[0-9]+$` matches `0`, so `u7 sh log <file> limit 0` silently runs `tail -n 0` and returns nothing with exit 0. The error message on the following line explicitly says "positive integer", but `0` is accepted. Users seeing silent empty output for `limit 0` are likely to be confused.
```suggestion
if ! [[ "$n" =~ ^[1-9][0-9]*$ ]]; then
```
This changes the regex to require at least one digit 1-9 optionally followed by more digits, which correctly excludes `0`.
How can I resolve this? If you propose a fix, please make it concise.| match) | ||
| if [[ -z "$3" ]]; then | ||
| echo "Usage: u7 sh log <file> match <pattern>" | ||
| return 1 | ||
| fi | ||
| grep -- "$3" "$file" | ||
| ;; |
There was a problem hiding this comment.
grep exit code propagates as function failure on no-match
When grep finds no matching lines it exits with code 1, which becomes the return value of _u7_show. A caller checking $? (or a script using set -e) will see this as a command failure even though "no results" is not an error. Consider suppressing or distinguishing the no-match exit code:
| match) | |
| if [[ -z "$3" ]]; then | |
| echo "Usage: u7 sh log <file> match <pattern>" | |
| return 1 | |
| fi | |
| grep -- "$3" "$file" | |
| ;; | |
| match) | |
| if [[ -z "$3" ]]; then | |
| echo "Usage: u7 sh log <file> match <pattern>" | |
| return 1 | |
| fi | |
| grep -- "$3" "$file" || true | |
| ;; |
Appending || true keeps the function exit code at 0 when there are simply no matches, while still showing any matched lines.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/show.sh
Line: 270-276
Comment:
**`grep` exit code propagates as function failure on no-match**
When `grep` finds no matching lines it exits with code `1`, which becomes the return value of `_u7_show`. A caller checking `$?` (or a script using `set -e`) will see this as a command failure even though "no results" is not an error. Consider suppressing or distinguishing the no-match exit code:
```suggestion
match)
if [[ -z "$3" ]]; then
echo "Usage: u7 sh log <file> match <pattern>"
return 1
fi
grep -- "$3" "$file" || true
;;
```
Appending `|| true` keeps the function exit code at `0` when there are simply no matches, while still showing any matched lines.
How can I resolve this? If you propose a fix, please make it concise.| env) COMPREPLY=($(compgen -W "match" -- "$cur")) ;; | ||
| http) COMPREPLY=($(compgen -W "get head headers" -- "$cur")) ;; | ||
| docker) COMPREPLY=($(compgen -W "containers images volumes networks all" -- "$cur")) ;; | ||
| log) COMPREPLY=($(compgen -W "limit match follow" -- "$cur")) ; _filedir ;; |
There was a problem hiding this comment.
_u7_complete_args is called for any argument beyond the entity, so this line runs for both position 3 (where the user should type a <file>) and position 4+ (where a keyword is expected). The result is that limit, match, and follow appear alongside file-name completions even when typing the file path.
This is consistent with the existing check pattern at line 79, so it may be intentional. If you want position-aware completion you would need to inspect $cword relative to $entity_idx, but that is a larger refactor. As-is, it is a minor UX roughness rather than a functional bug.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/completions.sh
Line: 41
Comment:
**Completion offers keywords at every argument position**
`_u7_complete_args` is called for any argument beyond the entity, so this line runs for both position 3 (where the user should type a `<file>`) and position 4+ (where a keyword is expected). The result is that `limit`, `match`, and `follow` appear alongside file-name completions even when typing the file path.
This is consistent with the existing `check` pattern at line 79, so it may be intentional. If you want position-aware completion you would need to inspect `$cword` relative to `$entity_idx`, but that is a larger refactor. As-is, it is a minor UX roughness rather than a functional bug.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Code Review
This pull request introduces a new log subcommand to the show entity, enabling users to view, follow, or search log files with optional line limits. The changes include shell completion updates in lib/completions.sh, the core implementation in lib/show.sh, and comprehensive test coverage in test.sh. Review feedback suggests refactoring the log logic to improve maintainability by centralizing the usage string and consolidating the limit and default cases to reduce duplication.
| log) | ||
| local file="$1" | ||
| if [[ -z "$file" ]]; then | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| fi | ||
| if [[ ! -f "$file" ]]; then | ||
| echo "File not found: $file" | ||
| return 1 | ||
| fi | ||
| case "$2" in | ||
| limit) | ||
| local n="${3:-20}" | ||
| if ! [[ "$n" =~ ^[0-9]+$ ]]; then | ||
| echo "Error: limit must be a positive integer, got '$n'" | ||
| return 1 | ||
| fi | ||
| tail -n "$n" "$file" | ||
| ;; | ||
| match) | ||
| if [[ -z "$3" ]]; then | ||
| echo "Usage: u7 sh log <file> match <pattern>" | ||
| return 1 | ||
| fi | ||
| grep -- "$3" "$file" | ||
| ;; | ||
| follow) | ||
| tail -f "$file" | ||
| ;; | ||
| "") | ||
| tail -n 20 "$file" | ||
| ;; | ||
| *) | ||
| echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]" | ||
| return 1 | ||
| ;; | ||
| esac | ||
| ;; |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can define the usage string in a variable and reuse it. Also, the limit and default ("") cases can be combined since they have similar logic.
log)
local file="$1"
local usage="Usage: u7 sh log <file> [limit N|match <pattern>|follow]"
if [[ -z "$file" ]]; then
echo "$usage"
return 1
fi
if [[ ! -f "$file" ]]; then
echo "File not found: $file"
return 1
fi
case "$2" in
limit|"")
local n="${3:-20}"
if ! [[ "$n" =~ ^[0-9]+$ ]]; then
echo "Error: limit must be a positive integer, got '$n'"
return 1
fi
tail -n "$n" "$file"
;;
match)
if [[ -z "$3" ]]; then
echo "Usage: u7 sh log <file> match <pattern>"
return 1
fi
grep -- "$3" "$file"
;;
follow)
tail -f "$file"
;;
*)
echo "$usage"
return 1
;;
esac
;;
1 participant
Summary
logentity tou7 shfor viewing and searching log filesu7 sh log <file>(tail -20),limit N,match <pattern>, andfollow(tail -f)Test plan
bash test.shpasses all 147 tests (0 failures)u7 sh log /var/log/syslogu7 sh log /var/log/syslog limit 5u7 sh log /var/log/syslog match erroru7 sh log /var/log/syslog follow