gh-17 adding list and summary and prune function #32
Conversation
WalkthroughThe Changes
Sequence Diagram(s)git-artifact: Prune Command FlowsequenceDiagram
participant User
participant git-artifact
participant GitRepo
User->>git-artifact: Run 'git-artifact prune --glob PATTERN --keep N'
git-artifact->>GitRepo: List tags matching glob PATTERN
GitRepo-->>git-artifact: Return tag list
git-artifact->>git-artifact: Determine tags to delete (all but N most recent)
alt dry-run
git-artifact->>User: Print tags that would be deleted
else actual prune
git-artifact->>GitRepo: Delete old tags from remote
git-artifact->>User: Print summary of deleted tags
end
_tests.sh: Dynamic Test ExecutionsequenceDiagram
participant User
participant _tests.sh
participant TestCaseFn
User->>_tests.sh: Run script (with or without --testcase)
_tests.sh->>_tests.sh: Parse options, discover test functions
alt --testcase specified
_tests.sh->>TestCaseFn: Invoke specified test function
else
loop For each test function
_tests.sh->>TestCaseFn: Invoke test function
end
end
TestCaseFn->>_tests.sh: Run test, output results, call eval_testcase
_tests.sh->>User: Print test results and logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
🔇 Additional comments (14)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
_tests.sh (1)
415-415: Use mapfile to safely capture function names.The current approach could split function names incorrectly if they contain spaces (though unlikely).
Apply this safer approach:
- test_functions=($(declare -F | awk '{print $3}' | grep -E '^[0-9]+(\.[0-9]+)?$')) + mapfile -t test_functions < <(declare -F | awk '{print $3}' | grep -E '^[0-9]+(\.[0-9]+)?$')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.test/7/git-reference.logis excluded by!**/*.log.test/8/git-reference.logis excluded by!**/*.log.test/9/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (2)
_tests.sh(4 hunks)git-artifact(9 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
_tests.sh
[warning] 202-202: Quote this to prevent word splitting.
(SC2046)
[warning] 218-218: Quote this to prevent word splitting.
(SC2046)
[warning] 232-232: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
[warning] 287-287: Quote this to prevent word splitting.
(SC2046)
[warning] 304-304: Quote this to prevent word splitting.
(SC2046)
[warning] 323-323: Quote this to prevent word splitting.
(SC2046)
[warning] 350-350: Quote this to prevent word splitting.
(SC2046)
[warning] 386-386: Quote this to prevent word splitting.
(SC2046)
[warning] 415-415: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🔇 Additional comments (7)
git-artifact (4)
79-81: LGTM! Clear command descriptions added.The new commands
list,summary, andpruneare well-documented in the help text.
373-378: LGTM! Clean implementation of the list command.The function correctly lists tags matching the glob pattern and provides a count.
476-519: LGTM! Well-structured option parsing.The new options are properly handled with appropriate validation, especially the numeric check for
--keep.
587-612: LGTM! Consistent command dispatch implementation.All new commands properly validate their required options and source necessary dependencies.
_tests.sh (3)
4-31: Excellent documentation and usage function!The comprehensive usage instructions make it easy for contributors to understand how to add and run tests.
167-411: Excellent test refactoring with modular design!The conversion to individual test functions with dynamic discovery greatly improves maintainability. The consistent use of
testcase_headerandeval_testcaseensures uniformity across all tests.
377-411: Well-designed test for the prune functionality!Test 9 effectively validates both dry-run and actual prune operations, ensuring the correct number of tags are retained.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
git-artifact (2)
341-371: This issue was already identified in previous reviews.The word splitting issue on line 369 with
$tags_to_be_prunedand the lack of empty value checking has already been flagged in previous reviews.
380-407: This delimiter inconsistency issue was already identified.The inconsistent delimiter usage between key extraction (hardcoded "/") and info extraction (
$delimitervariable) on lines 403-404 has already been flagged in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
git-artifact(9 hunks)
🔇 Additional comments (9)
git-artifact (9)
79-93: LGTM! Clear documentation for new commands.The help text additions appropriately document the new
list,summary, andprunecommands along with their associated options. The formatting is consistent with the existing help structure.
477-477: LGTM! Consistent flag assignment.The change to set
arg_quiet=1is consistent with other boolean flag handling in the script.
482-487: LGTM! Proper option parsing for new flags.The parsing for
--dryrunand--verboseoptions is implemented correctly with appropriate variable assignments.
504-519: LGTM! Good input validation for numeric parameter.The option parsing for
--globand--keepis well implemented. The numeric validation for--keepusing regex is a good security practice to prevent injection attacks.
373-378: LGTM! Well-structured list command implementation.The
cmd_listfunction properly implements tag listing with appropriate quiet mode handling and summary output. The use of glob patterns is consistent with the updated approach.
412-412: LGTM! Consistent migration to glob patterns.The change from regex to glob pattern usage in the
find-latestfunction is consistent with the overall refactoring to use glob patterns throughout the tool.
421-421: LGTM! Updated error message reflects new functionality.The error message correctly refers to "glob pattern" instead of the previous regex terminology, maintaining consistency with the updated implementation.
456-456: LGTM! Helpful debug output addition.Adding debug output for the debug variable itself is useful for troubleshooting the debug functionality.
627-627: LGTM! Proper default value handling.The debug output for
arg_quietwith default value0is well implemented and consistent with bash best practices for handling potentially unset variables.
bicschneider
force-pushed
the
bicschneider/issue17-list-n-summary-functions
branch
from
7f109b2 to
1f66951
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
git-artifact (2)
341-371: Critical issue: Unquoted variable can cause command injection.Line 369 uses an unquoted variable
$tags_to_be_prunedwhich can cause word splitting and command failures. This is the same issue identified in the past review comments.Additionally, there are logic issues:
- Line 369 will fail if
tags_to_be_prunedis empty- The function doesn't handle the case when no tags match the glob pattern properly
Apply this fix:
- echo $tags_to_be_pruned | xargs git push origin --delete + if [[ -n "$tags_to_be_pruned" ]]; then + echo "$tags_to_be_pruned" | xargs git push origin --delete + else + echo "No tags to prune" + fi
395-407: Critical issue: Delimiter usage inconsistency.Line 403 uses hardcoded "/" delimiter while line 404 uses the
$delimitervariable, causing inconsistent tag grouping when delimiter is not "/".Apply this fix:
- key="${key%%/*}" + key="${key%%"$delimiter"*}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.test/7/git-reference.logis excluded by!**/*.log.test/8/git-reference.logis excluded by!**/*.log.test/9/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (2)
_tests.sh(4 hunks)git-artifact(9 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
_tests.sh
[warning] 202-202: Quote this to prevent word splitting.
(SC2046)
[warning] 218-218: Quote this to prevent word splitting.
(SC2046)
[warning] 232-232: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
[warning] 287-287: Quote this to prevent word splitting.
(SC2046)
[warning] 304-304: Quote this to prevent word splitting.
(SC2046)
[warning] 323-323: Quote this to prevent word splitting.
(SC2046)
[warning] 350-350: Quote this to prevent word splitting.
(SC2046)
[warning] 386-386: Quote this to prevent word splitting.
(SC2046)
[warning] 415-415: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🔇 Additional comments (22)
git-artifact (15)
79-81: LGTM! Clear help text updates.The help text accurately describes the new commands (list, summary, prune) that have been added to the tool.
86-93: LGTM! Option parsing enhancements.The new options are properly documented in the help text with clear descriptions of their purposes and usage contexts.
104-112: LGTM! Proper option organization.The options are well-organized by command context, making it clear which options apply to which commands.
373-378: LGTM! Clean implementation of list command.The list command properly uses the glob pattern and provides useful output with tag counts.
380-393: LGTM! Good summary command implementation.The summary command provides a useful aggregation of tags by prefix using the delimiter approach.
412-412: LGTM! Proper glob pattern usage.The find-latest function correctly uses the glob pattern instead of regex for tag filtering.
421-421: LGTM! Consistent error messaging.The error message correctly refers to "glob pattern" instead of regex, maintaining consistency with the updated functionality.
456-456: LGTM! Consistent debug variable usage.The debug logging uses the consistent variable name format.
477-477: LGTM! Proper quiet flag implementation.The quiet flag is properly set using consistent variable naming.
482-487: LGTM! New option flags implemented correctly.The new dryrun and verbose flags are properly parsed and stored in appropriate variables.
504-507: LGTM! Glob option replaces regex.The option parsing correctly implements the glob pattern flag, replacing the previous regex approach.
512-519: LGTM! Proper validation of keep option.The keep option includes proper validation to ensure it's a numeric value, with appropriate error handling.
587-597: Fix inconsistent variable usage.The code correctly uses
arg_globvariable but the error message text still references the old option name.Apply this fix:
- echo "ERROR: -g|--glob is required for $arg_command" + echo "ERROR: -g|--glob is required for $arg_command"Actually, looking more closely, this appears to be correct. The message is consistent with the option name.
598-612: LGTM! Proper command validation for new commands.The new commands (summary and prune) are properly integrated into the command validation logic with appropriate option requirements.
629-629: LGTM! Consistent variable naming in debug output.The debug output uses the consistent
arg_quietvariable name._tests.sh (7)
4-31: LGTM! Comprehensive usage documentation.The usage function provides excellent documentation for the test script, including clear instructions for adding new tests and understanding the test structure.
33-61: LGTM! Robust command-line argument parsing.The argument parsing is well-implemented with proper error handling for unknown options and clear variable naming.
69-75: LGTM! Proper debug mode initialization.The debug mode setup correctly exports variables and enables shell debugging when requested.
88-100: LGTM! Enhanced testcase_header function.The function now automatically creates missing test directories and reference files, which improves the test setup process.
102-106: LGTM! Useful helper function for test log generation.The
generate_git_test_logfunction encapsulates the git log generation logic, making it reusable across tests.
115-117: LGTM! Improved test log handling.The logic properly checks for existing test logs and uses them when available, providing good flexibility for custom test outputs.
167-411: LGTM! Excellent modularization of test cases.The conversion of inline test blocks to individual functions is a significant improvement that enables:
- Selective test execution via the
--testcaseoption- Better maintainability and readability
- Consistent error handling and logging
The test functions cover the new git-artifact commands (list, summary, prune) comprehensively.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
git-artifact (2)
369-369: Fix potential word splitting and handle empty tag list.The variable
$tags_to_be_prunedshould be quoted to prevent word splitting. Additionally, the command will fail if no tags need to be pruned.
395-407: Fix delimiter usage inconsistency in tag grouping.The key extraction on line 403 always uses "/" as delimiter (
${key%%/*}), but the info extraction on line 404 uses the$delimitervariable. This will cause incorrect grouping when delimiter is not "/".
🧹 Nitpick comments (2)
git-artifact (2)
373-378: Eliminate duplicate git ls-remote calls for efficiency.The same
git ls-remotecommand is executed twice, which is inefficient and could yield inconsistent results if tags change between calls.Apply this refactor to call the command once:
cmd_list() { + local tags_output + tags_output=$(git ls-remote --tags --ref --sort=v:refname origin "refs/tags/${arg_glob:-*}") if [[ ${arg_quiet:-} -ne 1 ]]; then - git ls-remote --tags --ref --sort=v:refname origin "refs/tags/${arg_glob:-*}" | cut -d / -f 3- + echo "$tags_output" | cut -d / -f 3- fi - printf "Tags found: %s : %s\n" "${arg_glob}" "$(git ls-remote --tags --ref --sort=v:refname origin "refs/tags/${arg_glob:-*}" | wc -l)" + printf "Tags found: %s : %s\n" "${arg_glob}" "$(echo "$tags_output" | wc -l)" }
587-610: Fix error message text for glob option.The error message on line 592 incorrectly references "arg_glob" instead of "glob".
Apply this fix:
- echo "INFO: -g|--arg_glob is not set: Defaulting to '*' (all tags)." + echo "INFO: -g|--glob is not set: Defaulting to '*' (all tags)."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
git-artifact(9 hunks)
🔇 Additional comments (5)
git-artifact (5)
79-81: LGTM! Help text properly documents new functionality.The additions to the help text correctly describe the new commands and options, maintaining consistency with the existing format.
Also applies to: 87-93, 104-106, 110-112
380-393: LGTM! Summary function implementation is solid.The function correctly uses associative arrays to group tags by prefix and provides a clear summary output.
412-412: LGTM! Glob pattern integration is consistent.The function correctly uses
arg_globand updates error messages to reflect the change from regex to glob patterns.Also applies to: 421-421
477-477: LGTM! Option parsing enhancements are well implemented.The new options are correctly parsed with appropriate validation (numeric check for --keep) and consistent variable naming.
Also applies to: 482-487, 504-506, 512-519
456-456: LGTM! Debug statements properly updated.The debug statements are consistent with the updated variable naming scheme throughout the script.
Also applies to: 627-627
bicschneider
force-pushed
the
bicschneider/issue17-list-n-summary-functions
branch
from
43bfc67 to
a26554b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.test/7/git-reference.logis excluded by!**/*.log.test/8/git-reference.logis excluded by!**/*.log.test/9/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (2)
_tests.sh(4 hunks)git-artifact(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- git-artifact
🧰 Additional context used
🪛 Shellcheck (0.10.0)
_tests.sh
[warning] 202-202: Quote this to prevent word splitting.
(SC2046)
[warning] 218-218: Quote this to prevent word splitting.
(SC2046)
[warning] 232-232: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
[warning] 287-287: Quote this to prevent word splitting.
(SC2046)
[warning] 304-304: Quote this to prevent word splitting.
(SC2046)
[warning] 323-323: Quote this to prevent word splitting.
(SC2046)
[warning] 350-350: Quote this to prevent word splitting.
(SC2046)
[warning] 386-386: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
_tests.sh (1)
415-417: Good adoption ofmapfile– array fills are now safeSwitching to
mapfile -t test_functions …eliminates SC2207 word-splitting risks from the earlier implementation. Nice upgrade.
bicschneider
force-pushed
the
bicschneider/issue17-list-n-summary-functions
branch
3 times, most recently
from
66ffb5c to
1ec7e35
Compare
Breaking: - option regex replaced with glob as was implemented as glob. regex might come later using grep as filter New features: - added: function "list" artifacts command with glob pattern - added: function "summary" using delimiter (default: /) . it finds all first fields and then counts number of artifacts given the pattern - added: function "prune". given a pattern find all artifacts and then keep the last amount given (default: 20). It is supporting --dryrun option Tests: - refactor tests to functions and enable single test case execution - it is still defaulting to * if not given - tests for new functions
bicschneider
force-pushed
the
bicschneider/issue17-list-n-summary-functions
branch
from
1ec7e35 to
3bcc012
Compare
additionally: refactor tests to functions and enable single test case execution
Summary by CodeRabbit