chadtmiller.com/knotless

Switch repository

Loading…

What changes

Merges into a branch that is not linear always make a merge commit. knotless merge, the MCP merge tool and the pages' merge box no longer fast-forward when they could: the commit is a merge of base then head, its message ending with the pull's AT-URI, so history names every pull. getMergeStatus's and merge's method is merge or squash; fastForward is no longer made. Checks are still read at the head when the base has not moved, as the merge commit then has the head's tree, and the runner still reuses a pull's pass at the push by tree.

A pull already on its base merges as it stands. knotless merge of a pull whose head the base has, pushed there by hand, writes its merged state and moves nothing, rather than an empty merge commit.

A push that lands a pull merges it. After a push lands, git-remote-knotless reads the open pulls into each branch it moved and marks merged each whose head the branch has now and did not before (forge::landed), closing what it names, and says so on git's stderr. A fork's pull counts when the pushing checkout has its head. A failure to read or mark them is said, and the push stands.

Commits link to the pull that brought them in. The pages link a commit by its message's last line as before, or else by the merged states: a commit in a merged pull's from..commit is that pull's, the one bringing in least when two did (a stack pushed at once). It reuses the walk vouched already makes of those ranges, so a page reads nothing more. Repositories whose pulls were fast-forwarded link their history so as soon as this deploys.

Why

group-host's pulls landed by fast-forward. Their commits linked to no pull, and the four pushed onto main stayed open until merged by hand. A merge commit, as GitHub makes from its merge button, names the pull in history itself; a push that lands a pull's head marks it merged, as GitHub's does; and every commit of a merged pull is linked to it, whatever the merge.

How it was checked

pushes_mark_the_pulls_they_land_merged (knotless-serve forge tests): a stack of two, a fork's pull and a pull of its own pushed to main at once are marked merged and the issue one names is closed; a pull not on main, one into a branch the push left alone and one already on main stay open; the last merges as it stands without moving main; and listCommits links each commit to the pull bringing in least. a_push_lands_though_its_pulls_cannot_be_read covers an appview that is not there. Fast-forward assertions in the forge, gateway and MCP tests now expect the merge commit. Left to CI: everything else that merges.

Shortcomings

Each push to a branch now reads the repository's pulls once from the appview (or the space), one request. The pages' merge box keeps its words for fastForward, for servers not yet updated.

This pull makes merge always produce a merge commit naming the pull, or a squash into a linear branch. A pull whose head the base already has is marked merged without moving the branch. It also adds forge::landed, which git-remote-knotless runs after a push to mark merged each open pull whose head the pushed branch newly contains. The commit-to-pull linking in the pages and the tests were not covered, so my read is of the CLI/helper, MCP and merge paths.

I read the forge.rs, helper.rs and MCP changes, and found no bug.

• merge skips the push when there is true, and still writes the merged state and closes issues.
• brought requires the head to be in the new tip and not in the old one, so a rewind or an already-landed pull is not re-marked.
• A failure in landed is reported on stderr and the push stands.
• The changeset, the docs (api.md), the lexicon description and the MCP tool text were updated.

I did not read the serve and web changes (the pull's commit-linking code) or the new tests. The diff was too large to list its files, and I read only the files I could name. At review time docs had passed and check, test, shots and e2e were still queued or running, so I have not seen them pass.

One note, not a blocker: landed loads the repository's pulls from the appview on every push to any branch, even when the repository has no open pull into the branch that moved. If that request is slow, it adds to every push. Consider skipping it when the pushed branches are not the base of any pull the helper already knows about, if that is cheap to find.

The only change since my last review (a8bc290 to d86609b) is a commit that updates tests in the mcp, forge, gateway, private_flow and web suites. They now expect a merge commit where a pull could have fast-forwarded. The edits match the behaviour the pull describes. For example, the merged commit differs from the head, and the web log's older commit now links to its pull. I found nothing to change. My earlier note about landed reading the pulls on every push still stands, and it isn't a blocker. At review time check was running and test was queued, so I haven't seen them pass. They will show whether any merge-related assertion was missed.

The only change since my last review (d86609b to 15fb30f) is in crates/knotless-serve/tests/forge.rs, in merges_wait_for_their_checks. The test now keeps the merge commit's id as tip. It records the later pull's failing check against tip instead of the old head. That is right now that a merge commit is made and main has moved, so the base the check names is that commit. Nothing else changed, and I have no new findings. My earlier note that landed reads the pulls on every push still stands and isn't a blocker. At review time check and docs had passed. test, web, e2e, shots and lexicons were still running or queued. The test run covers these merge assertions.