[Changelog] Make the fragment check merge-aware in pre-commit - #7528
[Changelog] Make the fragment check merge-aware in pre-commit#7528hujc7 wants to merge 1 commit into
Conversation
With --include-worktree the check diffs the worktree against the merge-base of origin/<base> and HEAD. While a merge of the base branch is pending, HEAD is still the pre-merge tip, so the base's own commits since the fork point sit in the worktree and are reported as this branch's changes. The scheduled changelog-compile commits delete fragments, so every develop merge after one of them fails the immutability rule and reports the compiled packages as missing a fragment. When MERGE_HEAD exists, measure from the newer of the two merge-bases (HEAD's and MERGE_HEAD's), which for a develop merge is develop's tip. The three-dot diff used in CI is unchanged.
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The merge-aware diff-base selection narrowly fixes pending base-branch merges while leaving the CI three-dot diff path unchanged. One error-handling regression should be corrected before merge.
- Design and architecture: Using
MERGE_HEADonly for pending merges and advancing the diff base only when the candidate is newer is a well-scoped design that preserves the existingPRDiffstructure and committed-diff behavior. - API:
PRDiff.from_gitretains its signature and return semantics. The intended behavior change is limited to selecting the local worktree diff target during a pending merge. - Implementation: The optional Git probes appropriately use return codes, but the initial mandatory
merge-baselookup now suppresses failure and can passNoneinto subsequent subprocess argument lists, replacing a useful Git failure with an opaqueTypeError. Keep that lookup checked while retaining lenient handling for optional merge-state probes.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| result = subprocess.run(["git", *args], capture_output=True, text=True, cwd=REPO_ROOT) | ||
| return result.stdout.strip() if result.returncode == 0 else None | ||
|
|
||
| diff_target = _git("merge-base", remote_base, "HEAD") |
There was a problem hiding this comment.
🟡 Warning · Implementation — Required merge-base failure now yields None
The replaced call used check=True, so a missing origin/<base> ref or unrelated histories raised CalledProcessError carrying git's message. _git swallows non-zero exit codes, so diff_target can become None and is passed straight into the git diff argument list (and into --is-ancestor), producing an opaque TypeError: expected str ... not NoneType. Keep this required lookup checked and use the lenient helper only for the optional MERGE_HEAD probes.
Greptile SummaryThe PR makes local changelog-fragment checks aware of pending merges by selecting a newer applicable merge base when
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking regression in how invalid or unavailable base refs are reported by the local hook. The intended pending-merge behavior is covered, but suppressing the initial merge-base error allows None to reach subprocess.run and replace the existing handled error with a traceback. Files Needing Attention: tools/changelog/cli.py Important Files Changed
Reviews (1): Last reviewed commit: "Make the changelog fragment check merge-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| result = subprocess.run(["git", *args], capture_output=True, text=True, cwd=REPO_ROOT) | ||
| return result.stdout.strip() if result.returncode == 0 else None | ||
|
|
||
| diff_target = _git("merge-base", remote_base, "HEAD") |
There was a problem hiding this comment.
When origin/develop is unavailable or has no common ancestor with HEAD, _git returns None, which is passed to subprocess.run and produces an unhandled TypeError instead of the existing concise Git error. Keep this initial query throwing, or explicitly handle its failure before using diff_target.
1 participant
Summary
Merging develop into a branch no longer fails the local
check changelog fragmentshook whenever develop has compiled fragments since the fork point. The hook now measures a pending merge from the base branch's tip instead of the old fork point.Description
tools/changelog/cli.py,PRDiff.from_git(include_worktree=True), diffed the worktree againstmerge-base(origin/<base>, HEAD). Duringgit merge developHEAD is still the pre-merge tip, so develop's commits since the fork point are in the worktree but not in HEAD and read as branch changes; the scheduled compile commits delete fragments, which trips the immutability rule and marks the compiled packages as missing a fragment (observed on #7161: 7 false immutability hits, 7 false missing packages, none genuine). WhenMERGE_HEADexists, the diff base is now the newer of the two merge-bases. The three-dot diff CI uses is unchanged.Fixes # (none)
Type of change
Release backport
developTest plan
python -m pytest tools/changelog/test/92 passed; the new test fails on develop and passes hereChecklist
pre-commitchecks with./isaaclab.sh --formatCONTRIBUTORS.mdor my name already exists there