NO-ISSUE: commitchecker: debug --ancestry-path discrepancy#114
NO-ISSUE: commitchecker: debug --ancestry-path discrepancy#114tmshort wants to merge 9 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tmshort: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Run the commit validation twice using a CheckMode type (AncestryPath, NoAncestryPath) to make the intent clear: - NoAncestryPath uses the CLI-provided start (e.g. PULL_BASE_SHA), catching PR commits even when the branch has fallen behind main — the case where --ancestry-path silently returns zero commits and produces a false-positive pass. - AncestryPath uses the computed upstream merge base, walking the correct linear carry path through the downstream repo. Both checks log any errors they find. Only AncestryPath errors cause a non-zero exit; to also fail on NoAncestryPath errors, add commitchecker.NoAncestryPath: true to failingChecks. Signed-off-by: Todd Short <tshort@redhat.com> Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
tmshort
force-pushed
the
debug-ancestry-path
branch
from
355b2cf to
9c2390f
Compare
|
/cc @jacobsee Thanks for digging into this. It sounds like the same problem I ran into last year in openshift/release#67720 (comment). We will definitely want to fix it across all repos with a verify-commits presubmit. Here it is breaking on openshift/kubernetes: openshift/kubernetes#2368 (comment). |
|
Here is our failing false positive PR: |
|
This should not change behavior, it is mostly adding debugging, so perhaps we can get it in? |
|
Would it be simpler to consider it a failure when start and end are different and there are zero commits along the path? |
I could also add an error for that. |
If CommitsBetween returns zero commits but start and end resolve to different SHAs, the range is effectively empty due to filtering (e.g. --ancestry-path silently dropping all PR commits). Treat this as an error rather than a silent pass. Adds SameCommit() and ErrNoCommitsFound to the package to support the check. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <todd.short@me.com>
|
And there it is |
| } | ||
|
|
||
| var ErrNotCommit = fmt.Errorf("one or both of the provided commits was not a valid commit") | ||
| var ErrNoCommitsFound = fmt.Errorf("no commits found between start and end, but they resolve to different commits") |
The pattern `commitchecker` matches any file starting with that prefix, including test files like commits_test.go in subdirectories. Use `/commitchecker` to only match the built binary at the directory root.
The `if err != nil` check was always false because `err` was declared as `var err error` and never assigned before the check.
…ectCommitsBetween Split CommitsBetween (which always used --ancestry-path) into two explicit functions: - AllCommitsBetween: finds all non-merge commits reachable from end but not start - DirectCommitsBetween: same, but restricted to the direct ancestry path via --ancestry-path Both call a shared private gitLog() helper. The caller in main() is updated to call each explicitly instead of looping over CheckMode values, with shared error handling extracted into listCommits() and validation into validateCommits(). The error message for git log failures now includes the full command for easier copy-paste debugging.
When all reachable commits are found but none are on the direct ancestry path, fail with an error telling the user to rebase. This prevents false positive passes when a PR branch has fallen behind main. Also removes SameCommit, ErrNoCommitsFound, and CheckMode which are no longer used after the refactor.
TestStaleBranch creates a DAG where a PR branch forks from an early commit, main advances, then the PR is merged. Verifies that AllCommitsBetween finds the PR commits but DirectCommitsBetween does not, confirming the --ancestry-path behavior. TestUpToDateBranch verifies both functions return the same commits on a linear history.
Add a Limitations section to the README explaining stale-branch detection, the error message users will see, how to read CI logs, and how to fix it (rebase the PR). Add ancestry-path.md with a detailed explanation of --ancestry-path git semantics, why the commitchecker needs it, and how it can drop commits on stale branches.
|
After spending some time understanding the PR and the Key changes:
The PR is split into 6 commits to make it easier to review incrementally. |
commitchecker: refactor ancestry-path handling and add stale-branch detection
|
Thanks @sanchezl. I actually reviewed the changes against the I think is is ready for final review. |
|
@tmshort: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
To save review time, could you explain what scenario this fixes that isn't also fixed by starting from the merge base of the pull request ref and the base ref? |
|
Unfortunately, we just had another failure of the commit checker, and our downstream bumper is broken. @benluddy wrote:
According to claude: The merge-base approach would also fix the false positive, but has a failure mode in the rebase scenario that Stale branch + merge-base:
Even adding The merge-base approach would give the right answer if we used the PR head (G = |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86, grokspawn, jianzhangbjz, tmshort The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@tmshort please provide explicit links to the failing PRs so we can verify our understanding. @benluddy and I spent time last week investigating this. Here's what we found:
With (1) and (3), a single pass with If the failures you're seeing are in the operator-framework repos that already have a |
|
This is an open test PR that is failing: This a PR that unfortunately received a false positive and was merged: |
|
@sanchezl |
|
@sanchezl |
7 participants
Run git log both with and without --ancestry-path. If the outputs differ, print a warning showing both sets of commits so it is visible in CI logs when --ancestry-path is silently dropping PR commits.
This can happen when a PR branch has fallen behind main: the PR commits are not descendants of PULL_BASE_SHA, so --ancestry-path excludes them and the checker validates zero commits (false positive pass).
Assisted-by: Claude Sonnet 4.6 noreply@anthropic.com