gh-18 : fix git version 2.49.0 test problem #19
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce monthly scheduled workflow runs, add optional debug and verbose workflow inputs, and expand Alpine Git test coverage to include versions v2.49.0 and v2.47.2. Dockerfile and workflow scripts now create and run as a non-root user matching the host's UID/GID. Test scripts improve logging, error handling, and reporting, while Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Workflow Inputs
participant Docker Build
participant Docker Run
participant Test Script
GitHub Actions->>Workflow Inputs: Accept debug/verbose (manual dispatch)
GitHub Actions->>Docker Build: Build image with USER_ID/GROUP_ID
Docker Build->>Docker Run: Image runs as gituser (non-root)
Docker Run->>Test Script: Pass debug/verbose env vars
Test Script->>Test Script: Run all test cases, aggregate results
Test Script->>Docker Run: Output summary, exit with global status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 selected for processing (1)
✨ 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 (2)
.github/workflows/pr.yml (2)
25-25: Fix YAML formatting issues flagged by static analysis.Several formatting issues need to be addressed:
- default: true - + default: true +And remove trailing spaces from lines 58-61, 68, 75-76, 80, 87, 94-95.
Also applies to: 58-61, 68-68, 75-76, 80-80, 87-87, 94-95
65-66: Fix indentation in new test jobs.The indentation for the new test jobs is inconsistent with the existing pattern.
- steps: - - name: Checkout code + steps: + - name: Checkout codeAlso applies to: 84-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pr.yml(4 hunks)Dockerfile(1 hunks)README.md(2 hunks)_tests.sh(4 hunks)git-artifact(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr.yml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[warning] 63-63: wrong indentation: expected 6 but found 8
(indentation)
[warning] 65-65: wrong indentation: expected 10 but found 8
(indentation)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 80-80: trailing spaces
(trailing-spaces)
[warning] 82-82: wrong indentation: expected 6 but found 8
(indentation)
[warning] 84-84: wrong indentation: expected 10 but found 8
(indentation)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 94-94: trailing spaces
(trailing-spaces)
[error] 95-95: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/pr.yml
66-66: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
68-68: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2038:warning:8:5: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames
(shellcheck)
85-85: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
87-87: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
87-87: shellcheck reported issue in this script: SC2038:warning:8:5: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames
(shellcheck)
🔇 Additional comments (19)
Dockerfile (1)
8-14: Excellent security improvement by adding non-root user support.The addition of
USER_IDandGROUP_IDbuild arguments with proper user creation is a good security practice. The implementation correctly creates a non-root user and switches context before git configuration.git-artifact (6)
1-1: Good portability improvement with env shebang.Using
#!/usr/bin/env bashinstead of a fixed path improves portability across different systems.
6-27: Excellent environment validation with comprehensive error reporting.The
check_environment()function provides thorough validation of the Bash environment with clear error messages. The version checking logic correctly handles both major and minor version requirements.
29-49: Robust git environment validation.The
check_git_environment()function properly validates the Git installation and provides helpful diagnostic information. The logic correctly checksGIT_EXEC_PATHand path configurations.
51-58: Useful debug information function.The
show_info()function provides valuable environment details for troubleshooting. All the information gathered would be helpful for debugging issues.
60-104: Good modularization of options specification.Moving the options specification into a dedicated function improves code organization and makes it easier to maintain the help text and option definitions.
527-534: Excellent conditional execution pattern for sourceable scripts.The conditional block using
"${BASH_SOURCE[0]}" == "${0}"is the correct way to make a script both executable and sourceable. The sequence of environment checks, debug info, and initialization is well-structured.README.md (2)
43-43: Improved clarity in workflow description.The change from "The history is basically like this:" to "The history of git-artifact workflow can basically look like this:" provides better context and clarity.
63-76: Enhanced Mermaid diagram with consistent tag naming.The updates to include consistent "/bin" suffixes on tags and the addition of "2.0/src" and "2.0/test" commits better demonstrate the horizontal commit structure and artifact appending capabilities of git-artifact.
_tests.sh (5)
1-1: Good portability improvement with env shebang.Consistent with the git-artifact script, using
#!/usr/bin/env bashimproves portability.
9-12: Excellent integration with git-artifact environment validation.Sourcing the git-artifact script and calling
check_environment()ensures consistent environment validation across the test suite. The formatted git version output is also more user-friendly.
21-21: Good addition of global exit code tracking.The
global_exit_codevariable enables test result accumulation, allowing all tests to run even if some fail.
48-52: Improved test failure handling with result accumulation.The modified failure handling correctly accumulates test failures in
global_exit_codewhile providing clear "NOK" output and optional verbose logging. This is much better than the previous fail-fast approach.
203-205: Excellent test summary and exit code handling.The final summary message and exit with the accumulated exit code provides clear feedback about overall test results. This completes the improved test result accumulation pattern.
.github/workflows/pr.yml (5)
9-10: Good addition of scheduled testing.The monthly scheduled run will help catch issues proactively. The cron expression
'0 0 1 * *'correctly runs at midnight on the first day of each month.
14-25: Excellent addition of debug and verbose input parameters.The workflow dispatch inputs provide valuable flexibility for troubleshooting and controlling output verbosity during manual runs.
54-61: Good integration with Dockerfile user/group handling.The Docker build and run commands correctly pass
USER_IDandGROUP_IDbuild arguments and use the current user's UID/GID for execution. The environment variables from inputs are properly passed to the container.
62-78: Excellent addition of git v2.49.0 test job.This directly addresses the PR objective of fixing git version 2.49.0 test problems. The job structure is consistent with existing test jobs.
81-97: Good addition of git v2.47.2 test coverage.Adding more git version test coverage improves compatibility testing across different git versions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
_tests.sh (1)
6-9: Broaden debug flag recognition
The expression only activates whendebug=true. Accepting common truthy values (e.g.,1,yes) improves DX:-[[ ${debug:-} == true ]] && { +[[ ${debug:-false} =~ ^(true|1|yes)$ ]] && {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
_tests.sh(4 hunks)git-artifact(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- git-artifact
🔇 Additional comments (1)
_tests.sh (1)
14-15: Good move sourcinggit-artifactand validating the environment early
Using$(dirname "$0")keeps the path robust, and an upfrontcheck_environmentguards against subtle runtime issues.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/pr.yml (1)
32-33: Update toactions/checkout@v4.
Both actionlint and a previous review already highlighted that@v2runs on an outdated runner image and is now deprecated.Also applies to: 68-69, 89-90, 109-110, 128-129
_tests.sh (1)
5-5:set -estill prevents the aggregated-exit-code logic.Previous feedback remains unaddressed: any unexpected non-zero status before the very last lines terminates the script and bypasses the
global_exit_codesummary. Consider disabling-eafter the setup phase or wrapping test execution withset +e … set -e.Also applies to: 210-212
🧹 Nitpick comments (1)
.github/workflows/pr.yml (1)
14-25: Trim trailing whitespace & fix minor YAML-lint errors.Lines 25, 70, 84, 91 carry stray spaces and several child keys are mis-indented by two spaces (warnings on 65, 67, 86, 88). They do not break the workflow but fail
yamllint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr.yml(4 hunks)_tests.sh(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr.yml
[error] 25-25: trailing spaces
(trailing-spaces)
[warning] 65-65: wrong indentation: expected 6 but found 8
(indentation)
[warning] 67-67: wrong indentation: expected 10 but found 8
(indentation)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 84-84: trailing spaces
(trailing-spaces)
[warning] 86-86: wrong indentation: expected 6 but found 8
(indentation)
[warning] 88-88: wrong indentation: expected 10 but found 8
(indentation)
[error] 91-91: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/pr.yml
68-68: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
70-70: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
70-70: shellcheck reported issue in this script: SC2044:warning:7:16: For loops over find output are fragile. Use find -exec or a while read loop
(shellcheck)
70-70: shellcheck reported issue in this script: SC2086:info:9:13: Double quote to prevent globbing and word splitting
(shellcheck)
89-89: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
91-91: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2044:warning:7:16: For loops over find output are fragile. Use find -exec or a while read loop
(shellcheck)
91-91: shellcheck reported issue in this script: SC2086:info:9:13: Double quote to prevent globbing and word splitting
(shellcheck)
bicschneider
force-pushed
the
bicschneider/issue18-git2.49.0-test-problem
branch
from
57f21cc to
6eb6991
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/pr.yml (2)
32-33: Still using deprecatedactions/checkout@v2– upgrade now
v2is no longer maintained and will eventually stop receiving security patches. The migration effort is a one-liner.- uses: actions/checkout@v2 + uses: actions/checkout@v4Also applies to: 45-46, 58-59, 72-73, 85-86, 98-99
50-53: Unquoted shell variables – repeat of SC2046 / SC2086 findings
All the build/run commands still interpolate unquoted variables and subshells ($(id -u),$(pwd),${{ github.event.inputs.* }}), leaving the workflow vulnerable to word-splitting and pathname expansion.The exact quoting patch suggested earlier still applies verbatim.
Also applies to: 63-65, 77-79, 90-92, 103-105
_tests.sh (1)
5-5:set -estill short-circuits aggregated-exit-code logic
A single non-zero inside any test brace aborts the script beforeglobal_exit_codeis updated, defeating the new summary.
Useset +ejust before running testcases (or wrap the block) and re-enable afterwards.-set -euo pipefail +set -uo pipefail # keep -u/-o, drop -e to allow per-test failures
🧹 Nitpick comments (3)
.github/workflows/pr.yml (2)
24-25: YAML lint failures: trailing spaces & mis-indentation
The current file does not pass yaml-lint:• trailing whitespace (lines 25, 40, 60, 67, 74)
• job-block indentation off by two spaces (e.g. line 55 expects 6, found 8)While GitHub’s parser is tolerant, failing static checks makes CI red in strict environments – strip the spaces and normalise indentation.
Also applies to: 40-40, 55-57, 69-71, 74-74
41-106: Collapse the N-almost-identical jobs into a matrix
Six copy-pasted jobs differ only by thegit_versionvalue. A single job with a matrix strategy will cut ~80 lines and future-proof version additions.Example snippet:
strategy: matrix: git_version: [latest, v2.49.0, v2.47.2, v2.36.2, v2.26.2] steps: - uses: actions/checkout@v4 - run: | set -eux gv=${{ matrix.git_version }} docker build --build-arg ALPINE_GIT_DOCKER_VERSION="$gv" \ --build-arg USER_ID="$(id -u)" \ --build-arg GROUP_ID="$(id -g)" \ -t "git-artifact:${gv}" . docker run --rm -u "$(id -u):$(id -g)" \ -e debug="${{ inputs.debug }}" \ -e verbose="${{ inputs.verbose }}" \ -v "$(pwd):/git" -v "$HOME/.ssh:/root/.ssh" \ "git-artifact:${gv}" artifact-tests_tests.sh (1)
217-234:for log in $(find …)breaks on filenames with spaces
The word-splitting you guarded against elsewhere is re-introduced here. A safe, zero-delimited loop avoids this pitfall.-for log in $( find . -name run.log -o -name nok.log | sort ); do +find . \( -name run.log -o -name nok.log \) -print0 | sort -z | while IFS= read -r -d '' log; do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr.yml(5 hunks)_tests.sh(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr.yml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[warning] 55-55: wrong indentation: expected 6 but found 8
(indentation)
[warning] 57-57: wrong indentation: expected 10 but found 8
(indentation)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
[warning] 69-69: wrong indentation: expected 6 but found 8
(indentation)
[warning] 71-71: wrong indentation: expected 10 but found 8
(indentation)
[error] 74-74: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/pr.yml
58-58: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-60: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
72-72: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
74-74: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
| mv run.log ok.log | ||
| else | ||
| echo "ERROR: Test $test failed: ${testcase_synopsis}" | ||
| exit 1 | ||
| echo "Test $test : NOK : ${testcase_synopsis}" | ||
| mv run.log nok.log | ||
| [[ ${verbose:-} == true ]] && cat git-test.log | ||
| global_exit_code=2 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential false-negative when diff errors
diff exit > 1 (I/O error, etc.) is treated the same as “files differ” and the script continues. Capture the status to distinguish unexpected errors:
- if diff -w git-test.log git-reference.log ; then
+ diff -w git-test.log git-reference.log
+ rc=$?
+ if [[ $rc -eq 0 ]]; then
…
+ elif [[ $rc -eq 1 ]]; then # files differ – expected branch
+ …
+ else # diff crashed
+ echo "Test $test : ERROR (diff failed, rc=$rc)"
+ global_exit_code=2
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mv run.log ok.log | |
| else | |
| echo "ERROR: Test $test failed: ${testcase_synopsis}" | |
| exit 1 | |
| echo "Test $test : NOK : ${testcase_synopsis}" | |
| mv run.log nok.log | |
| [[ ${verbose:-} == true ]] && cat git-test.log | |
| global_exit_code=2 | |
| fi | |
| diff -w git-test.log git-reference.log | |
| rc=$? | |
| if [[ $rc -eq 0 ]]; then | |
| echo "Test $test : OK : ${testcase_synopsis}" | |
| mv run.log ok.log | |
| elif [[ $rc -eq 1 ]]; then # files differ – expected branch | |
| echo "Test $test : NOK : ${testcase_synopsis}" | |
| mv run.log nok.log | |
| [[ ${verbose:-} == true ]] && cat git-test.log | |
| global_exit_code=2 | |
| else # diff crashed | |
| echo "Test $test : ERROR (diff failed, rc=$rc)" | |
| global_exit_code=2 | |
| fi |
🤖 Prompt for AI Agents
In _tests.sh around lines 50 to 56, the script treats any diff exit code greater
than 1 as files differing, which can mask I/O or other unexpected errors. Modify
the script to capture the diff exit status separately, check if it is exactly 1
(files differ) or greater than 1 (error), and handle the error case distinctly
by reporting it and possibly exiting or setting a different error code to avoid
false negatives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
_tests.sh (2)
5-5:set -estill breaks aggregated test reporting
set -eaborts the script on the first non-zero exit outside anif/whiletest, so many commands inside the{ … > run.log; }blocks can still kill the run beforeglobal_exit_codeis updated. This is the exact problem raised in the previous review and it remains unresolved.-set -euo pipefail +# keep -u/-o but drop –e after setup so every test runs +set -uo pipefail
47-50:differror codes > 1 still treated as “files differ”You kept the plain
if diff …; thenconstruct, so an I/O error (exit > 1) is silently interpreted as a legitimate mismatch. Capturing the RC avoids false negatives:- if diff -w git-reference.log git-test.log ; then + diff -w git-reference.log git-test.log + rc=$? + if [[ $rc -eq 0 ]]; then + # identical + elif [[ $rc -eq 1 ]]; then + # files differ (expected branch) + else + echo "Test $test : ERROR (diff failed, rc=$rc)" + global_exit_code=2 + fi
🧹 Nitpick comments (2)
_tests.sh (2)
68-73: Unquoted paths explode when the repo path contains spacesEvery occurrence of
$remote_tester_repo,$local_tester_repo,$clone_tester_repoand$(pwd)should be wrapped in double quotes to avoid word-splitting and globbing surprises.-git init --bare -b "${default_branch:-main}" $remote_tester_repo || { - git init --bare $remote_tester_repo +git init --bare -b "${default_branch:-main}" "$remote_tester_repo" || { + git init --bare "$remote_tester_repo"(Apply the same quoting throughout the function.)
228-237: Word-splitting risk infor log in $(find …)loopThe
$()command substitution splits on IFS, so filenames with spaces/new-lines break the loop. Safer to read fromfinddirectly:-for log in $( find . -name run.log -o -name nok.log | sort ); do +find . \( -name run.log -o -name nok.log \) -print0 | sort -z | while IFS= read -r -d '' log; do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.test/2/git-reference.logis excluded by!**/*.log.test/3/git-reference.logis excluded by!**/*.log.test/4/git-reference.logis excluded by!**/*.log.test/5/git-reference.logis excluded by!**/*.log.test/6/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (2)
.gitignore(1 hunks)_tests.sh(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
bicschneider
force-pushed
the
bicschneider/issue18-git2.49.0-test-problem
branch
from
f6a1308 to
477994d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/pr.yml (2)
32-32: Update outdated GitHub Actions.The
actions/checkout@v2action is outdated and should be updated to a more recent version.- uses: actions/checkout@v2 + uses: actions/checkout@v4Also applies to other checkout actions in the file.
50-53: Fix shell quoting issues and trailing whitespace.Static analysis detected shell quoting issues and YAML formatting problems that should be addressed.
- docker build --build-arg ALPINE_GIT_DOCKER_VERSION=${git_version} --build-arg USER_ID=$(id -u) --build-arg GROUP_ID=$(id -g) -t git-artifact:${git_version} . - docker run --rm -u $(id -u):$(id -g) -e debug=${{ github.event.inputs.debug}} -e verbose=${{ github.event.inputs.verbose}} -v $(pwd):/git -v $HOME/.ssh:/root/.ssh git-artifact:latest --version - docker run --rm -u $(id -u):$(id -g) -e debug=${{ github.event.inputs.debug}} -e verbose=${{ github.event.inputs.verbose}} -v $(pwd):/git -v $HOME/.ssh:/root/.ssh git-artifact:latest artifact-tests + docker build --build-arg ALPINE_GIT_DOCKER_VERSION="${git_version}" --build-arg USER_ID="$(id -u)" --build-arg GROUP_ID="$(id -g)" -t "git-artifact:${git_version}" . + docker run --rm -u "$(id -u):$(id -g)" -e debug="${{ github.event.inputs.debug }}" -e verbose="${{ github.event.inputs.verbose }}" -v "$(pwd):/git" -v "$HOME/.ssh:/root/.ssh" "git-artifact:${git_version}" --version + docker run --rm -u "$(id -u):$(id -g)" -e debug="${{ github.event.inputs.debug }}" -e verbose="${{ github.event.inputs.verbose }}" -v "$(pwd):/git" -v "$HOME/.ssh:/root/.ssh" "git-artifact:${git_version}" artifact-testsAlso remove trailing whitespace on line 53.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.test/2/git-reference.logis excluded by!**/*.log.test/3/git-reference.logis excluded by!**/*.log.test/4/git-reference.logis excluded by!**/*.log.test/5/git-reference.logis excluded by!**/*.log.test/6/git-reference.logis excluded by!**/*.log
📒 Files selected for processing (5)
.github/workflows/pr.yml(5 hunks).gitignore(1 hunks)Dockerfile(1 hunks)_tests.sh(5 hunks)git-artifact(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- Dockerfile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr.yml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[warning] 55-55: wrong indentation: expected 6 but found 8
(indentation)
[warning] 57-57: wrong indentation: expected 10 but found 8
(indentation)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
[warning] 69-69: wrong indentation: expected 6 but found 8
(indentation)
[warning] 71-71: wrong indentation: expected 10 but found 8
(indentation)
[error] 74-74: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/pr.yml
58-58: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-60: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
72-72: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
74-74: shellcheck reported issue in this script: SC2046:warning:3:87: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:3:117: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:20: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:29: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:4:127: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:4:142: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:20: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:29: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2046:warning:5:127: Quote this to prevent word splitting
(shellcheck)
74-74: shellcheck reported issue in this script: SC2086:info:5:142: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (11)
git-artifact (1)
337-340: LGTM! Debug output enhancement aligns well with logging improvements.The explicit echo of the debug variable before enabling shell tracing improves traceability and consistency with similar changes in
_tests.sh. The block format is more readable than the single-line conditional..github/workflows/pr.yml (4)
9-10: Good addition of scheduled monthly runs.The monthly cron schedule will help catch regressions and compatibility issues over time.
14-25: Excellent enhancement with configurable debug and verbose inputs.These workflow inputs provide flexible control over test execution and align well with the debug/verbose handling added to the test scripts.
54-66: New v2.49.0 test job addresses the PR objective.Good addition of Git v2.49.0 testing to resolve the test problem mentioned in the PR title. The job follows the established pattern with proper user/group ID handling.
However, apply the same shell quoting fixes as mentioned in the previous comment.
68-80: Good addition of v2.47.2 test coverage.This expands the Git version test matrix appropriately. Apply the same shell quoting and formatting fixes as other jobs.
_tests.sh (6)
6-9: Good debug output enhancement.The explicit echo of the debug variable before enabling shell tracing improves consistency with
git-artifactand provides better traceability.
23-23: Excellent addition of global exit code tracking.This allows all tests to run and accumulate failures rather than exiting on the first error, which is much better for CI/testing scenarios.
37-40: Good workaround for Git 2.48 compatibility.The removal of
.git/refs/remotes/origin/HEADaddresses backward compatibility issues. This aligns with the PR objective to fix Git v2.49.0 test problems.
55-61: Improved test result handling and logging.The renaming of logs to
ok.log/nok.logand settingglobal_exit_code=2instead of immediate exit allows for better test aggregation and reporting.
85-89: Enhanced log documentation improves clarity.The expanded explanation of the three log types (
run.log,ok.log,nok.log) helps users understand the test output structure.
216-241: Excellent test summary and aggregated exit handling.The final summary section provides clear feedback on test results and properly exits with the accumulated error code. This is a significant improvement over the previous immediate-exit behavior.
|
|
||
| clear | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider the interaction between set -e and test aggregation.
The set -e flag may still cause the script to exit before reaching the new summary logic if any unexpected command fails. The test execution blocks are wrapped in { } > log 2>&1 which should contain failures, but other commands between tests could still trigger early exit.
Run this script to check if there are any commands outside the test blocks that could fail:
🏁 Script executed:
#!/bin/bash
# Check for potentially failing commands outside test execution blocks
echo "Checking for commands that might fail with set -e enabled..."
echo "Commands between test blocks:"
gawk '/^[[:space:]]*} > .*\/run\.log 2>&1[[:space:]]*$/{intest=0} !intest && !/^[[:space:]]*#/ && !/^[[:space:]]*$/ && /[a-zA-Z]/ {print NR ": " $0} /^[[:space:]]*{[[:space:]]*$/{intest=1}' _tests.shLength of output: 3964
Prevent unintended exits from setup commands under set -e
The initial setup and function definitions in _tests.sh include several commands outside the test–block braces that could fail and abort the entire script before your new summary logic runs. For example:
- Line 3:
clear - Lines 12–13:
which git/git --version - Line 15:
cd .test - Line 18:
git clean -xffdq . - Lines 72–81 (inside
generate_base_repo): variousgit artifactinvocations
If any of these commands return a non-zero status, set -euo pipefail will terminate the script prematurely.
To guard against this, consider one of the following fixes:
- Move
set -euo pipefailinside each test block (or intoeval_testcase) so it only applies during test execution. - Wrap non-critical setup commands with
|| true. - Temporarily disable
-efor the setup phase usingset +e…set -earound those commands.
This will ensure that an unexpected failure in your environment setup doesn’t bypass the aggregated test summary.
🤖 Prompt for AI Agents
In _tests.sh at line 5, the use of 'set -euo pipefail' causes the script to exit
prematurely if any setup commands fail. To fix this, either move 'set -euo
pipefail' inside each test block or the eval_testcase function so it only
applies during test execution, or wrap non-critical setup commands like 'clear',
'which git', 'git --version', 'cd .test', and 'git clean -xffdq .' with '||
true' to prevent failure exits, or temporarily disable the '-e' option around
these setup commands using 'set +e' before and 'set -e' after them. This will
prevent unintended script termination during setup and allow the test summary
logic to run.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores