Skip to content

fix: accept destination dev-branch ancestors in integration history check (PTFE-3343)#271

Open
delthas wants to merge 2 commits into
mainfrom
bugfix/PTFE-3343/history-mismatch-nondiverged-dev
Open

fix: accept destination dev-branch ancestors in integration history check (PTFE-3343)#271
delthas wants to merge 2 commits into
mainfrom
bugfix/PTFE-3343/history-mismatch-nondiverged-dev

Conversation

@delthas

@delthas delthas commented Jun 22, 2026

Copy link
Copy Markdown

Problem

Bert-E raised a false History mismatch (BranchHistoryMismatch, code 113) — telling the user to reset even though nothing was rebased — when an integration branch sat on a development branch that had not diverged from its parent (development/X.Y freshly branched off development/X.(Y-1), still pointing at the same commit D) and that integration branch carried a real merge commit.

When it happens

The trigger is a real (non-fast-forward) merge commit on w/X.Y whose first parent is the shared dev tip D. Two ways to get there:

  • Automated cascade, "late" PR — when the PR's feature branch was branched before D (the PR is behind the dev tip), the auto-created w/X.Y merge cannot fast-forward (feature and D are siblings), so Bert-E produces a real merge commit whose first parent is D. PRs are frequently behind the dev tip, so this is the common case. (Thanks @tcarmet for spotting this path.)
  • Manual resolution — an operator hand-resolves w/X.Y with a forced merge commit (e.g. git merge -s ours) so the higher development branch ignores the cascaded change (the ARSN-600 scenario).

It does not trigger when the PR is already up-to-date with the dev tip: the merge then fast-forwards and no merge commit is created.

Root cause

update_integration_branches() builds acceptable_parents from three git log A..B diffs (prev_set, dst_set, wbranch_set). A..B excludes the boundary commit, so when development/X.Y == development/X.(Y-1), dst_set is empty and the shared dev tip D — the first parent of the integration merge — appears in none of the sets. The check then rejects a parent that legitimately belongs to the target development branch, contradicting the check's own documented intent (its comment explicitly allows parents from "the target development branch").

Fix

Accept a merge parent that is an ancestor of the destination development branch (Branch.includes_commit, i.e. git merge-base --is-ancestor), matching the documented intent.

Genuine rebase / force-push detection is preserved: a rebased source leaves a merge whose offending parent is a feature-branch commit that is not an ancestor of any development branch, so it still raises. The existing BranchHistoryMismatch tests still pass. Two failing-first regression tests cover the non-diverged case:

  • test_history_mismatch_nondiverged_dev_branch — manual-resolution path
  • test_history_mismatch_nondiverged_dev_auto_integration — auto-created merge of a late PR

Full tox -e tests-noqueue and tox -e tests suites pass (202 tests each).

Jira: PTFE-3343

…heck (PTFE-3343)

When a development branch has not diverged from its parent (it points at
the same commit), a manually-resolved integration branch carrying a forced
merge commit tripped a false BranchHistoryMismatch. The merge's first parent
is the shared development-branch tip, which the `git log A..B` diffs used to
build acceptable_parents exclude (the boundary commit is dropped), so the
check rejected a parent that legitimately belongs to the target development
branch.

Accept a merge parent that is an ancestor of the destination development
branch (Branch.includes_commit), matching the check's own documented intent.
Genuine rebase / force-push detection is preserved.
@delthas delthas requested a review from a team as a code owner June 22, 2026 14:43
@delthas delthas requested review from a team, DarkIsDude and benzekrimaha June 22, 2026 14:46
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.62%. Comparing base (9d05418) to head (a16808b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   89.60%   89.62%   +0.02%     
==========================================
  Files          81       81              
  Lines       10589    10614      +25     
==========================================
+ Hits         9488     9513      +25     
  Misses       1101     1101              
Flag Coverage Δ
integration 87.23% <100.00%> (+0.03%) ⬆️
tests 87.19% <100.00%> (+0.03%) ⬆️
tests-BuildFailedTest 26.62% <7.69%> (-0.06%) ⬇️
tests-QuickTest 34.14% <7.69%> (-0.08%) ⬇️
tests-RepositoryTests 26.28% <7.69%> (-0.06%) ⬇️
tests-TaskQueueTests 51.34% <11.53%> (-0.14%) ⬇️
tests-TestBertE 65.50% <100.00%> (+0.11%) ⬆️
tests-TestQueueing 53.56% <11.53%> (-0.14%) ⬇️
tests-api-mock 15.33% <0.00%> (-0.04%) ⬇️
tests-noqueue 77.42% <100.00%> (+0.06%) ⬆️
tests-noqueue-BuildFailedTest 26.62% <7.69%> (-0.06%) ⬇️
tests-noqueue-QuickTest 34.14% <7.69%> (-0.08%) ⬇️
tests-noqueue-RepositoryTests 26.28% <7.69%> (-0.06%) ⬇️
tests-noqueue-TaskQueueTests 51.34% <11.53%> (-0.14%) ⬇️
tests-noqueue-TestBertE 61.87% <100.00%> (+0.12%) ⬆️
tests-noqueue-TestQueueing 26.31% <7.69%> (-0.06%) ⬇️
tests-server 28.24% <0.00%> (-0.07%) ⬇️
unittests 43.34% <0.00%> (-0.11%) ⬇️
utests 28.10% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DarkIsDude DarkIsDude removed their request for review June 23, 2026 07:52
@francoisferrand francoisferrand requested a review from tcarmet June 23, 2026 09:21

@tcarmet tcarmet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work! Please add the extra testcase :)

Comment thread bert_e/tests/test_bert_e.py
…eck (PTFE-3343)

When the destination development branch has not diverged from its parent
and the PR's feature branch is behind the shared tip, bert-e auto-creates the
integration branch via a real (non-fast-forward) merge whose first parent is
the shared dev tip -- the same condition the manual-resolution test covers,
but reached through the fully-automated path. Added per review feedback from
tcarmet.
for rev in wbranch_set:
if not all(p in acceptable_parents for p in rev.parents):
if not all(p in acceptable_parents or
wbranch.dst_branch.includes_commit(p)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why includes_commit rather than just adding the dev tip to dst_set (the boundary that .. drops)? I think includes_commit is the better choice , just confirming it's deliberate and that wbranch_set stays small enough that a per-parent merge-base is a non-issue.

Comment on lines +3787 to +3789
self.gitrepo.cmd('git checkout development/4.3')
self.gitrepo.cmd('git checkout -b development/4.4')
self.gitrepo.cmd('git push -u origin development/4.4')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could pull that into a tiny helper (e.g. branch_nondiverged_dev('4.3', '4.4')) so the two tests only differ on the part that matters (manual merge vs late-PR auto-merge). Not blocking tho

@benzekrimaha benzekrimaha left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM , my comments are not blocking just a nit and a confirmation question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants