Fix pgindent issues and run check in Github Actions workflow#305
Fix pgindent issues and run check in Github Actions workflow#305
Conversation
b71d42d to
5d9007e
Compare
rasifr
left a comment
There was a problem hiding this comment.
Even though it’s a standalone CLI utility, Its still part of Spock repo and I would prefer to keep processing it, so the formatting stays consistent across the codebase.
|
Need rebase |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNamed enums replace anonymous typedef enums and Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/spock_shmem.c (1)
139-139: Existing test coverage gap noted in XXX comment.The comment flags missing tests for edge cases like 0 or 1 allowed workers.
Would you like me to help draft test cases or open an issue to track this test coverage improvement?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_shmem.c` at line 139, Add explicit unit tests covering the XXX edge cases noted in src/spock_shmem.c: create tests that exercise allowed-worker edge values (0, 1, and >1) for the spock_shmem initialization/creation code paths (e.g., the functions that accept or compute allowed_workers such as spock_shmem_init / spock_shmem_create or the routine that enforces max/allowed worker counts). For each case assert correct behavior: successful init when 0 or 1 is valid (or proper rejection/error if intended), correct allocation/limits for >1, and any related teardown paths; add these test cases to the shmem test file (e.g., tests/test_spock_shmem.c) and open a tracking issue if you prefer to split adding tests into a separate PR.utils/pgindent/README.md (1)
48-52: Add a language tag to the fenced block.Use a fence language (e.g.,
text) for the branch URL block for markdownlint compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/pgindent/README.md` around lines 48 - 52, The fenced code block containing the three branch URLs (https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=REL_16_STABLE, REL_17_STABLE, REL_18_STABLE) lacks a language tag; update the opening fence from ``` to ```text (or another appropriate fence language) so the block is tagged for markdownlint compatibility and clearer rendering in README.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/spock_conflict_stat.h`:
- Line 44: The header is missing the final closing `#endif` for the include-guard
SPOCK_CONFLICT_STAT_H; after the existing closing for the version check (`#endif`
/* PG_VERSION_NUM >= 180000 */) add the matching `#endif` for the initial `#ifndef`
SPOCK_CONFLICT_STAT_H (with an explanatory comment like /* SPOCK_CONFLICT_STAT_H
*/) so the include-guard is properly terminated and the file preprocesses
correctly.
In `@utils/pgindent/README.md`:
- Around line 59-62: Update the README entry that currently references
".github/workflows/pgindent-check.yml" to point to
".github/workflows/spockbench.yml" and note that the pgindent check runs as a
step (pgindent check) inside that workflow; change any wording that says the
workflow file runs "./run-pgindent.sh --check" on PRs to instead state that the
spockbench workflow contains a pgindent-check step which executes the script
against pinned PG versions and fetches typedefs for supported branches so
contributors are not directed to a non-existent workflow file.
In `@utils/pgindent/run-pgindent.sh`:
- Around line 42-44: The current loop uses "curl -sf ... >> \"$TMPFILE\" || echo
...", which only warns on failure and can produce incomplete typedefs.list;
change the failure handling so a failed fetch for a branch causes the script to
exit non‑zero (fail fast) instead of continuing. Update the curl invocation that
references BUILDFARM_URL, branch, and TMPFILE to log an error to stderr with
context (including $branch) and then exit 1 when curl fails, ensuring CI stops
on missing typedefs rather than producing flaky --check results.
- Around line 50-55: The current loop in run-pgindent.sh that appends typedef
names to "$TMPFILE" only extracts single-line declarations via awk '{print $3}',
missing anonymous multi-line typedefs whose names appear on the closing brace
line (e.g., "typedef enum { ... } Name;"). Modify the block that iterates over
the keywords ("typedef struct" "typedef enum" "typedef union") to also search
for closing-brace lines and extract the identifier before the trailing
semicolon: for each match include both the single-line pattern extraction
(existing awk '{print $3}') and an additional grep/sed/awk pipeline that finds
lines containing a '}' followed by a token and ';' and appends that token to
"$TMPFILE" (use the same TMPFILE variable and the same include filters), so
names like SpockConflictType, SpockSaveOriginConflictOption, SpockWorkerStatus,
and SpockWorkerType are captured.
---
Nitpick comments:
In `@src/spock_shmem.c`:
- Line 139: Add explicit unit tests covering the XXX edge cases noted in
src/spock_shmem.c: create tests that exercise allowed-worker edge values (0, 1,
and >1) for the spock_shmem initialization/creation code paths (e.g., the
functions that accept or compute allowed_workers such as spock_shmem_init /
spock_shmem_create or the routine that enforces max/allowed worker counts). For
each case assert correct behavior: successful init when 0 or 1 is valid (or
proper rejection/error if intended), correct allocation/limits for >1, and any
related teardown paths; add these test cases to the shmem test file (e.g.,
tests/test_spock_shmem.c) and open a tracking issue if you prefer to split
adding tests into a separate PR.
In `@utils/pgindent/README.md`:
- Around line 48-52: The fenced code block containing the three branch URLs
(https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=REL_16_STABLE,
REL_17_STABLE, REL_18_STABLE) lacks a language tag; update the opening fence
from ``` to ```text (or another appropriate fence language) so the block is
tagged for markdownlint compatibility and clearer rendering in README.md.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18be8789-7388-4494-a256-eb273242c0ae
📒 Files selected for processing (38)
.github/workflows/spockbench.ymlinclude/spock.hinclude/spock_conflict.hinclude/spock_conflict_stat.hinclude/spock_group.hinclude/spock_node.hinclude/spock_output_plugin.hinclude/spock_proto_native.hinclude/spock_relcache.hinclude/spock_sync.hinclude/spock_worker.hsrc/compat/18/spock_compat.csrc/spock.csrc/spock_apply.csrc/spock_apply_heap.csrc/spock_autoddl.csrc/spock_conflict.csrc/spock_conflict_stat.csrc/spock_dependency.csrc/spock_executor.csrc/spock_failover_slots.csrc/spock_functions.csrc/spock_group.csrc/spock_node.csrc/spock_output_plugin.csrc/spock_proto_json.csrc/spock_proto_native.csrc/spock_readonly.csrc/spock_relcache.csrc/spock_repset.csrc/spock_rmgr.csrc/spock_shmem.csrc/spock_sync.csrc/spock_worker.cutils/pgindent/README.mdutils/pgindent/gen-typedefs.shutils/pgindent/run-pgindent.shutils/pgindent/typedefs.list
💤 Files with no reviewable changes (2)
- include/spock_proto_native.h
- utils/pgindent/gen-typedefs.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
utils/pgindent/README.md (1)
59-62:⚠️ Potential issue | 🟡 MinorCI workflow reference looks stale.
Line 59 points to
.github/workflows/pgindent.yml, but this PR’s objective says pgindent checks run insidespockbench.yml. Please align the README to the actual workflow entry point.#!/bin/bash set -euo pipefail echo "Workflow files:" fd -p .github/workflows echo echo "pgindent references in workflows:" rg -n "run-pgindent\\.sh|pgindent" .github/workflows -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/pgindent/README.md` around lines 59 - 62, Update the README entry that currently references `.github/workflows/pgindent.yml` and the statement that PG indent checks run via that workflow: change it to reference the actual workflow file `spockbench.yml` as the CI entry point and note that `run-pgindent.sh --check` (or `pgindent`) is invoked from `spockbench.yml`; also remove or reword the claim about building `pg_bsd_indent` from a pinned PG version if that logic moved, and ensure the README mentions the correct workflow filename (`spockbench.yml`) and the related script names (`run-pgindent.sh`, `pgindent`) so readers can find the CI invocation.utils/pgindent/run-pgindent.sh (1)
52-57:⚠️ Potential issue | 🟠 MajorTypedef extraction still misses anonymous multi-line declarations.
Lines 52-57 only capture single-line
typedef struct/enum/union Namepatterns and skip declarations where the typedef name is on the closing brace line (} Name;). This can still produce incompletetypedefs.list.🛠️ Proposed fix
echo "Extracting Spock typedefs from source..." -for keyword in "typedef struct" "typedef enum" "typedef union"; do - grep -rn "$keyword" "$SPOCK_ROOT" \ - --include="*.c" --include="*.h" \ - | grep -v "{" \ - | awk '{print $3}' >> "$TMPFILE" || true -done +find "$SPOCK_ROOT" -type f \( -name "*.c" -o -name "*.h" \) -print0 \ + | xargs -0 awk ' + BEGIN { in_tdef = 0 } + { + if (match($0, /typedef[[:space:]]+(struct|enum|union)[[:space:]]+([A-Za-z_][A-Za-z0-9_]*)/, m)) { + print m[2]; + } + if ($0 ~ /typedef[[:space:]]+(struct|enum|union)[[:space:]]*$/) { + in_tdef = 1; + } + if (in_tdef && match($0, /}[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*;/, m)) { + print m[1]; + in_tdef = 0; + } + } + ' >> "$TMPFILE"#!/bin/bash set -euo pipefail tmp_current=$(mktemp) tmp_expected=$(mktemp) # Current extractor behavior for keyword in "typedef struct" "typedef enum" "typedef union"; do grep -rn "$keyword" . --include="*.c" --include="*.h" \ | grep -v "{" \ | awk '{print $3}' >> "$tmp_current" || true done grep -v '^$' "$tmp_current" | sort -u > "${tmp_current}.sorted" # Multiline-aware baseline find . -type f \( -name "*.c" -o -name "*.h" \) -print0 \ | xargs -0 awk ' BEGIN { in_tdef = 0 } { if (match($0, /typedef[[:space:]]+(struct|enum|union)[[:space:]]+([A-Za-z_][A-Za-z0-9_]*)/, m)) print m[2]; if ($0 ~ /typedef[[:space:]]+(struct|enum|union)[[:space:]]*$/) in_tdef = 1; if (in_tdef && match($0, /}[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*;/, m)) { print m[1]; in_tdef = 0; } } ' | sort -u > "$tmp_expected" echo "Typedefs missed by current extractor:" comm -23 "$tmp_expected" "${tmp_current}.sorted" | head -n 50🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/pgindent/run-pgindent.sh` around lines 52 - 57, The current loop that appends matches to "$TMPFILE" (for keyword in "typedef struct" "typedef enum" "typedef union"; do ... done) only captures single-line typedefs and misses typedef names declared on the closing brace (e.g., "} Name;"); replace that loop with a multiline-aware extractor that scans *.c and *.h files (using awk or a small parser) to emit both single-line typedefs and typedef names found after closing braces, writing the results into "$TMPFILE" and then normalizing with grep -v '^$' | sort -u so the rest of the script can consume the complete typedefs list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/pgindent/README.md`:
- Around line 48-52: The fenced code block containing the three buildfarm URLs
is missing a language specifier (MD040); update the opening fence (the triple
backticks) for that block to include a language token such as text (e.g.,
replace ``` with ```text) so the block becomes a labelled code fence and
satisfies markdownlint.
---
Duplicate comments:
In `@utils/pgindent/README.md`:
- Around line 59-62: Update the README entry that currently references
`.github/workflows/pgindent.yml` and the statement that PG indent checks run via
that workflow: change it to reference the actual workflow file `spockbench.yml`
as the CI entry point and note that `run-pgindent.sh --check` (or `pgindent`) is
invoked from `spockbench.yml`; also remove or reword the claim about building
`pg_bsd_indent` from a pinned PG version if that logic moved, and ensure the
README mentions the correct workflow filename (`spockbench.yml`) and the related
script names (`run-pgindent.sh`, `pgindent`) so readers can find the CI
invocation.
In `@utils/pgindent/run-pgindent.sh`:
- Around line 52-57: The current loop that appends matches to "$TMPFILE" (for
keyword in "typedef struct" "typedef enum" "typedef union"; do ... done) only
captures single-line typedefs and misses typedef names declared on the closing
brace (e.g., "} Name;"); replace that loop with a multiline-aware extractor that
scans *.c and *.h files (using awk or a small parser) to emit both single-line
typedefs and typedef names found after closing braces, writing the results into
"$TMPFILE" and then normalizing with grep -v '^$' | sort -u so the rest of the
script can consume the complete typedefs list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 963df3d4-e11f-40bb-b571-4dce94ee4968
📒 Files selected for processing (8)
.github/workflows/pgindent.ymlinclude/spock_conflict.hinclude/spock_group.hsrc/spock_conflict.cutils/pgindent/README.mdutils/pgindent/gen-typedefs.shutils/pgindent/run-pgindent.shutils/pgindent/typedefs.list
💤 Files with no reviewable changes (1)
- utils/pgindent/gen-typedefs.sh
✅ Files skipped from review due to trivial changes (2)
- include/spock_group.h
- .github/workflows/pgindent.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/spock_conflict.c
- include/spock_conflict.h
There was a problem hiding this comment.
♻️ Duplicate comments (1)
include/spock_conflict_stat.h (1)
44-44:⚠️ Potential issue | 🔴 CriticalMissing closing
#endiffor the include guard (build blocker).The
#ifndef SPOCK_CONFLICT_STAT_Hat line 12 is never closed. The#endifon line 44 only closes the#if PG_VERSION_NUM >= 180000block. This will cause a preprocessing error.,
🔧 Proposed fix
`#endif` /* PG_VERSION_NUM >= 180000 */ + +#endif /* SPOCK_CONFLICT_STAT_H */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/spock_conflict_stat.h` at line 44, The file is missing the closing `#endif` for the include guard SPOCK_CONFLICT_STAT_H; add a final `#endif /* SPOCK_CONFLICT_STAT_H */` after the existing `#endif /* PG_VERSION_NUM >= 180000 */` (i.e., at the end of the header) so the `#ifndef SPOCK_CONFLICT_STAT_H` opened earlier is properly closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@include/spock_conflict_stat.h`:
- Line 44: The file is missing the closing `#endif` for the include guard
SPOCK_CONFLICT_STAT_H; add a final `#endif /* SPOCK_CONFLICT_STAT_H */` after
the existing `#endif /* PG_VERSION_NUM >= 180000 */` (i.e., at the end of the
header) so the `#ifndef SPOCK_CONFLICT_STAT_H` opened earlier is properly
closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd86bf24-a902-492c-8410-18f08892249d
📒 Files selected for processing (33)
include/spock.hinclude/spock_conflict.hinclude/spock_conflict_stat.hinclude/spock_group.hinclude/spock_node.hinclude/spock_output_plugin.hinclude/spock_proto_native.hinclude/spock_relcache.hinclude/spock_sync.hinclude/spock_worker.hsrc/compat/18/spock_compat.csrc/spock.csrc/spock_apply.csrc/spock_apply_heap.csrc/spock_autoddl.csrc/spock_conflict.csrc/spock_conflict_stat.csrc/spock_dependency.csrc/spock_executor.csrc/spock_failover_slots.csrc/spock_functions.csrc/spock_group.csrc/spock_node.csrc/spock_output_plugin.csrc/spock_proto_json.csrc/spock_proto_native.csrc/spock_readonly.csrc/spock_relcache.csrc/spock_repset.csrc/spock_rmgr.csrc/spock_shmem.csrc/spock_sync.csrc/spock_worker.c
💤 Files with no reviewable changes (1)
- include/spock_proto_native.h
✅ Files skipped from review due to trivial changes (27)
- src/spock_readonly.c
- src/compat/18/spock_compat.c
- include/spock.h
- src/spock_proto_json.c
- src/spock_proto_native.c
- include/spock_sync.h
- src/spock.c
- src/spock_repset.c
- include/spock_relcache.h
- include/spock_output_plugin.h
- include/spock_node.h
- src/spock_shmem.c
- src/spock_dependency.c
- src/spock_apply.c
- src/spock_executor.c
- src/spock_failover_slots.c
- src/spock_rmgr.c
- src/spock_conflict_stat.c
- src/spock_output_plugin.c
- src/spock_sync.c
- src/spock_relcache.c
- src/spock_group.c
- src/spock_apply_heap.c
- include/spock_group.h
- include/spock_worker.h
- src/spock_functions.c
- src/spock_worker.c
🚧 Files skipped from review as they are similar to previous changes (2)
- include/spock_conflict.h
- src/spock_conflict.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utils/pgindent/README.md (1)
59-62:⚠️ Potential issue | 🟡 MinorCI workflow reference is still stale; point this to
spockbench.ymland its pgindent step.This section still references
.github/workflows/pgindent.yml, but the PR objective says pgindent checks now run inside.github/workflows/spockbench.yml(with PG15 skipped in matrix). Please update wording so contributors look at the correct workflow.Proposed doc fix
-The GitHub Actions workflow `.github/workflows/pgindent.yml` runs -`./run-pgindent.sh --check` on every pull request to `main`. It builds -`pg_bsd_indent` from a pinned PG version (currently PG 18) and fetches -typedefs for all supported branches. +The GitHub Actions workflow `.github/workflows/spockbench.yml` includes a +`pgindent-check` step that runs `./run-pgindent.sh --check` on pull requests +to `main` (skipping the PG15 matrix entry). It builds `pg_bsd_indent` from a +pinned PG version (currently PG 18) and fetches typedefs for all supported +branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/pgindent/README.md` around lines 59 - 62, Update the README section that currently references ".github/workflows/pgindent.yml" to point to ".github/workflows/spockbench.yml" and explicitly mention the "pgindent" step within that workflow (which runs ./run-pgindent.sh --check), and note that the matrix skips PG15; keep the note about building pg_bsd_indent from a pinned PG version and fetching typedefs for supported branches but replace the stale workflow file name with "spockbench.yml" and the "pgindent" step identifier so contributors are directed to the correct CI job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@utils/pgindent/README.md`:
- Around line 59-62: Update the README section that currently references
".github/workflows/pgindent.yml" to point to ".github/workflows/spockbench.yml"
and explicitly mention the "pgindent" step within that workflow (which runs
./run-pgindent.sh --check), and note that the matrix skips PG15; keep the note
about building pg_bsd_indent from a pinned PG version and fetching typedefs for
supported branches but replace the stale workflow file name with
"spockbench.yml" and the "pgindent" step identifier so contributors are directed
to the correct CI job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c089edd-e66e-4d7e-94fe-b81215b8d7cf
📒 Files selected for processing (3)
include/spock_conflict.hsrc/spock_conflict.cutils/pgindent/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- include/spock_conflict.h
- src/spock_conflict.c
The gen-typedefs.sh script accumulated duplicates on each run because it only appended to typedefs.list. Fixed by: - Uncommenting curl to fetch fresh PostgreSQL typedefs (overwrites file) - Adding sort/uniq to remove any internal duplicates - Cleaning up typedefs.list Sorting allow us to stabilise the content in case a struct or enum has been displaced. This ensures repeated script runs produce consistent, deduplicated output.
…heck Replace the minimal gen-typedefs.sh with a full run-pgindent.sh script that fetches typedef lists from the PostgreSQL buildfarm for PG 16, 17, and 18, merges them with Spock-specific typedefs extracted from the source tree, and runs pgindent on all .c/.h files. PG 15 is excluded because pg_bsd_indent is absent from its source tree. The script supports a --check flag for CI dry-run mode and uses set -euo pipefail with fail-fast error handling on curl failures. It uses absolute paths so it works from any working directory. Add .github/workflows/pgindent.yml: a standalone pull-request workflow that builds pg_bsd_indent from PG 18 inside the existing Docker image and runs ./run-pgindent.sh --check. Rewrite utils/pgindent/README.md with full documentation covering quick start, prerequisites, cross-version typedef rationale, CI integration, and the files list. Name previously anonymous enums: - SpockResolveOption in include/spock_conflict.h - GroupProgressTupDescColumns in include/spock_group.h Regenerate typedefs.list from the merged PG 16-18 buildfarm output.
Improve pgindent infrastructure and code formatting
Cross-version typedef solution
Different PostgreSQL major versions define different structs and enums, causing
pgindent to produce different formatting depending on which PG version's typedefs
are used. This PR solves the problem by fetching typedef lists from the PostgreSQL
buildfarm for all supported branches (PG 16, 17, 18), merging them with
Spock-specific typedefs extracted from the source tree, and deduplicating into a
single
typedefs.list. PG 15 is excluded becausepg_bsd_indentis not presentin its source tree and its typedefs are a subset of PG 16's.
Changes
clean up artifacts (empty lines, comment fragments), and support
--checkmode for CI dry-run
pgindent-check.ymlworkflow.The formatting check now runs inside the existing
spockbench.ymlworkflowafter the docker build step, reusing the PG source tree already in the
container. Skipped for the PG 15 matrix entry
Spock source typedefs
SpockResolveOption,GroupProgressTupDescColumns), changespock_conflict_resolverfromintto
SpockResolveOption, remove duplicate extern declarations.cand.hfiles using theunified typedef list
gen-typedefs.shtorun-pgindent.sh, fixedduplicate typedef generation, updated README with cross-version documentation