lint: have git-subtree-check check for backportability #35686

A subtree update is easier to backport when its merge commit is based on the previous subtree merge instead of on a newer master commit. 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.

Update git-subtree-check.sh to locate the merge that introduced the latest subtree squash reachable from COMMIT and error if its first parent is not the previous subtree merge.

Sometimes subtree and Bitcoin Core API changes are incompatible, making it necessary to base an update on a later commit. Add --incompatible to explicitly skip the backportability check in that case.

The CI lint job always sets --incompatible, so we rely on reviewers to run the new check.

As a preparatory refactor, replace getopts with explicit option parsing and add --remote as the preferred long alias for -r.

Example that passes:

test/lint/git-subtree-check.sh src/ipc/libmultiprocess 66b4e30e

Example that errors because the update is not based on the previous subtree merge:

test/lint/git-subtree-check.sh src/ipc/libmultiprocess 02afa661

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35686.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• fetch the subtreed remote first -> fetch the upstream remote first [“subtreed” is not a standard word; the intended meaning is unclear without guessing]

^{2026-09-23 10:42:46}

I'm not sure. We sometimes cherry-pick commits from the subtree if they contain fixes that need backporting. Is this really solving a problem?

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?

Also, can you explain how this simplifies "backport"? IIUC you are referring to merging the subtree merge commit as-is into several branches? I think this is nice, and good to keep in mind, but the alternative of having a second similar subtree merge (with only the commit id different) on the backport branch is also fine and harmless?

you are referring to merging the subtree merge commit as-is into several branches? I think this is nice, and good to keep in mind

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.

And it was specifically suggested here: #33439 (comment)

For future releases (31.x and later) we could base new subtree updates on previous subtree updates instead of on newer master commits.

And it was specifically suggested here: #33439 (comment)

For future releases (31.x and later) we could base new subtree updates on previous subtree updates instead of on newer master commits.

I did that in the last update commit (fa911d815dba280f7d4a1b3fe7b786be37afaf75). I guess in theory one could even come up with a copy-pasteable scripted-diff to mark/check such commits. Something like:

$ ./test/lint/commit-script-check.sh HEAD~..HEAD 
Running script for: 84868e9b2c96a14a49cec4a8b30e95bb8857d720

 # Verify last commit is also the last merge subtree commit
 [ "$( git log -1)" == "$( git log -1 src/ipc/libmultiprocess )" ]
 git merge 6d5f753921578eefdd3fce64cfc8ee7951b6cbc4 -m 'dummy'
Merge made by the 'ort' strategy.
 src/ipc/libmultiprocess/doc/versions.md        | 44 ++++++++++++++++++++++++--------------------
 src/ipc/libmultiprocess/include/mp/type-data.h |  3 ++-
 src/ipc/libmultiprocess/include/mp/version.h   |  2 +-
 src/ipc/libmultiprocess/test/mp/test/foo.capnp |  9 ++++++++-
 src/ipc/libmultiprocess/test/mp/test/foo.h     |  4 +++-
 src/ipc/libmultiprocess/test/mp/test/test.cpp  | 23 +++++++++++++++++------
 6 files changed, 55 insertions(+), 30 deletions(-)
OK



$ git log 
commit 84868e9b2c96a14a49cec4a8b30e95bb8857d720 (HEAD)
Merge: a9d1b652f3 6d5f753921

    scripted-diff: ipc: Merge libmul subtree update
    
    -BEGIN VERIFY SCRIPT-
    
     # Verify last commit is also the last merge subtree commit
     [ "$( git log -1)" == "$( git log -1 src/ipc/libmultiprocess )" ]
     git merge 6d5f753921578eefdd3fce64cfc8ee7951b6cbc4 -m 'dummy'
    
    -END VERIFY SCRIPT-

This way, each commit can opt-in or out, and enforce it. But :man_shrugging:

@maflcko I'm mainly concerned about the author forgetting to do this, and the reviewer(s) forgetting to check. That's what this linter fixes. Presumably both the author and reviewer will run this script.

If the author didn't forget, they can just suggest that reviewers confirm that HEAD^1 is the previous subtree merge.

cc @ryanofsky

Code review ACK 4ace058d85b9e7d005b39c31ed5d4eab71afc350. I think having the warning is better than not having it, but a stricter approach would seem better here.

The only reason this check should not be satisfied is when subtree and bitcoin core changes are incompatible. The script could trigger an error instead of warning if that's not the case. Also:

• The warning text seems potentially confusing and hard to act on because it doesn't say what the mismatched hashes mean.
• The documentation seems vague when it says to use the previous subtree "when possible" and because "this makes backporting easier." It should say more specifically when to stack on the subtree and how it does makes backporting easier.
• Skipping this check when the subtree commit is not the HEAD commit seems confusing. Previously the script could find the most recent subtree update by itself and perform a full check. Now it only does a partial check unless it is explicitly given the subtree merge commit hash (or that commit happens to be checked out).

Suggestion: I think a variation of this PR 15b44bfe7e4fa62b867ebcc2465bfaed27d689aa would address all these concerns

Also marco's verify script idea #35686 (comment) seems interesting. It's awkward because the script it is running is non-trivial and includes a hardcoded hash, but could be simplified if the scripted diff called another script in the repository.

@ryanofsky thanks, I took your version, but added an error if $merge somehow isn't set.

I split the options handling refactor into a separate commit. Also added --remote and replaced -r in the documentation (-r is kept as an alias).

Code review ACK 5b8973e355441cfed24b16cf2b9bb2aade7ca89f. 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.

The main benefit I see to this PR is just documenting the practice of basing new subtrees updates on previous updates, which is nice because it allows the same update commits to be merged into multiple branches, saving reviewer effort. The PR also makes this condition easy for reviewers to check by running the normal linter command.

Note: The new linter --incompatible option could be dropped in the future by requiring a stricter commit message format for subtree merge commits, requiring a tag in the merge commit messages to indicate whether the merged changes are not compatible and should be allowed to use a newer base. But for now I think having the option is better, because it makes it straightforward for reviewers and authors to check the stricter base condition now, without requiring the stricter condition to be imposed on all subtrees going forward.

re: sedited #35686 (comment)

We sometimes cherry-pick commits from the subtree if they contain fixes that need backporting. Is this really solving a problem?

Wow I didn't know this was done. Apparently this is implemented by disabling the subtree check entirely. Examples:

• 29.x: 53a5c7f1c95 in #35674
• 30.x: d09bd0f4025 in #35668
• 31.x: 1ee11d8ba64 in #35699

This can make sense when there is an isolated fix that needs to be cherry-picked, but it requires more manual review.

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 without requiring the same changes to be reviewed multiple times. Being able to merge the same commit hash into multiple branches is especially nice in that case. (It could be useful for cherry-picks too, but that's not the aim.)

ACK 5b8973e355441cfed24b16cf2b9bb2aade7ca89f

Tested the new backportability check:

• fa911d815: GOOD: correctly stacked; first parent is the previous subtree merge a9d1b652
• 02afa661: ERROR: first parent is not the previous subtree merge 2063f02bd5.

This matches the intended behaviour. Also verified that --incompatible correctly suppresses the check.

Test runs: <img width="1896" height="310" alt="Screenshot from 2026-08-24 08-30-37" src="https://github.com/user-attachments/assets/cbeb15ee-27cb-48c2-8b33-144039db4947" />

Parent inspection: <img width="1061" height="655" alt="Image" src="https://github.com/user-attachments/assets/55fdbb95-f2e1-45e5-a462-601e094cf9dc" />

Any reviewers who had hesitation about this PR earlier have feedback about the latest version? I think this change is practically useful because the condition it is checking for is difficult to verify without a script. It also includes good documentation updates, and the error message ("subtree contents are valid, but [...] the subtree update may not be possible to backport directly to release branches") should be clearer now than it was before.

Any reviewers who had hesitation about this PR earlier have feedback about the latest version?

I'm still not sure about the current --incompatible mechanics: 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. Maybe it would be better to change the docs (and the merge script?) in bitcoin-maintainer-tools to make sure the subtree pull is done correctly?

Yeah, just doing the few-line doc update here in this pull should be sufficient? If not, then it can always be re-visited in the future.

Also, it seems odd to add all this logic and Bash overhead for something that is basically a mostly irrelevant style preference.

@sedited I would be fine with making --incompatible the default, if more people prefer that.

I think the use cases are, in order of frequency:

1. Callers of ci/lint.py (which includes CI)
2. Reviewers checking a new subtree update
3. Someone who wants to verify subtrees without the other linters

For (1) the default doesn't matter. Changing the default would perhaps reduce confusion for (3), and slightly increase the odds of (2) missing the preference.

The script without the extra argument fails on most subtrees and the exception is applied to the CI.

This is intentional. Most subtree updates don't involve backwards incompatible API changes, so the new check and the new instructions are meant to encourage subtree bumps that can be directly backported with the same commit hash and more easily reviewed. This is better than the current behavior of bumping multiple times for different release branches, or disabling the subtree check in release branches (for example https://github.com/bitcoin/bitcoin/commit/53a5c7f1c95b604cd7abe11f58fe44c29b808ded, see #35686#pullrequestreview-4996279203) when changes need to be backported.

If there is a reason you think cleaner backports can't or shouldn't be used though, we can reverse the default behavior here. If we do that I would suggest naming the inverse option something like --strict or --check-clean-backport to be clear it is being more strict.

Somebody who just wants to verify subtree integrity does not care whether updates to the subtree are easier to backport.

The message says the subtree contents are valid but the changes can't be directly backported to release branches. If you think the error message isn't clear maybe we can improve it? I don't see a reason to hide this information, and don't think the current output is hard to understand.

disabling the subtree check in release branches, see #35686 (review)) when changes need to be backported.

Not sure why this is being brought up here, as it is unrelated for several reasons:

• The subtree check was disabled in the same pull that did the cherry-pick. Nothing in this pull here is preventing anyone from doing a cherry-pick or wholesale disabling the subtree check.
• Moreover, I presume a subtree merge wasn't intended anyway, because it would pull in other prior changes. This pull is about subtree updates with the same contents and nothing in this pull is is changing or preventing updates where the content is different.

As mentioned in my prior comment, the changes here are purely of stylistic nature and won't have any effect on the source code contents of this repo.

Again, I think it would be better to just change the doc/developer-notes.md. If a problem arises in the future, it can be considered to apply more changes to the workflow, but I think using unrelated problems as the motivation for this pull seems confusing.

.

Not sure why this is being brought up here, as it is unrelated for several reasons

There are 4 ways to update a subtree in a release branch: 1-merge an existing subtree bump commit from master, 2-create a new subtree bump commit specifically for that release branch. 3-cherry pick a change into a subtree and disable the linter. 4-edit the subtree directly and disable the linter.

These approaches are all different ways to accomplish the same thing, therefore they are related. Also I did not bring up cherrypicking, it was brought up earlier here: #35686 (comment).

IMO this approaches form a spectrum where the first is more reviewable and transparent and the last is the hardest to review and least transparent. This PR is a small, harmless nudge towards the earlier part of the spectrum, therefore I have ACKed it and would encourage others to review or weigh in with their opinions here.

Right, but 1 and 2 are equally trivial to review. The harder part are 3 and 4. This pull is only about a possible improvement to 1 and 2.

Also the reason I don't think the documentation change change is as important as the script change, is that creating a subtree bump that can be directly backported is trivial. Testing if a bump commit was created in right way is a lot more tricky. Or at least I don't know what straightforward git commands let you check this. So having the check in the subtree linter makes sense to me because it makes something complicated and annoying easy. I don't care very much if the check is turned on by default, though I do think it would be nice to turn on by default, since it seems like something that could increase transparency and decrease review burden.

Testing if a bump commit was created in right way is a lot more tricky. Or at least I don't know what straightforward git commands let you check this.

A simple git log has all the details needed: The subtree dir with the current squash and the previous one, and the merge commit as well as the previous merge commit. E.g.: git log -4 fa911d815dba280f7d4a1b3fe7b786be37afaf75

Also, a simple git diff can be used after re-creating the steps and observing an empty diff.

Also, I am sure an LLM can find a third way, or help with the prior step.

In any case, if way-1 falls back to way-2, it is harmless. There is no need to have a script for this. 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 #35980)

I don't like nacking pulls, but to clarify, all my previous comments were trying to say: Approach NACK

@maflcko wrote:

A simple git log has all the details needed

I think you're incorrectly assuming that most reviewers understand git as well as you do. Most mere mortals have to sit for ten minutes to wrap their heads around what an easy to backport subtree should look like. And they'll forget by the next time. Even to realize that something is not a problem takes mental effort.

It's better to figure this out once, express it in a linter, and only require people to think about it (with guide from an LLM) when that linter complains. In terms of cumulative cognitive burden, that should outweigh the effort needed to occasionally maintain this script. It's not like git or our subtree workflow change frequently.

Maintaining documentation (with lots of commands) is also more effort than maintaining scripts. The best way is probably an LLM that tries the instructions.

Even to realize that something is not a problem takes mental effort.

There is no problem to solve, this is just a mostly irrelevant style preference. The set of devs that would spend mental effort to think this is a problem, but also don't understand git log or git diff should be zero. Also, if someone doesn't know git at all, they probably shouldn't be bumping subtrees.

Maintaining documentation (with lots of commands) is also more effort than maintaining scripts.

You are modifying the dev notes in this pull, along with scripts. I don't think any scripts or "verification" is needed here for a style preference that is harmless to get wrong.

I think the 3 documented bash commands to bump the subtree are sufficient.

@maflcko your input is valuable here but if you don't think this PR is solving a problem that is worth your time, it seems like you could just decline to review it. An approach nack that does not cite specific harms the PR would cause if merged is hard to react or respond to. If the current PR would negatively impact you or anyone else, what exactly is the negative impact? We've discussed numerous alternatives and variations above (default on vs. off, dropping --incompatible, a commit-message-tag alternative), would any of them help?

On "no problem to solve" I don't think this is theoretical. Once people follow the new guidance to base updates on the previous subtree merge, it's easy for two non-identical subtree bumps to land on different branches unnoticed, because nothing today checks for that and nothing tells reviewers to check it by hand. git log/git diff can confirm a mismatch, but only for someone who remembers to run them for this specific purpose on this specific pair of commits. When the same commit hash is reused instead, whether two PRs merge identical subtrees becomes instantly verifiable, without the manual step reviewers will otherwise tend to skip.

@maflcko your input is valuable here but if you don't think this PR is solving a problem that is worth your time, it seems like you could just decline to review it.

I think the project is better off if more reviewers nack'd changes they don't like with a rationale. Of course, if a nack doesn't make sense, it is trivial to dismiss and ignore. However, discouraging nacks and telling reviewers to instead mute and walk away from harmful changes doesn't seem too useful.

An approach nack that does not cite specific harms the PR would cause if merged is hard to react or respond to. If the current PR would negatively impact you or anyone else, what exactly is the negative impact?

Not sure why this was missed, but the negative impacts are:

• This adds logic and code for something that has never been a problem in the past. The negative impact here is confusion.
• This add documentation and logic overhead for subtree bumps. This is an extra cost/time for everyone.
• Extra code and logic needs to be maintained and understood, which is a cost.
• The discussion here mentions unrelated problems to the changes here, and there seems to be some confusion that this will fix issues that are not being fixed. Having code for something without understanding exactly what the goal is does seems like a negative impact. The cost here is that this will likely need to be reverted or reworked some time in the future.

it's easy for two non-identical subtree bumps to land on different branches unnoticed

This pull request doesn't change anything about that. Before and after this pull request, non-identical subtree bumps can land on different branches "unnoticed": I'll ignore that this script here isn't marked for backport, so I'll assume that reviewers will backport the script themselves and run it, and check that it is passing. However, the script only checks that the bump is backportable, not that it was backported.

When the same commit hash is reused instead, whether two PRs merge identical subtrees becomes instantly verifiable, without the manual step reviewers will otherwise tend to skip.

So I think the goal here is to have identical commit IDs across branches. Checking that is a manual process before and after this pull request, so I don't think anything about that changes here.

Also, literally the first comment on this pull explains that identical commit IDs won't be possible in all cases anyway (https://github.com/bitcoin/bitcoin/pull/35686#issuecomment-4936395031), so this seems like a case-by-case review anyway and not something that should be automated.

Also, whether or not the commit ID of a subtree change is identical to some other commit ID should mostly be irrelevant, because what really matters is the source code that will be shipped to users. So reviewers should better spend time on reviewing that a backported subtree bump doesn't introduce incompatibility bugs in the old branch instead of distracting themselves with less relevant git metadata.

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.

Hi Marco, I think nacks are useful and I'm not trying discouraging them. I just couldn't figure out how this PR would harm you or anyone else, or cause any problems if merged. This PR seems to help increase quality and ease of reviews in an area where they seem a little weak (backports). It also cleans up linter code and adds an extra check that is difficult to perform by hand, printing clear and obvious output describing its results.

One thing I would like to discourage is spending time debating benefits of PRs. If you don't think a PR is beneficial, you should definitely say that and make it clear. It is very clear that you don't think this PR is beneficial, and that is useful feedback, and it could be a good signal to close this PR, and for others not to review it, and to discourage future work in this area. And I don't have a problem with closing this PR, I just want to increase the quality of review discussion around this PR by trying to specifically identify any possible harms.

Benefits of PRs are more subjective than harms because different people use different features of code and care about different aspects of it. The best way to adjudicate benefits, IMO, is for people to review PRs they are interested in, and if a PR attracts enough review for the type of change it is and does not cause harms, then it can be merged. Harms can almost always be discussed more objectively than benefits, especially if they are discussed with some specifics as in this type of person trying to do this action will be harmed because X will be the case instead of Y.

Thank you for clarifying the harms you see. I guess I thought initially you were only opposed to the linter change here, not the documentation change, but now even the documentation change is bad, I guess because there are extra steps to run. And you think the linter extra check here is a maintenance burden and "will likely need to be reverted or reworked some time in the future." I don't really agree with either objection. I think the documentation is clear and simple because it literally gives you five lines of code you can paste. And it tells you how to run the linter which it didn't do before and chooses a good default base commit instead of leaving it unspecified. I also don't know why the new check would need to be changed or reverted later but maybe you can clarify what kind of scenario you have in mind. I might suggest though since this is such a low stakes area (a linter change) a better approach might not be to debate vaguely whether a change may break something in the future (when there is not a specific concern), but to just point out change seems fragile, and if it is actually is fragile, be gratified that you are right and have a very strong argument to remove it in the future if/when there is actually a real problem.

Anyway thank you for the feedback, I was really only trying to clarify and understand it, and suggest a way to proceed without taking a lot of time debating very subjective concerns which are ultimately less important than more objective ones.