Skip to content

Commit 9eb48e0

Browse files
pattonwebzpattonwebz
committed
Address code review feedback on @SInCE tool and release script
1 parent 4c1dfeb commit 9eb48e0

2 files changed

Lines changed: 104 additions & 53 deletions

File tree

‎scripts/prep_release.sh‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -34,26 +34,21 @@ DEVELOP_BRANCH=develop
3434
RELEASE_BRANCH_NAME=release/${BUMPED_VERSION}
3535

3636
# Ensure local tags are fully in sync with remote tags before release prep.
37+
# Uses process substitution to avoid orphaned temp files on early exit.
3738
verify_local_tags_match_remote() {
38-
local local_tags_file
39-
local remote_tags_file
40-
41-
local_tags_file=$(mktemp)
42-
remote_tags_file=$(mktemp)
43-
44-
# Always clean up temporary files before returning.
45-
trap 'rm -f "${local_tags_file}" "${remote_tags_file}"' RETURN
46-
47-
# --refs avoids peeled annotated-tag entries (^{}) for clean one-line refs.
48-
git show-ref --tags | awk '{print $1" "$2}' | sort > "${local_tags_file}"
49-
git ls-remote --tags --refs origin | awk '{print $1" "$2}' | sort > "${remote_tags_file}"
50-
51-
if ! diff -u "${remote_tags_file}" "${local_tags_file}" >/dev/null; then
39+
# --refs strips peeled annotated-tag entries (^{}) for clean one-line refs.
40+
if ! diff -u \
41+
<(git ls-remote --tags --refs origin 2>/dev/null | awk '{print $1" "$2}' | sort) \
42+
<(git show-ref --tags 2>/dev/null | awk '{print $1" "$2}' | sort) \
43+
>/dev/null; then
5244
echo
5345
echo "Error: local tags do not match tags on origin."
54-
echo "Please sync tags before running release prep."
46+
echo "Please sync tags before running release prep:"
47+
echo " git fetch origin --tags --prune-tags"
5548
echo
56-
diff -u "${remote_tags_file}" "${local_tags_file}" || true
49+
diff -u \
50+
<(git ls-remote --tags --refs origin 2>/dev/null | awk '{print $1" "$2}' | sort) \
51+
<(git show-ref --tags 2>/dev/null | awk '{print $1" "$2}' | sort) || true
5752
return 1
5853
fi
5954

@@ -206,32 +201,40 @@ echo
206201

207202
# Stash any existing uncommitted work (tracked modifications + untracked files)
208203
# so that the subsequent git diff picks up only what the tool changes.
204+
# Track stash ref before/after to guard against popping an unrelated stash when
205+
# git stash push exits 0 even if there was nothing to save.
209206
STASH_MESSAGE="prep_release: pre-since-tag stash"
207+
STASH_BEFORE=$(git rev-parse -q --verify refs/stash || true)
210208
git stash push --include-untracked -m "${STASH_MESSAGE}"
211-
STASH_CREATED=$?
209+
STASH_AFTER=$(git rev-parse -q --verify refs/stash || true)
210+
STASH_CREATED=false
211+
if [[ -n "${STASH_AFTER}" && "${STASH_AFTER}" != "${STASH_BEFORE}" ]]; then
212+
STASH_CREATED=true
213+
fi
212214

213215
# Ensure the stash is always restored, even if the script exits early.
216+
# Setting STASH_CREATED=false before popping prevents the EXIT trap double-pop.
214217
restore_stash() {
215-
if [[ ${STASH_CREATED} -eq 0 ]]; then
218+
if [[ "${STASH_CREATED}" == true ]]; then
216219
echo
217220
echo "Restoring stashed changes"
218221
echo
222+
STASH_CREATED=false
219223
git stash pop
220224
fi
221225
}
222226
trap restore_stash EXIT
223227

224228
php tools/update-since-tags.php --version="${BUMPED_VERSION}" --changed-since-last-tag
225229

226-
# Stage only the tracked PHP files that were modified by the tool.
227-
# git diff --name-only only lists tracked files with unstaged changes,
228-
# so untracked files are never included.
229-
SINCE_CHANGES=$(git diff --name-only -- '*.php')
230-
if [[ -n "${SINCE_CHANGES}" ]]; then
230+
# Stage only the tracked PHP files modified by the tool.
231+
# Use NUL-delimited output + mapfile + xargs -0 so paths with spaces are safe.
232+
mapfile -d '' SINCE_CHANGES < <(git diff --name-only -z -- '*.php')
233+
if (( ${#SINCE_CHANGES[@]} > 0 )); then
231234
echo
232235
echo "Committing @since tag updates"
233236
echo
234-
echo "${SINCE_CHANGES}" | xargs git add --
237+
printf '%s\0' "${SINCE_CHANGES[@]}" | xargs -0 git add --
235238
git commit -m "Update @since placeholders to ${BUMPED_VERSION}"
236239
else
237240
echo "No @since placeholder tags found — skipping commit."

‎tools/update-since-tags.php‎

Lines changed: 77 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
edac_write_line( 'Usage: php tools/update-since-tags.php --version=<x.y.z> [options]' );
3636
edac_write_line( '' );
3737
edac_write_line( 'Options:' );
38-
edac_write_line( ' --root=<path> Root directory to scan (default: current working directory)' );
38+
edac_write_line( ' --root=<path> Root directory to scan (default: parent of the tools/ directory)' );
3939
edac_write_line( ' --placeholder=<value> Placeholder token after @since (default: x.x.x)' );
4040
edac_write_line( ' --changed-since-tag=<tag> Only scan tracked PHP files changed since this Git tag/ref' );
4141
edac_write_line( ' --changed-since-last-tag Only scan tracked PHP files changed since latest Git tag' );
@@ -51,20 +51,41 @@
5151
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
5252
}
5353

54-
$root = $opts['root'] ?? dirname( __DIR__ );
55-
if ( ! is_string( $root ) || ! is_dir( $root ) ) {
56-
edac_write_line( 'Error: root directory does not exist: ' . (string) $root, STDERR );
54+
// getopt() returns false (not null) for optional params given without a value (e.g. --root).
55+
$root_opt = $opts['root'] ?? dirname( __DIR__ );
56+
if ( false === $root_opt ) {
57+
edac_write_line( 'Error: --root requires a value.', STDERR );
58+
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
59+
}
60+
if ( ! is_string( $root_opt ) || ! is_dir( $root_opt ) ) {
61+
edac_write_line( 'Error: root directory does not exist: ' . (string) $root_opt, STDERR );
62+
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
63+
}
64+
// Use realpath() so filesystem-root paths like / or C:\ are preserved correctly.
65+
$root = realpath( $root_opt );
66+
if ( false === $root ) {
67+
edac_write_line( 'Error: could not resolve root directory: ' . $root_opt, STDERR );
5768
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
5869
}
59-
$root = rtrim( (string) $root, DIRECTORY_SEPARATOR );
6070

61-
$placeholder = $opts['placeholder'] ?? 'x.x.x';
62-
if ( ! is_string( $placeholder ) || '' === trim( $placeholder ) ) {
71+
$placeholder_opt = $opts['placeholder'] ?? 'x.x.x';
72+
if ( false === $placeholder_opt ) {
73+
edac_write_line( 'Error: --placeholder requires a value.', STDERR );
74+
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
75+
}
76+
if ( ! is_string( $placeholder_opt ) || '' === trim( $placeholder_opt ) ) {
6377
edac_write_line( 'Error: --placeholder must be a non-empty string.', STDERR );
6478
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
6579
}
80+
$placeholder = $placeholder_opt;
6681

67-
$changed_since_tag = $opts['changed-since-tag'] ?? null;
82+
// getopt() returns false for optional params given without a value (e.g. --changed-since-tag).
83+
$changed_since_tag_opt = $opts['changed-since-tag'] ?? null;
84+
if ( false === $changed_since_tag_opt ) {
85+
edac_write_line( 'Error: --changed-since-tag requires a value.', STDERR );
86+
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
87+
}
88+
$changed_since_tag = $changed_since_tag_opt;
6889
$changed_since_last_tag = isset( $opts['changed-since-last-tag'] );
6990
$dry_run = isset( $opts['dry-run'] );
7091

@@ -73,6 +94,15 @@
7394
exit( EDAC_SINCE_TOOL_EXIT_BAD_ARGS ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
7495
}
7596

97+
// Validate the supplied ref actually exists before proceeding.
98+
if ( null !== $changed_since_tag ) {
99+
$ref_check = trim( edac_run_git( $root, 'rev-parse --verify ' . escapeshellarg( (string) $changed_since_tag ) ) );
100+
if ( '' === $ref_check ) {
101+
edac_write_line( 'Error: invalid Git reference or tag: ' . (string) $changed_since_tag, STDERR );
102+
exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
103+
}
104+
}
105+
76106
if ( $changed_since_last_tag ) {
77107
$changed_since_tag = trim( edac_run_git( $root, 'describe --tags --abbrev=0' ) );
78108
if ( '' === $changed_since_tag ) {
@@ -147,28 +177,36 @@
147177
* @return string[]
148178
*/
149179
function edac_get_all_php_files( string $root, array $excluded_dirs ): array {
150-
$files = [];
151-
$iterator = new RecursiveIteratorIterator(
152-
new RecursiveDirectoryIterator( $root, RecursiveDirectoryIterator::SKIP_DOTS )
180+
$files = [];
181+
$directory_iterator = new RecursiveDirectoryIterator( $root, RecursiveDirectoryIterator::SKIP_DOTS );
182+
183+
// Prune excluded directories before recursing into them for efficiency.
184+
$filter = new RecursiveCallbackFilterIterator(
185+
$directory_iterator,
186+
// phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed,VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
187+
static function ( SplFileInfo $item, $key, RecursiveCallbackFilterIterator $iterator ) use ( $root, $excluded_dirs ): bool {
188+
if ( $item->isDir() ) {
189+
$relative_path = ltrim( substr( $item->getPathname(), strlen( $root ) ), DIRECTORY_SEPARATOR );
190+
foreach ( $excluded_dirs as $excluded ) {
191+
if ( $relative_path === $excluded || 0 === strpos( $relative_path, $excluded . DIRECTORY_SEPARATOR ) ) {
192+
return false;
193+
}
194+
}
195+
}
196+
197+
return true;
198+
}
153199
);
154200

201+
$iterator = new RecursiveIteratorIterator( $filter );
202+
155203
foreach ( $iterator as $item ) {
156204
if ( ! $item instanceof SplFileInfo || $item->isDir() ) {
157205
continue;
158206
}
159207

160-
$path = $item->getPathname();
161-
$relative_path = ltrim( str_replace( $root, '', $path ), DIRECTORY_SEPARATOR );
162-
163-
foreach ( $excluded_dirs as $excluded ) {
164-
$needle = $excluded . DIRECTORY_SEPARATOR;
165-
if ( 0 === strpos( $relative_path, $needle ) || false !== strpos( $relative_path, DIRECTORY_SEPARATOR . $needle ) ) {
166-
continue 2;
167-
}
168-
}
169-
170208
if ( 'php' === strtolower( (string) $item->getExtension() ) ) {
171-
$files[] = $path;
209+
$files[] = $item->getPathname();
172210
}
173211
}
174212

@@ -180,13 +218,21 @@ function edac_get_all_php_files( string $root, array $excluded_dirs ): array {
180218
/**
181219
* Get tracked PHP files changed between a given ref and HEAD.
182220
*
221+
* Exits with EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR if the git command fails,
222+
* so a bad ref or non-repo root does not silently return an empty list.
223+
*
183224
* @param string $root Root directory.
184225
* @param string $ref Git ref/tag.
185226
* @return string[]
186227
*/
187228
function edac_get_changed_php_files_since_ref( string $root, string $ref ): array {
188229
$cmd = 'diff --name-only ' . escapeshellarg( $ref . '..HEAD' ) . ' -- ' . escapeshellarg( '*.php' );
189-
$output = edac_run_git( $root, $cmd );
230+
$output = edac_run_git( $root, $cmd, $exit_code );
231+
232+
if ( 0 !== $exit_code ) {
233+
edac_write_line( "Error: git diff failed (exit {$exit_code}) for ref: {$ref}", STDERR );
234+
exit( EDAC_SINCE_TOOL_EXIT_RUNTIME_ERROR ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
235+
}
190236

191237
$files = [];
192238
foreach ( preg_split( '/\r?\n/', trim( $output ) ) as $line ) {
@@ -208,15 +254,17 @@ function edac_get_changed_php_files_since_ref( string $root, string $ref ): arra
208254
/**
209255
* Run a Git command in the target root and return stdout.
210256
*
211-
* @param string $root Root directory.
212-
* @param string $args Git args.
257+
* @param string $root Root directory.
258+
* @param string $args Git args (already shell-escaped as needed).
259+
* @param int|null $exit_code Reference populated with the git process exit code.
213260
* @return string
214261
*/
215-
function edac_run_git( string $root, string $args ): string {
216-
$cmd = 'git -C ' . escapeshellarg( $root ) . ' ' . $args . ' 2>/dev/null';
217-
$output = shell_exec( $cmd ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_shell_exec
262+
function edac_run_git( string $root, string $args, ?int &$exit_code = null ): string {
263+
$cmd = 'git -C ' . escapeshellarg( $root ) . ' ' . $args;
264+
$output = [];
265+
exec( $cmd, $output, $exit_code ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec
218266

219-
return is_string( $output ) ? $output : '';
267+
return implode( "\n", $output );
220268
}
221269

222270
/**

0 commit comments

Comments
 (0)