chadtmiller.com/knotless

Switch repository

Loading…

What changes

A pull based on another pull's branch moves when that pull lands. The lower pull's head branch is deleted, and each open pull based on it now targets what the lower pull landed on, as GitHub's retargeting does. This holds for knotless merge and for the merge queue. A merge with --keep-branch moves nothing.

The moved pull still shows only its own changes. A squash leaves the pull above holding the lower pull's original commits. So the state record that moves it carries since, a new optional field: the commit the lower pull's head was at. Its diff, commit list, merge and checks all start there. since stops counting once the branch is rebased or takes in its base, as its own history then says where it starts.

A runner's state record with no state now counts, so the runner that lands the queue can move these pulls.

Why

Stacked pulls are the case where one change needs another that has not landed. Today the lower pull's branch is kept alive for the pull above, which stays pointed at it, and a reviewer of the pull above sees the lower pull's changes again once it is moved by hand onto a linear branch. This is the first step of stack support: showing a stack and queueing one in order build on it.

Moving alone would be worse than today on a linear branch, so the start commit goes with it.

How it was checked

The forge test in knotless-serve covers a stack of three on a linear branch: the lowest squashed, the middle's diff, the middle after taking in its base, the top after a rebase, and all three files on the branch at the end. It also covers moving on a branch that is not linear. Nothing has been built or run locally: this relies on the check and test checks.

Shortcomings

The branch is deleted before the records that move the pulls are written, as a queue batch does both in that order. If a landing stops between the two, the pull above targets a branch that is gone, and needs knotless pull retarget, without its start commit. Moving is not yet tested through the queue's landing itself.

🤖 Generated with Claude Code

This pull moves open pulls based on a landed pull's branch onto that pull's target. It records since (the lower head) so their diff, commits, merge and gauge start there. It also lets a runner's stateless record count.

I read the whole diff and found nothing that must change. In since_at, since counts only while it is an ancestor of the head and the merge-base with the base is still an ancestor of since. That matches the stated "rebased or took in its base" cases. A later retarget or rename record clears since, because assemble overwrites it. The merge_tree_from plumbing is applied consistently across merge, compare, diff, admit, the queue stack and landing, and the runner.

Notes, none blocking:

• queue::land now calls moved_on, and the body says no test reaches it. AGENTS.md makes test hold a 100% coverage gate, so that branch will fail the check until a queue landing test covers it. Add one, ideally with a stacked pull in the batch.
• allowed now lets a runner write a record with no state on any subject, not only a base/since move. A runner is already trusted to land pulls, so this is low risk. Consider limiting it to records that carry base.
• The delete-before-move window is already named in the body. In merge, writing the move records in the same applyWrites as the merged state would close part of it. That is not possible for the branch delete, so I accept it as is.

The only change since my last round is a one-line edit in since_at. It calls git.output_of(&args) in place of git.output(&args, None), so the ancestor check now goes through git's public call. Behavior is the same, and I found nothing wrong with it. My earlier notes still stand and none of them blocks the merge. The main one is that the test check's 100% coverage gate needs a test that lands a stacked pull through queue::land.

The only change since my last round is in pull_range in web.rs. An error from forge::since_of is now mapped with not_found instead of being passed up as it was. I found nothing wrong with the edit. It only changes which error comes back when the start commit can't be resolved. The check and test runs for this commit were still queued or running when I looked. My earlier notes still stand and none of them blocks the merge. The main one is that the 100% coverage gate in test needs a test that lands a stacked pull through queue::land.

The only change since my last round is in the forks_pulls_reviews_and_merges test in forge.rs. The test failed at de07dac, and this edit appears to answer that failure. It drops the old check that the pull list shows atop based on main after tidy lands. It now merges atop and fetches main to confirm h is there, so it tests the move by landing on main. This fits the new behavior, and I found nothing wrong with it.

The test and check runs for this commit were still queued or running when I looked, so I can't confirm they pass. The pull was ejected from the queue because test failed at the earlier commit. My earlier notes still stand and none blocks the merge. The main one is that the 100% coverage gate needs a test that lands a stacked pull through queue::land. If test fails again on coverage, that is the likely cause.