#35686 lint: have git-subtree-check check for backportability
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35686 lint: have git-subtree-check check for backportability | Ready | 0 (+2) -1 | Disputed | 10 + 66 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Updates test/lint/git-subtree-check.sh to check that the most recent subtree merge commit is stacked directly on the previous subtree merge commit. Adds an --incompatible flag to bypass this requirement when API changes require a newer base commit, and updates repo hygiene linting and developer documentation accordingly.
Problem: When subtree updates are based on newer commits on master rather than the previous subtree merge, the resulting merge commit cannot be reused verbatim when backporting to maintenance branches, forcing reviewers to inspect and verify a newly generated subtree merge for every release branch.
Evidence of importance:
- PR description: 'The exact same merge commit can then be reused on release branches, making the backport trivial to verify without rereviewing a newly generated subtree merge.'
Risk of a mistake: low (tooling, docs). Only touches repository linting scripts and documentation; a bug would only cause local subtree lint checks to report false positives or negatives.
Changed since the previous assessment: No substantial changes to the approach or diff; refactors command-line argument parsing and documents the practice in developer notes.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: deciding whether to proceed or adjust approach following maflcko's Approach NACK and sedited's feedback 2026-09-30
- author: author to decide how to proceed following maflcko's Approach NACK and sedited's comment on default flags 2026-09-30
Agreement
Disputed
objection that needs a change, the objector pushed back after a reply (maflcko)
Thread notes: Re-read conversion: changed r:4790659090 from objection to suggestion since ryanofsky approved the PR while suggesting improvements; added questions from c:4936395031 and c:4936824044; added suggestion rc:4073574189 which was addressed. Tracked maflcko's standing Approach NACK through c:5905591607. / Added questions from sedited (c:4936395031) and maflcko (c:4936824044 part 2) and documentation suggestion from maflcko (rc:4073574189). maflcko reinforced his Approach NACK and moved toward a Conce
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | question, usefulness | asks whether this solves a real problem given that commits from subtrees are sometimes cherry-picked when fixes need backporting | answered · ryanofsky explained the PR addresses wider subtree bumps rather than isolated cherry-picks | answered | by others | 2026-07-10: 'We sometimes cherry-pick commits from the subtree if they contain fixes that need backporting. Is this really solving a problem?' Status from: 2026-08-21: ryanofsky: 'This PR is primarily trying to address a different use-case, when there are wider improvements like stability fixes in the subtree that you want to backport to multiple release branches' |
| maflcko (member) | objection, correctness | warning on merge commits that adjust code could encourage build failures or warn unnecessarily when adjustments are required | fixed · fix pushed · author added --incompatible option to opt out of the check | changed | yes | 2026-07-10: 'In some rare cases it is required to adjust the code for the subtree bump in the merge commit itself (to avoid build failures on the merge commit itself), so that would warn in that case and somehow encourage build failures?' Status from: 2026-07-16: maflcko: 'I did that in the last update commit... This way, each commit can opt-in or out, and enforce it. But :man_shrugging:' |
| maflcko (member) | question, approach | how does merging the same subtree commit as-is simplify backports compared to creating a second similar merge commit on the backport branch? | answered · author explained that reusing the exact same merge commit hash makes backports trivial to review | answered | yes | 2026-07-10: 'can you explain how this simplifies "backport"? IIUC you are referring to merging the subtree merge commit as-is into several branches?' Status from: 2026-07-10: Sjors: 'Yes, we've done this a few times for libmultiprocess: #34804 and #34952 have the exact same merge commit hash, making the pack-port trivial to review.' |
| ryanofsky (member) | objection, approach | use a stricter check (error instead of warning), clarify error messages and documentation, and inspect commits even when not checked out at HEAD | fixed · fix pushed · author adopted suggested changes in c:5102482210; ryanofsky ACKed update | changed | yes | 2026-07-27: 'I think having the warning is better than not having it, but a stricter approach would seem better here.' Status from: 2026-08-21: ryanofsky: 'Thanks for the updates. New commit split makes this easier to review and I think the script messages and documentation are easier to understand now.' |
| sedited (member) | objection, interface | default behavior causes git-subtree-check to fail on most subtrees for users who only want to verify subtree integrity | answered · author agreed to invert default if preferred, and ryanofsky explained rationale; sedited has not followed up | answered | yes | 2026-09-21: 'The script without the extra argument fails on most subtrees and the exception is applied to the CI. My feeling is this is done the wrong way around. Somebody who just wants to verify subtree integrity does not care whether updates to the subtree are easier to backport.' Status from: 2026-09-22: Sjors: 'I would be fine with making `--incompatible` the default, if more people prefer that.' |
| maflcko (member) | objection, approach | adds unnecessary bash logic and maintenance overhead for what is a style preference that can be verified with git log or documentation | contested · maflcko reaffirmed objection and reiterated NACK in c:5794085899 and c:5905591607 following author and reviewer replies | changed | yes | 2026-09-22: 'I don't see why something so trivial needs so much code or even effort that all needs to be maintained in the future (like https://github.com/bitcoin/bitcoin/pull/35980) ... Approach NACK' Status from: 2026-09-30: maflcko: 'The more I think about this, the stronger my NACK gets and I am slowly leaning toward a wholesale concept NACK instead of an approach NACK.' |
| maflcko (member) | suggestion, maintenance | apply suggested checkout commands to developer-notes documentation to make instructions easy to copy-paste | fixed · fix pushed · author applied checkout instructions to doc/developer-notes.md | changed | yes | 2026-09-22: 'I think the suggested change should be applied. Otherwise, it is harder to copy-paste the instructions here and they'll have to be figured out fresh on each invocation.' Status from: 2026-09-23: Sjors: 'Done' |
Support:
- ryanofsky (member) (ACK, 2026-07-27) [read: doc/developer-notes.md, test/lint/git-subtree-check.sh]: having the warning is better than not having it, though a stricter approach would seem better
- ryanofsky (member) (ACK, 2026-08-21) [read: doc/developer-notes.md, test/lint/git-subtree-check.sh]: documents practice of basing new subtree updates on previous updates, allowing same commits to be merged into multiple branches and saving reviewer effort
- BrandonOdiwuor (contributor) (ACK, 2026-08-23) [tested]: tested that the new backportability check passes and errors as expected, and verified --incompatible suppresses the check
Participants:
- maflcko (member, objection, 10 statements 2026-07-10 to 2026-09-30): Approach NACK; considers bash logic unnecessary maintenance overhead for a style preference
- sedited (member, objection, 2 statements 2026-07-10 to 2026-09-21): objected that defaulting to check backportability causes failures when only verifying subtree integrity; questioned whether it solves a real problem
- ryanofsky (member, support, 9 statements 2026-07-27 to 2026-09-30): ACKed; argued the check simplifies non-trivial verification and documents good backporting practice
- BrandonOdiwuor (contributor, support, 1 statements 2026-08-23 to 2026-08-23): tested and ACKed
Files
86 lines under test/bench/ci.
- test/lint/git-subtree-check.sh +51/-16
- test/lint/README.md +9/-3
- doc/developer-notes.md +10/-0
- test/lint/test_runner/src/lint_repo_hygiene.rs +6/-1
Card
The short text the ranking pass compares across PRs.
Updates git-subtree-check.sh to verify that subtree merge commits are stacked directly on the previous subtree merge commit, allowing backports to release branches to reuse the exact same merge commit without a second round of review. The check can be bypassed with --incompatible when API breakage requires a newer base commit, which CI does by default to avoid false failures. This is a developer tooling and repository hygiene enhancement primarily benefiting backport maintainers and reviewers.
Data
thread read through 2026-09-30 (30 statements), head 6dab31cb46
labels: Tests
Categories
Test infrastructure
P3 · test coverage
Extends git-subtree-check to enforce that subtree updates can be backported verbatim to release branches without rereviewing. This adds a helpful hygiene check for subtree maintenance, but addresses an internal developer workflow issue that is reasonably deferrable.
Membership: Labeled Tests; modifies test/lint/git-subtree-check.sh and test/lint/test_runner/src/lint_repo_hygiene.rs.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1