Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/completions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ _u7_complete_entities() {
local verb="$1" cur="$2"
case "$verb" in
show|sh)
COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes port usage network git env http docker system definition functions --help" -- "$cur"))
COMPREPLY=($(compgen -W "ip csv json line ssl files diff cpu memory disk processes port usage network git env log http docker system definition functions --help" -- "$cur"))
;;
make|mk)
COMPREPLY=($(compgen -W "dir file password user copy link archive clone template sequence --help" -- "$cur"))
Expand Down Expand Up @@ -38,6 +38,7 @@ _u7_complete_args() {
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 ;;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

*) _filedir ;;
esac
;;
Expand Down
40 changes: 40 additions & 0 deletions lib/show.sh
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,45 @@ _u7_show() {
esac
;;

log)
local file="$1"
if [[ -z "$file" ]]; then
echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]"
return 1
fi
Comment on lines +252 to +256

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

Suggested change
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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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.

echo "Error: limit must be a positive integer, got '$n'"
return 1
fi
Comment on lines +263 to +267

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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

tail -n "$n" "$file"
;;
match)
if [[ -z "$3" ]]; then
echo "Usage: u7 sh log <file> match <pattern>"
return 1
fi
grep -- "$3" "$file"
;;
Comment on lines +270 to +276

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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:

Suggested change
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.

follow)
tail -f "$file"
;;
"")
tail -n 20 "$file"
;;
*)
echo "Usage: u7 sh log <file> [limit N|match <pattern>|follow]"
return 1
;;
esac
;;
Comment on lines +251 to +288

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
      ;;


http)
local method="$1"
local url="$2"
Expand Down Expand Up @@ -302,6 +341,7 @@ Entities:
network
git <authors|branches|tags|log [N]|status|diff|remotes>
env [match <pattern>]
log <file> [limit N|match <pattern>|follow]
http <get|head|headers> <url>
docker <containers|images|volumes|networks|all>
definition of <word>
Expand Down
48 changes: 48 additions & 0 deletions test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -988,6 +988,54 @@ else
((PASSED++))
fi

# ===== sh log tests =====

# Setup: create a temp log file
LOG_FILE="$TEST_DIR/test.log"
for i in $(seq 1 30); do
echo "line $i: log entry" >> "$LOG_FILE"
done

# Test: sh log default shows last 20 lines
result=$(u7 sh log "$LOG_FILE" 2>&1)
line_count=$(echo "$result" | wc -l | tr -d ' ')
assert_equals "sh log default shows 20 lines" "20" "$line_count"
assert_contains "sh log default starts at line 11" "line 11" "$result"

# Test: sh log with limit
result=$(u7 sh log "$LOG_FILE" limit 5 2>&1)
line_count=$(echo "$result" | wc -l | tr -d ' ')
assert_equals "sh log limit 5 shows 5 lines" "5" "$line_count"
assert_contains "sh log limit 5 starts at line 26" "line 26" "$result"

# Test: sh log with match
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)
Comment on lines +1012 to +1015

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

line_count=$(echo "$result" | wc -l | tr -d ' ')
assert_equals "sh log match finds 2 ERROR lines" "2" "$line_count"

# Test: sh log missing file
result=$(u7 sh log nonexistent.log 2>&1)
assert_contains "sh log reports missing file" "File not found" "$result"

# Test: sh log no arguments
result=$(u7 sh log 2>&1)
assert_contains "sh log no args shows usage" "Usage" "$result"

# Test: sh log invalid limit
result=$(u7 sh log "$LOG_FILE" limit abc 2>&1)
assert_contains "sh log rejects non-integer limit" "limit must be a positive integer" "$result"

# Test: sh log match without pattern
result=$(u7 sh log "$LOG_FILE" match 2>&1)
assert_contains "sh log match without pattern shows usage" "Usage" "$result"

# Test: sh log unknown subcommand
result=$(u7 sh log "$LOG_FILE" badarg 2>&1)
assert_contains "sh log unknown subcommand shows usage" "Usage" "$result"

# Cleanup
cd /
rm -rf "$TEST_DIR"
Expand Down
Loading