chadtmiller.com/knotless

Switch repository

Loading…

What changes

A stack queued lands in turn. A pull queued while the pull whose branch it targets is queued, or ejected to be fixed, now waits out of line. When the one below lands, it moves to what that one landed on and joins that line. knotless queue lists one waiting as stacked, and its page says Waits for the pull below it to land.

Leaving takes those waiting. knotless queue --leave on a pull takes queued pulls waiting on it too, highest first, so none lands on the branch below.

The merge box adds a stack at once. On a pull with open pulls below it, Add all N to merge queue queues them lowest first, then this one. Add to merge queue stays, and adds this pull alone.

Why

Without this, a stack's upper pull queued early lands on the lower pull's branch, and its work never reaches main. Queueing one at a time, waiting for each to land, is the only safe way today.

How it was checked

• Unit test a_pull_waits_for_the_one_below_it: order, positions, cascade, forks, a pull left, a loop.
• Integration test a_stack_lands_in_turn on the runner's real queue: two pulls land in order on main, the upper's squash holding only its own file, and one leaving takes the other. This is also the first test that moves a pull through queue landing.
• Merge box drawn with just look at top of a stack, waiting and busy, desktop light and phone dark.
• This pull was opened on the map pull's branch, to be moved by that pull's landing on prod.

Shortcomings

• A fresh critique found the phone's button row wraps with Add all N alone beside the menu. Left as it is: four controls do not fit one phone row, and the row wraps as it does for any long label. Only a stack's pull gets the fourth.
• Pulls land one at a time, each with its own run. A stack is not checked once at its top, as the mock drew. Batching a stack needs the batch to squash through a chain of bases, which is a much larger change.
• Merge pull stays on a pull with open pulls below. The mock turned it off, but merging a part into its feature branch is the normal flow there.
• A lower pull closed while one above waits, or merged keeping its branch, leaves the upper in the line for that branch, and it lands there. Moving only happens when the branch goes.
• knotless queue --leave refuses an ejected pull, so one above an ejected pull waits until the lower is fixed, closed, or the upper leaves.
• No CLI flag queues a stack. Agents queue bottom-up.

🤖 Generated with Claude Code

This pull makes a queued pull wait out of line while the pull below it in a stack is queued or ejected. When the lower pull lands, the upper one moves to what it landed on. queue leave also takes the pulls waiting on the one that leaves, highest first. The merge box gets an "Add all N" button.

I found nothing that must change.

• below, lands_on (which stops on a loop) and leaving agree with each other.
• enqueue on an already queued pull keeps its place, so queueing the stack lowest first is safe to repeat.
• A pull whose head is a fork's branch is not treated as stacked.
• The docs, the change file and the lexicon description were updated with the code.

I could not read the runner's checks for this head, because run_list returned nothing. I also did not trace how the runner moves the upper pull when the lower one lands.

One thing to know: "Add all N" only checks the top pull's state before enabling. If a lower pull is a draft, conflicts or has failed checks, queuePull refuses it partway through the loop. Some of the stack is then queued and the page shows the error. That is acceptable, since the lower pulls that did queue still land correctly.

The code is the same as the commit I approved earlier (4c4679d). The only new commit is a merge of main, which touches none of the files in this diff. My earlier approval therefore still holds, and I found nothing new that must change.

The pull makes a queued pull wait out of line while the pull below it in a stack is queued or ejected. It moves to what the lower pull landed on once that lands. queue leave takes the waiting pulls with it, highest first, and the merge box adds an "Add all N" button. The unit and integration tests cover order, cascade, forks, a pull leaving and a loop. The docs, the change file and the lexicon text were updated with the code.

My earlier note still applies. "Add all N" checks only the top pull's state before enabling. If a lower pull is refused partway through, part of the stack stays queued. The lower pulls that did queue still land correctly, so this is acceptable. The shortcomings listed in the body are fair.

The only change since the head I last approved (f7d9b9d) is 1aef991. It swaps [top.uri.clone()] for std::slice::from_ref(&top.uri) in one unit-test assertion, which clears a clippy clone lint. The assertion checks the same thing as before, and no production code changed. My earlier approval still holds, and I found nothing new that must change.

My earlier note about "Add all N" still applies. It checks only the top pull's state before enabling, so a lower pull that is refused partway through leaves part of the stack queued. The pulls that did queue still land correctly, so I don't ask for a change.