add format-check cirrus workflow and pre-commit hooks.#903
add format-check cirrus workflow and pre-commit hooks.#903gaoxueyu merged 1 commit intoIvorySQL:masterfrom
Conversation
WalkthroughAdds PR-only CI formatting checks via Cirrus, introduces a pre-commit hook to auto-format staged C/C++ files with pgindent, provides a helper script and Makefile target to enable hooks, and updates English and Chinese docs describing the formatting workflow. No runtime code paths or public APIs are changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Git as Git (pre-commit)
participant Hook as Pre-commit Hook
participant Build as Build (Meson/Ninja)
participant Indent as pg_bsd_indent
participant PGI as pgindent
participant Repo as Repo Index
Dev->>Git: git commit
Git->>Hook: invoke .githooks/pre-commit
alt PG_NO_FORMAT=1
Hook-->>Git: exit 0 (skip)
else Format C/C++
Hook->>Build: ensure pg_bsd_indent built
Build->>Indent: build/check executable
Hook->>PGI: format staged C/C++ (PGINDENT/PGTYPEDEFS)
PGI-->>Hook: exit code
Hook->>Repo: git add re-formatted files
Hook->>PGI: pgindent --check on staged files
alt Unformatted remains
Hook-->>Git: exit non-zero (block commit)
else Clean
Hook-->>Git: exit 0 (allow commit)
end
end
sequenceDiagram
autonumber
actor PR as PR Event
participant Cirrus as Cirrus CI
participant Ctn as Debian Container
participant Build as Meson/Ninja
participant Indent as pg_bsd_indent
participant PGI as pgindent
participant Git as Git (BASE..HEAD)
PR->>Cirrus: triggers PR-only workflow
Cirrus->>Ctn: start container, install deps
Ctn->>Build: configure minimal Meson
Build->>Indent: build pg_bsd_indent
Ctn->>Git: compute changed .c/.h/.cpp/.hpp (exclude formatter dir)
alt No relevant changes
Ctn-->>Cirrus: skip job (Passed)
else Files present
Ctn->>PGI: pgindent --check on changed files
alt Differences found
Ctn->>PGI: pgindent --diff (emit patch)
Ctn-->>Cirrus: fail with non-zero
else Clean
Ctn-->>Cirrus: print "Passed"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/enable-git-hooks.sh (1)
27-28: Quote thenprocexpansion to keep ShellCheck happy.ShellCheck is right that the command substitution should be quoted so the
-jflag always receives a single argument. It also silences SC2046 for future runs.- if ! make -C src/tools/pg_bsd_indent -j$(nproc 2>/dev/null || echo 2) >/dev/null; then + if ! make -C src/tools/pg_bsd_indent -j"$(nproc 2>/dev/null || echo 2)" >/dev/null; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.cirrus.format.yml(1 hunks).cirrus.star(1 hunks).githooks/pre-commit(1 hunks)CONTRIBUTING.md(1 hunks)Makefile(1 hunks)README_CN.md(1 hunks)tools/enable-git-hooks.sh(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 48-48: Missing required phony target "all"
(minphony)
[warning] 48-48: Missing required phony target "clean"
(minphony)
[warning] 48-48: Missing required phony target "test"
(minphony)
🪛 Shellcheck (0.11.0)
tools/enable-git-hooks.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: meson_build (ubuntu-latest)
| # output += config_from(".cirrus.tasks.yml") | ||
|
|
||
| # PR-only format check | ||
| output += config_from(".cirrus.format.yml") |
There was a problem hiding this comment.
Re-include .cirrus.tasks.yml or the rest of CI stops running.
By commenting out config_from(".cirrus.tasks.yml") you no longer append the repository’s main Cirrus task matrix (Linux/Windows/BSD/macOS builds, tests, etc.). Those hundreds of lines in .cirrus.tasks.yml now never get loaded, so every push/PR loses its full regression coverage.(github.com)
Please keep the existing tasks and add the new format check alongside them, e.g.:
- # output += config_from(".cirrus.tasks.yml")
-
- # PR-only format check
- output += config_from(".cirrus.format.yml")
+ output += config_from(".cirrus.tasks.yml")
+
+ # PR-only format check
+ output += config_from(".cirrus.format.yml")Otherwise the formatting job becomes the only Cirrus workload, which is a severe regression in our CI gate.
📝 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.
| # output += config_from(".cirrus.tasks.yml") | |
| # PR-only format check | |
| output += config_from(".cirrus.format.yml") | |
| output += config_from(".cirrus.tasks.yml") | |
| # PR-only format check | |
| output += config_from(".cirrus.format.yml") |
🤖 Prompt for AI Agents
.cirrus.star around lines 42 to 45: restore the call to
config_from(".cirrus.tasks.yml") (remove the comment) so the repository’s main
task matrix is loaded, and then append the PR-only format check by keeping
output += config_from(".cirrus.format.yml") so both the full CI tasks and the
new formatting job run; ensure the tasks file is loaded before or alongside the
format config and do not replace the tasks call with the format call.
| mapfile -t files < <(\ | ||
| git diff --cached --name-only --diff-filter=ACMR | \ | ||
| grep -E '\.(c|h|cpp|hpp)$' | \ | ||
| grep -vE '^src/tools/pg_bsd_indent/' || true) |
There was a problem hiding this comment.
Restore Bash 3.2 compatibility so the hook runs on macOS.
mapfile/readarray only exist in Bash 4+, but macOS still ships /bin/bash 3.2.57 by default. With your shebang resolving to that shell, the hook aborts entirely for every macOS contributor before any formatting happens.(dongtienvietnam.com)
Please switch to a portable read loop instead of mapfile, e.g.:
-mapfile -t files < <(\
- git diff --cached --name-only --diff-filter=ACMR | \
- grep -E '\.(c|h|cpp|hpp)$' | \
- grep -vE '^src/tools/pg_bsd_indent/' || true)
+files=()
+while IFS= read -r file; do
+ files+=("$file")
+done < <(git diff --cached --name-only --diff-filter=ACMR |
+ grep -E '\.(c|h|cpp|hpp)$' |
+ grep -vE '^src/tools/pg_bsd_indent/' || true)The rest of the script can stay the same, since ${#files[@]} and the later array expansions still work. That keeps the hook functional for the whole team without forcing everyone to install an alternate Bash.
🤖 Prompt for AI Agents
.githooks/pre-commit around lines 15-18: the script uses mapfile which requires
Bash 4+, breaking on macOS's default Bash 3.2; replace the mapfile usage with a
portable read loop that collects the git-diff output into the files array—run
the git diff | grep pipeline inside a command substitution or heredoc and
iterate each line with while IFS= read -r file; do files+=("$file"); done so the
array is populated in the current shell; keep the existing filters (grep -E for
extensions and grep -vE for excluded path) and the trailing || true logic
intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tools/enable-git-hooks.sh (2)
27-29: Quotenprocto avoid word splitting; address ShellCheck SC2046Unquoted command substitution can split if
nprocprints unexpected whitespace. Quote it and slightly polish the warning text.Apply:
- if ! make -C src/tools/pg_bsd_indent -j$(nproc 2>/dev/null || echo 2) >/dev/null; then - echo "[enable-git-hooks] Warning: build failed. you can build manually: make -C src/tools/pg_bsd_indent pg_bsd_indent" + if ! make -C src/tools/pg_bsd_indent -j"$(nproc 2>/dev/null || echo 2)" >/dev/null; then + echo "[enable-git-hooks] Warning: build failed. You can build manually: make -C src/tools/pg_bsd_indent pg_bsd_indent" fiBased on static analysis hint (SC2046).
18-21: Optional: add non-symlink fallback for Windows devs
ln -soften fails on Windows without admin/dev mode. Consider copying as a fallback so tools checking.git/hooksstill work.Example:
- ln -s ../.githooks/pre-commit .git/hooks/pre-commit || true + ln -s ../.githooks/pre-commit .git/hooks/pre-commit 2>/dev/null || \ + cp .githooks/pre-commit .git/hooks/pre-commit || trueCONTRIBUTING.md (1)
25-29: Doc nits: mention skip env and scope detailsTwo small additions improve developer experience.
- Note the escape hatch environment variable, if supported by the hook: e.g., “Set PG_NO_FORMAT=1 to bypass formatting for a commit.”
- Clarify file scope: “The hook and CI target C/C++ sources (e.g., .c/.h/.cpp/.hpp); tool sources under src/tools/pg_bsd_indent/ are excluded.”
I can propose a patch if you confirm the hook supports PG_NO_FORMAT as per README.
.cirrus.format.yml (1)
48-50: Quote exported paths for safetyGuard against spaces or special chars in working directory paths.
-export PGTYPEDEFS=$(pwd)/src/tools/pgindent/typedefs.list -export PGINDENT=$(pwd)/build/src/tools/pg_bsd_indent/pg_bsd_indent +export PGTYPEDEFS="$(pwd)/src/tools/pgindent/typedefs.list" +export PGINDENT="$(pwd)/build/src/tools/pg_bsd_indent/pg_bsd_indent"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.cirrus.format.yml(1 hunks).cirrus.star(1 hunks).githooks/pre-commit(1 hunks)CONTRIBUTING.md(1 hunks)Makefile(1 hunks)README.md(1 hunks)README_CN.md(1 hunks)tools/enable-git-hooks.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
tools/enable-git-hooks.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
🪛 checkmake (0.2.2)
Makefile
[warning] 48-48: Missing required phony target "all"
(minphony)
[warning] 48-48: Missing required phony target "clean"
(minphony)
[warning] 48-48: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
🔇 Additional comments (2)
.githooks/pre-commit (1)
15-18: Fix Bash 3.2 incompatibility in staged-file collection.Line 15 still uses
mapfile, which Bash 3.2 (macOS default) does not provide, so the hook aborts before formatting runs. Replace it with a portable read loop so everyone can commit without installing a newer shell.-mapfile -t files < <(\ - git diff --cached --name-only --diff-filter=ACMR | \ - grep -E '\.(c|h|cpp|hpp)$' | \ - grep -vE '^src/tools/pg_bsd_indent/' || true) +files=() +while IFS= read -r file; do + files+=("$file") +done < <(git diff --cached --name-only --diff-filter=ACMR | + grep -E '\.(c|h|cpp|hpp)$' | + grep -vE '^src/tools/pg_bsd_indent/' || true).cirrus.star (1)
42-45: Restore inclusion of the main Cirrus task matrix.Line 42 comments out
config_from(".cirrus.tasks.yml"), so the entire build/test matrix stops loading and only the new format job runs—critical regression for every PR/push. Keep the tasks inclusion and append the format check afterward.- # output += config_from(".cirrus.tasks.yml") - - # PR-only format check - output += config_from(".cirrus.format.yml") + output += config_from(".cirrus.tasks.yml") + + # PR-only format check + output += config_from(".cirrus.format.yml")
| echo "[FormatCheck] Running pgindent --check..." | ||
| set +e | ||
| ./src/tools/pgindent/pgindent --check $files | ||
| rc=$? | ||
| set -e | ||
| if [ $rc -ne 0 ]; then | ||
| echo "[FormatCheck] Unformatted changes detected; suggested patch below:" | ||
| ./src/tools/pgindent/pgindent --diff $files || true | ||
| exit $rc | ||
| fi |
There was a problem hiding this comment.
Avoid unquoted $files; use newline-delimited xargs for correctness
Unquoted $files is vulnerable to word splitting/globbing; filenames with spaces will break both --check and --diff. Switch to a newline-delimited xargs flow.
Apply:
- set +e
- ./src/tools/pgindent/pgindent --check $files
- rc=$?
- set -e
- if [ $rc -ne 0 ]; then
- echo "[FormatCheck] Unformatted changes detected; suggested patch below:"
- ./src/tools/pgindent/pgindent --diff $files || true
- exit $rc
- fi
+ # Run checks safely with newline-delimited xargs (handles spaces in paths).
+ set +e
+ printf '%s\n' "$files" | xargs -r -d '\n' ./src/tools/pgindent/pgindent --check
+ rc=$?
+ set -e
+ if [ $rc -ne 0 ]; then
+ echo "[FormatCheck] Unformatted changes detected; suggested patch below:"
+ printf '%s\n' "$files" | xargs -r -d '\n' ./src/tools/pgindent/pgindent --diff || true
+ exit $rc
+ fiOptionally extend extensions to cover .cc/.cxx/.hh/.hxx if the tree uses them.
📝 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.
| echo "[FormatCheck] Running pgindent --check..." | |
| set +e | |
| ./src/tools/pgindent/pgindent --check $files | |
| rc=$? | |
| set -e | |
| if [ $rc -ne 0 ]; then | |
| echo "[FormatCheck] Unformatted changes detected; suggested patch below:" | |
| ./src/tools/pgindent/pgindent --diff $files || true | |
| exit $rc | |
| fi | |
| echo "[FormatCheck] Running pgindent --check..." | |
| # Run checks safely with newline-delimited xargs (handles spaces in paths). | |
| set +e | |
| printf '%s\n' "$files" | xargs -r -d '\n' ./src/tools/pgindent/pgindent --check | |
| rc=$? | |
| set -e | |
| if [ $rc -ne 0 ]; then | |
| echo "[FormatCheck] Unformatted changes detected; suggested patch below:" | |
| printf '%s\n' "$files" | xargs -r -d '\n' ./src/tools/pgindent/pgindent --diff || true | |
| exit $rc | |
| fi |
🤖 Prompt for AI Agents
.cirrus.format.yml lines 54-63: the script uses unquoted $files which allows
word-splitting/globbing and breaks on filenames with spaces; change the
invocation to feed a newline- or NUL-delimited list to xargs and call pgindent
once per file (do this for both the --check and --diff invocations). Concretely,
emit files in a safe delimiter form (e.g. printf '%s\n' "$files" or better,
produce NUL-delimited output) and pipe into xargs (or xargs -0) to run
./src/tools/pgindent/pgindent --check and ./src/tools/pgindent/pgindent --diff
so filenames with spaces are preserved; also apply the same change to the diff
path and optionally extend the file-extension list to include .cc/.cxx/.hh/.hxx
if those are used in the repo.
Summary by CodeRabbit
New Features
Chores
Documentation