⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@sensei-hacker
Copy link
Member

@sensei-hacker sensei-hacker commented Jan 22, 2026

User description

Summary

Fixes a bug in the PG version check workflow where struct modifications in header files weren't detected when PG_REGISTER is in the corresponding .c file.

Problem

PR #11276 removed a field from blackboxConfig_s struct without incrementing the PG version, but the workflow didn't catch it because:

  • PG_REGISTER for blackboxConfig_t is in src/main/blackbox/blackbox.c
  • The struct typedef blackboxConfig_s is defined in src/main/blackbox/blackbox.h
  • The script only checked the diff of blackbox.c for struct changes, missing the header file

Solution

When checking a file with PG_REGISTER:

  1. First look for struct definition in that file's diff
  2. If not found, check the corresponding .h/.c companion file's diff
  3. Detect struct changes in either location

Testing

The fix will correctly detect the issue in PR #11276 once it's re-run.

Related


PR Type

Bug fix


Description

This description is generated by an AI tool. It may have inaccuracies

  • Fixes PG version check to detect struct changes in companion header files

  • Script now checks corresponding .h file when struct typedef is separate from PG_REGISTER location

  • Resolves false negatives where struct field modifications in headers were missed

  • Enables proper version validation for structs split across .c and .h files


Diagram Walkthrough

flowchart LR
  A["PG_REGISTER in .c file"] --> B["Check struct in same .c file"]
  B --> C{"Struct found?"}
  C -->|Yes| D["Validate struct changes"]
  C -->|No| E["Check companion .h file"]
  E --> F["Validate struct changes"]
  D --> G["Report version issues"]
  F --> G
Loading

File Walkthrough

Relevant files
Bug fix
check-pg-versions.sh
Add companion file struct checking logic                                 

.github/scripts/check-pg-versions.sh

  • Added logic to check companion .h/.c files when struct typedef is not
    found in the primary file
  • Detects struct changes in either the .c file with PG_REGISTER or its
    corresponding header file
  • Validates that companion file exists in the diff before attempting to
    parse it
  • Extracts struct body from companion file diff using same pattern
    matching as primary file
+17/-0   

The script now checks the corresponding .h file when PG_REGISTER is in
a .c file but the struct typedef is in the header. This fixes false
negatives where struct changes in headers weren't detected.

Example: blackboxConfig_t is registered in blackbox.c but the struct
typedef blackboxConfig_s is defined in blackbox.h. Previous version
only checked blackbox.c diff and missed the struct field removal.
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level Suggestion

The current method of parsing C code with sed and grep is fragile. A better long-term solution is to use a compiler-based tool to analyze the code's Abstract Syntax Tree (AST), which would provide a more reliable way to detect struct changes. [High-level, importance: 9]

Solution Walkthrough:

Before:

# In check-pg-versions.sh
struct_pattern="typedef struct ${struct_type%_t}_s"

# Get diff for current file
diff_output=$(git diff ...)

# Try to find struct definition using sed
struct_body_diff=$(echo "$diff_output" | sed -n "/${struct_pattern}/, /.../p")

# If not found, try companion file
if [ -z "$struct_body_diff" ]; then
    companion_diff=$(git diff ... -- "$companion_file")
    struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/, /.../p")
fi

# Find changes in the text-based diff
struct_changes=$(echo "$struct_body_diff" | grep -E "^[-+]" | grep -v ...)

After:

# Conceptual script using a C parser (e.g., libclang)
import clang.cindex

def get_struct_signature(file_content, struct_name):
    # Parse code into an Abstract Syntax Tree (AST)
    idx = clang.cindex.Index.create()
    tu = idx.parse('tmp.c', unsaved_files=[('tmp.c', file_content)])

    # Find struct in AST and generate a signature from its members
    # (This is far more robust than text matching)
    ...
    return signature

# Get file content from base and head commits
base_content = git.show(f"{BASE_COMMIT}:{file}")
head_content = git.show(f"{HEAD_COMMIT}:{file}")

# Compare signatures
base_sig = get_struct_signature(base_content, "my_struct_t")
head_sig = get_struct_signature(head_content, "my_struct_t")

if base_sig != head_sig:
    print("Struct has changed!")

Comment on lines +110 to +115
if [ -n "$companion_file" ] && git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -q "^${companion_file}$"; then
echo " 🔍 Struct not in $file, checking $companion_file"
local companion_diff=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$companion_file")
struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p")
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: To improve performance, avoid repeated git diff calls inside the loop by getting all diffs once at the start of the script and storing them in an associative array for later lookup. [general, importance: 6]

Suggested change
if [ -n "$companion_file" ] && git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -q "^${companion_file}$"; then
echo " 🔍 Struct not in $file, checking $companion_file"
local companion_diff=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$companion_file")
struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p")
fi
fi
# This suggestion requires changes outside the visible diff.
# At the start of the script, you would add:
# declare -A all_diffs
# while IFS= read -r changed_file; do
# all_diffs["$changed_file"]=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$changed_file")
# done < <(git diff --name-only $BASE_COMMIT..$HEAD_COMMIT)
# Then, the existing code block would be changed to:
if [ -n "$companion_file" ] && [ -v "all_diffs[$companion_file]" ]; then
echo " 🔍 Struct not in $file, checking $companion_file"
local companion_diff=${all_diffs[$companion_file]}
struct_body_diff=$(echo "$companion_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p")
fi

@sensei-hacker sensei-hacker merged commit 9a9033d into maintenance-9.x Jan 22, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants