Add @since update tool and release prep integration#1798
Conversation
📝 WalkthroughWalkthrough
Release Prep Hardening
ChangesRelease Prep Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 release preparation script and a PHP tool to automate tag verification and update placeholder @SInCE tags in PHP docblocks. The review feedback highlights several critical issues and improvement opportunities: a bug in the stash restoration logic that could pop unrelated stashes when no local changes exist, a recommendation to use process substitution instead of temporary files for tag comparison, and type-handling issues in the PHP script where getopt returning false for optional parameters could bypass null coalescing checks and cause runtime errors or silent failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Stash any existing uncommitted work (tracked modifications + untracked files) | ||
| # so that the subsequent git diff picks up only what the tool changes. | ||
| STASH_MESSAGE="prep_release: pre-since-tag stash" | ||
| git stash push --include-untracked -m "${STASH_MESSAGE}" | ||
| STASH_CREATED=$? | ||
|
|
||
| # Ensure the stash is always restored, even if the script exits early. | ||
| restore_stash() { | ||
| if [[ ${STASH_CREATED} -eq 0 ]]; then | ||
| echo | ||
| echo "Restoring stashed changes" | ||
| echo | ||
| git stash pop | ||
| fi | ||
| } | ||
| trap restore_stash EXIT |
There was a problem hiding this comment.
Unrelated Stash Pop Bug
If there are no local changes to stash, git stash push will print No local changes to save and exit with status 0. This means STASH_CREATED will be set to 0.
When restore_stash is subsequently called, it checks if STASH_CREATED is 0 and executes git stash pop. Since no stash was actually created by this script, git stash pop will pop and apply the topmost unrelated stash from the user's stash stack (or fail if the stack is empty). This can lead to silent data corruption or unexpected merge conflicts.
To fix this, we should check if there are any uncommitted changes (tracked or untracked) before attempting to stash.
| # Stash any existing uncommitted work (tracked modifications + untracked files) | |
| # so that the subsequent git diff picks up only what the tool changes. | |
| STASH_MESSAGE="prep_release: pre-since-tag stash" | |
| git stash push --include-untracked -m "${STASH_MESSAGE}" | |
| STASH_CREATED=$? | |
| # Ensure the stash is always restored, even if the script exits early. | |
| restore_stash() { | |
| if [[ ${STASH_CREATED} -eq 0 ]]; then | |
| echo | |
| echo "Restoring stashed changes" | |
| echo | |
| git stash pop | |
| fi | |
| } | |
| trap restore_stash EXIT | |
| # Check if there are any uncommitted changes (tracked or untracked) to stash. | |
| HAS_STASHED=false | |
| if [[ -n "$(git status --porcelain)" ]]; then | |
| echo "Stashing local changes before updating @since tags..." | |
| STASH_MESSAGE="prep_release: pre-since-tag stash" | |
| git stash push --include-untracked -m "${STASH_MESSAGE}" | |
| HAS_STASHED=true | |
| fi | |
| # Ensure the stash is always restored, even if the script exits early. | |
| restore_stash() { | |
| if [[ "${HAS_STASHED}" == "true" ]]; then | |
| echo | |
| echo "Restoring stashed changes" | |
| echo | |
| git stash pop | |
| fi | |
| } | |
| trap restore_stash EXIT |
There was a problem hiding this comment.
Fixed in commit 9eb48e01. The stash logic now compares refs/stash before and after the push — if the ref is unchanged (nothing was stashed), STASH_CREATED stays false and restore_stash is skipped entirely, preventing any pop of an unrelated stash.
| verify_local_tags_match_remote() { | ||
| local local_tags_file | ||
| local remote_tags_file | ||
|
|
||
| local_tags_file=$(mktemp) | ||
| remote_tags_file=$(mktemp) | ||
|
|
||
| # Always clean up temporary files before returning. | ||
| trap 'rm -f "${local_tags_file}" "${remote_tags_file}"' RETURN | ||
|
|
||
| # --refs avoids peeled annotated-tag entries (^{}) for clean one-line refs. | ||
| git show-ref --tags | awk '{print $1" "$2}' | sort > "${local_tags_file}" | ||
| git ls-remote --tags --refs origin | awk '{print $1" "$2}' | sort > "${remote_tags_file}" | ||
|
|
||
| if ! diff -u "${remote_tags_file}" "${local_tags_file}" >/dev/null; then | ||
| echo | ||
| echo "Error: local tags do not match tags on origin." | ||
| echo "Please sync tags before running release prep." | ||
| echo | ||
| diff -u "${remote_tags_file}" "${local_tags_file}" || true | ||
| return 1 | ||
| fi | ||
|
|
||
| echo "Tag sync check passed: local tags match origin." | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Process Substitution Refactoring
Instead of creating temporary files with mktemp and setting up a RETURN trap to clean them up, you can use Bash's process substitution (<(...)). This is much cleaner, more idiomatic, and completely avoids the risk of leaving orphaned temporary files if the script exits prematurely (e.g., due to set -e before the trap is triggered).
| verify_local_tags_match_remote() { | |
| local local_tags_file | |
| local remote_tags_file | |
| local_tags_file=$(mktemp) | |
| remote_tags_file=$(mktemp) | |
| # Always clean up temporary files before returning. | |
| trap 'rm -f "${local_tags_file}" "${remote_tags_file}"' RETURN | |
| # --refs avoids peeled annotated-tag entries (^{}) for clean one-line refs. | |
| git show-ref --tags | awk '{print $1" "$2}' | sort > "${local_tags_file}" | |
| git ls-remote --tags --refs origin | awk '{print $1" "$2}' | sort > "${remote_tags_file}" | |
| if ! diff -u "${remote_tags_file}" "${local_tags_file}" >/dev/null; then | |
| echo | |
| echo "Error: local tags do not match tags on origin." | |
| echo "Please sync tags before running release prep." | |
| echo | |
| diff -u "${remote_tags_file}" "${local_tags_file}" || true | |
| return 1 | |
| fi | |
| echo "Tag sync check passed: local tags match origin." | |
| return 0 | |
| } | |
| verify_local_tags_match_remote() { | |
| # Use process substitution to compare remote and local tags without creating temporary files. | |
| # 2>/dev/null ensures we don't trigger set -e if no tags exist. | |
| if ! diff -u <(git ls-remote --tags --refs origin 2>/dev/null | awk '{print $1" "$2}' | sort) <(git show-ref --tags 2>/dev/null | awk '{print $1" "$2}' | sort) >/dev/null; then | |
| echo | |
| echo "Error: local tags do not match tags on origin." | |
| echo "Please sync tags before running release prep." | |
| echo | |
| diff -u <(git ls-remote --tags --refs origin 2>/dev/null | awk '{print $1" "$2}' | sort) <(git show-ref --tags 2>/dev/null | awk '{print $1" "$2}' | sort) || true | |
| return 1 | |
| fi | |
| echo "Tag sync check passed: local tags match origin." | |
| return 0 | |
| } |
There was a problem hiding this comment.
Fixed in commit 9eb48e01. Replaced the mktemp + RETURN-trap approach with process substitution (<(...)) in verify_local_tags_match_remote(). No temp files are created so there is nothing to orphan on early exit.
| $root = $opts['root'] ?? dirname( __DIR__ ); | ||
| if ( ! is_string( $root ) || ! is_dir( $root ) ) { | ||
| fwrite( STDERR, 'Error: root directory does not exist: ' . (string) $root . "\n" ); | ||
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); | ||
| } | ||
| $root = rtrim( (string) $root, DIRECTORY_SEPARATOR ); | ||
|
|
||
| $placeholder = $opts['placeholder'] ?? 'x.x.x'; | ||
| if ( ! is_string( $placeholder ) || '' === trim( $placeholder ) ) { | ||
| fwrite( STDERR, "Error: --placeholder must be a non-empty string.\n" ); | ||
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); | ||
| } |
There was a problem hiding this comment.
Handle false from getopt for Optional Parameters
In PHP, getopt returns false for optional parameters (defined with ::) if they are specified on the command line without a value (e.g., --root or --placeholder).
Because the null coalescing operator (??) only checks for null, a value of false will bypass it and be assigned directly to $root or $placeholder. This leads to type errors or unexpected behavior (e.g., is_dir(false) is false, resulting in a cryptic error message).
We should explicitly check for false and handle it gracefully.
$root = $opts['root'] ?? dirname( __DIR__ );
if ( false === $root ) {
fwrite( STDERR, "Error: --root requires a value.\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}
if ( ! is_string( $root ) || ! is_dir( $root ) ) {
fwrite( STDERR, 'Error: root directory does not exist: ' . (string) $root . "\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}
$root = rtrim( (string) $root, DIRECTORY_SEPARATOR );
$placeholder = $opts['placeholder'] ?? 'x.x.x';
if ( false === $placeholder ) {
fwrite( STDERR, "Error: --placeholder requires a value.\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}
if ( ! is_string( $placeholder ) || '' === trim( $placeholder ) ) {
fwrite( STDERR, "Error: --placeholder must be a non-empty string.\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}There was a problem hiding this comment.
Fixed in commit 9eb48e01. Added explicit false === $root_opt and false === $placeholder_opt guards before the existing is_string/is_dir checks, both exit with a clear error message when an optional flag is supplied without a value.
| $changed_since_tag = $opts['changed-since-tag'] ?? null; | ||
| $changed_since_last_tag = isset( $opts['changed-since-last-tag'] ); | ||
| $dry_run = isset( $opts['dry-run'] ); | ||
|
|
||
| if ( null !== $changed_since_tag && $changed_since_last_tag ) { | ||
| fwrite( STDERR, "Error: use either --changed-since-tag or --changed-since-last-tag, not both.\n" ); | ||
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); | ||
| } |
There was a problem hiding this comment.
Handle false from getopt and Validate Git Reference
Similar to --root, if --changed-since-tag is passed without a value, getopt returns false. This causes null !== $changed_since_tag to evaluate to true, leading to an invalid Git command (git diff '..HEAD') and a silent failure where the tool reports success but scans 0 files.
Additionally, we should validate that the provided Git reference actually exists in the repository using git rev-parse --verify to prevent silent failures when a typo is made in the tag name.
$changed_since_tag = $opts['changed-since-tag'] ?? null;
if ( false === $changed_since_tag ) {
fwrite( STDERR, "Error: --changed-since-tag requires a value.\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}
$changed_since_last_tag = isset( $opts['changed-since-last-tag'] );
$dry_run = isset( $opts['dry-run'] );
if ( null !== $changed_since_tag && $changed_since_last_tag ) {
fwrite( STDERR, "Error: use either --changed-since-tag or --changed-since-last-tag, not both.\n" );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS );
}
if ( null !== $changed_since_tag ) {
$ref_check = edac_run_git( $root, 'rev-parse --verify ' . escapeshellarg( $changed_since_tag ) );
if ( '' === trim( $ref_check ) ) {
fwrite( STDERR, "Error: invalid Git reference or tag: {$changed_since_tag}\n" );
exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR );
}
}There was a problem hiding this comment.
Fixed in commit 9eb48e01. Added a false === $changed_since_tag_opt guard to catch --changed-since-tag given without a value, and a git rev-parse --verify check to validate the ref before use — exits with EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR if the ref does not exist.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/prep_release.sh`:
- Around line 210-220: The restore_stash logic in prep_release.sh should track
the exact stash entry created by git stash push, not just rely on STASH_CREATED,
because an empty push can still return success and a later pop may hit an
unrelated stash. Update restore_stash to record and verify the specific stash
reference created by the push, and make sure the EXIT trap is cleared before
attempting git stash pop so a failed pop cannot be retried by the trap; apply
the same fix wherever the stash restore flow is duplicated.
- Around line 229-235: The staging step in prep_release.sh currently uses echo
piped into xargs, which will break PHP paths containing spaces or other special
characters. Update the changed-files collection and staging flow around
SINCE_CHANGES so it uses NUL-delimited output and a matching xargs invocation,
and keep the git add logic in the same release-commit block so paths are staged
safely before the git commit.
In `@tools/update-since-tags.php`:
- Around line 186-218: The Git command path in edac_run_git() is swallowing
failures by redirecting stderr and converting non-string shell_exec output to an
empty string, which makes edac_get_changed_php_files_since_ref() treat command
errors as “no files changed.” Update edac_run_git() to detect a non-zero git
exit status for the diff call and surface that failure to the caller, then have
edac_get_changed_php_files_since_ref() stop and trigger
EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR when the git command fails instead of
returning an empty file list.
- Line 37: The help text for the CLI option is misleading because `--root` is
documented as defaulting to the current working directory, while the option
parsing in `update-since-tags.php` actually uses `dirname(__DIR__)` as the
default. Update the usage string near the `--root` help entry to match the real
default used by the option handling logic, and make sure the default shown there
stays consistent with the `--root` parsing code.
- Around line 34-43: The CLI script has PHPCS/WPCS violations from unescaped raw
output, the `$mode` assignment pattern, and trailing blank lines at the end of
the file. Clean up the help/usage output in the main option-handling block of
`update-since-tags.php` so it matches WordPress CLI coding standards, refactor
the `$mode` logic to avoid the flagged assignment style, and remove any extra
EOF whitespace so the file passes linting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffc37435-1d08-4fdd-9dfb-bd649958878e
📒 Files selected for processing (2)
scripts/prep_release.shtools/update-since-tags.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/update-since-tags.php (1)
149-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrune excluded directories before recursion.
This still walks through
.git,vendor,node_modules,build, anddist; the prefix check only drops files after the iterator has already descended into those trees. On large repos that makes the “full scan” path much slower than intended.Suggested refactor
- $iterator = new RecursiveIteratorIterator( - new RecursiveDirectoryIterator( $root, RecursiveDirectoryIterator::SKIP_DOTS ) - ); + $directory_iterator = new RecursiveDirectoryIterator( $root, RecursiveDirectoryIterator::SKIP_DOTS ); + $filter = new RecursiveCallbackFilterIterator( + $directory_iterator, + static function ( SplFileInfo $item, $key, RecursiveCallbackFilterIterator $iterator ) use ( $root, $excluded_dirs ): bool { + $path = $item->getPathname(); + $relative_path = ltrim( substr( $path, strlen( $root ) ), DIRECTORY_SEPARATOR ); + + if ( $item->isDir() ) { + foreach ( $excluded_dirs as $excluded ) { + if ( $relative_path === $excluded || 0 === strpos( $relative_path, $excluded . DIRECTORY_SEPARATOR ) ) { + return false; + } + } + } + + return true; + } + ); + $iterator = new RecursiveIteratorIterator( $filter ); @@ - foreach ( $excluded_dirs as $excluded ) { - $needle = $excluded . DIRECTORY_SEPARATOR; - if ( 0 === strpos( $relative_path, $needle ) || false !== strpos( $relative_path, DIRECTORY_SEPARATOR . $needle ) ) { - continue 2; - } - } - if ( 'php' === strtolower( (string) $item->getExtension() ) ) { $files[] = $path; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/update-since-tags.php` around lines 149 - 167, The edac_get_all_php_files() scan still recurses into excluded trees before filtering, so update the RecursiveDirectoryIterator/RecursiveIteratorIterator flow to prune directories up front rather than only skipping files after descent. Add directory-level exclusion handling in edac_get_all_php_files() using the existing $excluded_dirs list so .git, vendor, node_modules, build, and dist are never traversed, and keep the current file-collection logic intact for non-excluded paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/update-since-tags.php`:
- Around line 54-59: The `--root` normalization in `update-since-tags.php` is
stripping valid filesystem root paths by using `rtrim` on `$root`, which turns
`/` into empty and `C:\` into `C:`. Update the root handling logic around
`$opts['root']`, `dirname(__DIR__)`, and the `rtrim` step so absolute filesystem
roots are preserved while still removing only non-root trailing separators.
Ensure later directory traversal continues to use the normalized `$root` value
correctly.
---
Nitpick comments:
In `@tools/update-since-tags.php`:
- Around line 149-167: The edac_get_all_php_files() scan still recurses into
excluded trees before filtering, so update the
RecursiveDirectoryIterator/RecursiveIteratorIterator flow to prune directories
up front rather than only skipping files after descent. Add directory-level
exclusion handling in edac_get_all_php_files() using the existing $excluded_dirs
list so .git, vendor, node_modules, build, and dist are never traversed, and
keep the current file-collection logic intact for non-excluded paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca476be2-836a-47a8-8411-d71a699bb849
📒 Files selected for processing (1)
tools/update-since-tags.php
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/update-since-tags.php`:
- Around line 83-88: The option parsing in update-since-tags.php only rejects
`false`, so repeated `--changed-since-tag` inputs from `getopt()` can slip
through as an array and later become `"Array"` instead of failing as invalid
args. Update the `changed_since_tag_opt` handling to explicitly detect
non-string values/arrays before assigning `changed_since_tag`, and emit the same
bad-args error path used for missing values. Keep the fix localized around the
`changed_since_tag_opt` branch and the `changed_since_tag` assignment.
- Around line 97-103: The ref check in update-since-tags.php is treating every
empty rev-parse result as an invalid tag, even when the real problem is that the
supplied --root is not a valid Git repository. Update the validation flow around
edac_run_git() and the changed_since_tag check so it first verifies the work
tree/repository at $root before blaming the ref, and emit a separate
repository-not-found/error message when Git cannot inspect the repo. Keep the
existing invalid-ref handling only for cases where the repository is valid and
rev-parse --verify fails for the tag/ref itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7224da26-1287-4ba6-a2c0-f94bfb6550a4
📒 Files selected for processing (2)
scripts/prep_release.shtools/update-since-tags.php
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/prep_release.sh
| $changed_since_tag_opt = $opts['changed-since-tag'] ?? null; | ||
| if ( false === $changed_since_tag_opt ) { | ||
| edac_write_line( 'Error: --changed-since-tag requires a value.', STDERR ); | ||
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | ||
| } | ||
| $changed_since_tag = $changed_since_tag_opt; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject repeated --changed-since-tag values.
getopt() returns an array when the same option is passed more than once. This branch only guards false, so repeated --changed-since-tag values can fall through and later get string-cast into "Array" instead of failing cleanly as bad input.
Suggested fix
$changed_since_tag_opt = $opts['changed-since-tag'] ?? null;
if ( false === $changed_since_tag_opt ) {
edac_write_line( 'Error: --changed-since-tag requires a value.', STDERR );
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
}
+if ( null !== $changed_since_tag_opt && ! is_string( $changed_since_tag_opt ) ) {
+ edac_write_line( 'Error: --changed-since-tag may only be provided once.', STDERR );
+ exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
+}
$changed_since_tag = $changed_since_tag_opt;📝 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.
| $changed_since_tag_opt = $opts['changed-since-tag'] ?? null; | |
| if ( false === $changed_since_tag_opt ) { | |
| edac_write_line( 'Error: --changed-since-tag requires a value.', STDERR ); | |
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | |
| } | |
| $changed_since_tag = $changed_since_tag_opt; | |
| $changed_since_tag_opt = $opts['changed-since-tag'] ?? null; | |
| if ( false === $changed_since_tag_opt ) { | |
| edac_write_line( 'Error: --changed-since-tag requires a value.', STDERR ); | |
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | |
| } | |
| if ( null !== $changed_since_tag_opt && ! is_string( $changed_since_tag_opt ) ) { | |
| edac_write_line( 'Error: --changed-since-tag may only be provided once.', STDERR ); | |
| exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | |
| } | |
| $changed_since_tag = $changed_since_tag_opt; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/update-since-tags.php` around lines 83 - 88, The option parsing in
update-since-tags.php only rejects `false`, so repeated `--changed-since-tag`
inputs from `getopt()` can slip through as an array and later become `"Array"`
instead of failing as invalid args. Update the `changed_since_tag_opt` handling
to explicitly detect non-string values/arrays before assigning
`changed_since_tag`, and emit the same bad-args error path used for missing
values. Keep the fix localized around the `changed_since_tag_opt` branch and the
`changed_since_tag` assignment.
| // Validate the supplied ref actually exists before proceeding. | ||
| if ( null !== $changed_since_tag ) { | ||
| $ref_check = trim( edac_run_git( $root, 'rev-parse --verify ' . escapeshellarg( (string) $changed_since_tag ) ) ); | ||
| if ( '' === $ref_check ) { | ||
| edac_write_line( 'Error: invalid Git reference or tag: ' . (string) $changed_since_tag, STDERR ); | ||
| exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the Git work tree before blaming the ref.
This path treats any empty rev-parse --verify output as an invalid ref. If --root points at a non-repo directory, the tool emits the wrong error message even though the real failure is that Git cannot inspect the repository at all.
Suggested fix
// Validate the supplied ref actually exists before proceeding.
if ( null !== $changed_since_tag ) {
+ $repo_check = trim( edac_run_git( $root, 'rev-parse --is-inside-work-tree', $repo_exit_code ) );
+ if ( 0 !== $repo_exit_code || 'true' !== $repo_check ) {
+ edac_write_line( 'Error: --root is not a Git working tree: ' . $root, STDERR );
+ exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
+ }
+
$ref_check = trim( edac_run_git( $root, 'rev-parse --verify ' . escapeshellarg( (string) $changed_since_tag ) ) );
if ( '' === $ref_check ) {
edac_write_line( 'Error: invalid Git reference or tag: ' . (string) $changed_since_tag, STDERR );
exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/update-since-tags.php` around lines 97 - 103, The ref check in
update-since-tags.php is treating every empty rev-parse result as an invalid
tag, even when the real problem is that the supplied --root is not a valid Git
repository. Update the validation flow around edac_run_git() and the
changed_since_tag check so it first verifies the work tree/repository at $root
before blaming the ref, and emit a separate repository-not-found/error message
when Git cannot inspect the repo. Keep the existing invalid-ref handling only
for cases where the repository is valid and rev-parse --verify fails for the
tag/ref itself.
1 participant
What this PR does
tools/update-since-tags.phpto replace placeholder@since x.x.xvalues with a release version.--version,--dry-run,--changed-since-last-tag,--changed-since-tag,--root, and--placeholder.scripts/prep_release.shto:originbefore release prep continues.Commits
Notes
Summary by CodeRabbit
@sinceplaceholders to the target release version.readme.txt.@sinceupdates safer by stashing/restoring local work, updating only relevant tracked PHP changes, and skipping commits when nothing changes.