fix: rm no longer counts a commit and the commit that reverts it - #664
Merged
Merged
Conversation
A probe commit and its exact revert, left on a backup branch after the PR was squashed (kinisi_ros#11898), kept rm refusing. Neither has a copy on a remote, and a branch that changes nothing proves nothing to the squash rule. A fourth rule clears a pair: two counted single-parent commits, the second on the first, where the second tree is the tree under the first and the first tree is not. Pairs are taken from the bottom, one per commit, so a revert of a revert stays counted. A ref or worktree HEAD on the reverted commit, or a second commit that grew from it, holds the pair back. Every git refusal clears nothing.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideAdds a conservative fourth cleanup rule that clears only adjacent counted commit/revert pairs which provably cancel byte-for-byte and are not retained by another ref or child commit. It obtains the required graph and ref-tip metadata from Git, fails closed on incomplete data, integrates after copy/squash rules, and includes extensive behavioral and safety regression tests plus documentation. Sequence diagram for conservative reverted-pair cleanupsequenceDiagram
participant Unsaved
participant Git
participant Graph
participant RefTips
Unsaved->>Git: unpushed_graph()
Git-->>Unsaved: CommitGraph or refusal
alt graph unavailable or malformed
Unsaved-->>Unsaved: Clear nothing
else counted adjacent pair found
Unsaved->>Git: ref_tips()
Git-->>Unsaved: Ref and worktree HEAD tips or refusal
alt ref tips unavailable
Unsaved-->>Unsaved: Clear nothing
else pair not held by a tip or second child
Unsaved->>Graph: Verify tree and parent structure
Graph-->>Unsaved: Reverted pair
Unsaved-->>Unsaved: Clear C and R
else pair is held
Unsaved-->>Unsaved: Keep both counted
end
end
Flow diagram for bottom-up revert-pair matchingflowchart TD
A[Counted commits remain after copy and squash rules] --> B[Read unpushed graph with trees and parents]
B --> C{Adjacent counted C then R?}
C -- No --> D[Keep commits counted]
C -- Yes --> E{R tree equals tree under C, and C changes it?}
E -- No --> D
E -- Yes --> F{C has no ref tip and exactly one child?}
F -- No --> D
F -- Yes --> G[Clear C and R]
G --> H{Another revert follows?}
H -- Yes --> I[Bottom-up pairing leaves the later revert counted]
H -- No --> J[Pair is removed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
refs/replace or a graft can make git log show a commit and its revert as each other's parent. The walk down the pairs never ended and grew without limit. A walk now stops at a commit it has seen, and nothing on a cycle is a pair. The doc comments also say that a linked worktree's own refs are not read, as the count does not read them.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… top flag it re-derives A Pair names its reverted and revert commits, and the walk memo carries the pair it took, so the held-back check reads the pairs the walk found with no second reverts() call.
…ver counted Local tags are counted: unpushed_commits names them on the positive side. Only a tag from the remote, or refs/original, leaves a commit uncounted.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dl kinisi-ros-sim-gate-all-other-nothing-kged rmrefused on 2026-10-01. It said "holds 6 unpushed commit(s)". The 6 commits were on local branches that are not checked out. Their PR, kinisi_ros#11898, was squashed intomain, and GitHub deleted the remote branches.2 of the 6 are a commit and its exact revert, next to each other on
sim-gate-backup:a7d877c077"kinisi_benchmarks: Probe the sim selection with a docstring edit (revert before merge)"0bf0560c5b"Revert ..."0bf0560c5b^isa7d877c077, and the tree of0bf0560c5bis the tree ofa7d877c077^. Together they change nothing, so deleting them loses nothing. The guard counted them anyway. #653 listed "A commit and its revert" as a known limit of the copy rule. Each commit has no copy on its own. The squash rule from #659 does not clear them either, because a branch that changes nothing proves nothing to it.The other 4 are older drafts of commits that were edited later. Replay each one on the final version's parent and the tree is not the final tree (9 to 16 lines differ). They are real work that exists nowhere else, and they still count. So this change does not make that
rmsucceed. It makes the count 4, not 6.What this changes
A fourth step, the revert rule, runs after the squash rule. Like the other two, it can only remove commits from the count.
A pair is two counted commits, R on top of C. R has one parent, C. C has one parent of its own. The tree of R is the tree under C, and the tree of C is not. Trees, not patches, so R must take back the whole change of C, byte for byte. Both drop out.
These are never a pair:
A commit is in one pair at most, and the pairs are taken from the bottom. A revert of a revert puts the change back. So in C, R, R2 the first two drop out and R2 still counts. C, R, R2, R3 is two pairs.
A pair drops out only when nothing else holds the state of C. Each of these holds C, and then both commits still count:
So every ref that reaches C reaches it through R.
Two new git verbs.
unpushed_graphis onegit log --boundary --all --not --remoteswith each commit's tree and parents. The boundary lines give the tree under a commit made on top of the remote.ref_tipsis onegit rev-list --no-walk --all: the commit that each ref and each worktree's HEAD names, with tags peeled. It runs only when a pair is found.Every failure clears nothing. A refusal on either verb clears nothing, and so does a graph line in a shape the parser does not know.
The rule is
reverted_in_pairsinrust/devlaunch-core/src/domain/workspace_state.rs. It runs insideunsaved(), sodl --ls --jsonandrmstill give the same answer. It runs only when the other rules left a commit counted.docs/cleanup.mdandCHANGELOG.mddescribe the rule and drop "a commit and its revert" from the limits.What the revert rule does not find, so these commits still count:
How I checked it
Tests first. The new tests in
workspace_state/tests.rsfailed on a stub that cleared nothing, then passed with the rule:NothingToLose, in a SHA-1 and in a SHA-256 repository;NothingToLose;origin/mainholds as a cherry-pick still counts;ScriptedRunnertests: a refused graph, refused ref tips, a graph line in another shape, and a pair where either commit is no longer counted each clear nothing. A control with the same answers clears the pair.The squash-rule test that had a commit and its revert keep 2 now asks the squash rule on its own. It still clears nothing there, and the whole guard now gives
NothingToLose.A review pass by a separate agent tried to make the rule clear work that exists nowhere else. It found one defect, and I fixed it with a test that failed first:
refs/replaceentry or a graft can makegit logshow a commit and its revert as each other's parent. The walk down the pairs then never ended, and its memory grew without limit, sormanddl --ls --jsonhung. Now a walk stops at a commit it has seen, and nothing on a cycle is a pair.It also found that the doc comment over-promised: a linked worktree's own
refs/worktree/*do not hold a pair, because--alldoes not read them. The comment now says so. Things it tried that held: revert chains of 2 to 6 commits, nested annotated tags, tags on trees, the stash, shallow clones, short hashes, andgit logconfig such asformat.pretty,core.abbrevandcolor.ui=always.Mutants. I removed each guard in turn: the empty-C check, the tree check, one parent for R, one parent for C, C still counted, R still counted, pairs from the bottom, the ref-tip check, the second-child check, the two refusals, the two parser refusals and
--boundary. A test failed each time.In the container:
pixi run cargo test --workspace --no-fail-fast: no failures. On the first run, one test that does not reach this code (the_json_listing_migrates_the_cache_and_the_table_does_not) failed under load. It passed on 3 reruns and on the run after the review fix.HOME=/tmp/noid GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 pixi run cargo test -p devlaunch-core --lib: 2078 passed.pixi run cargo clippy --locked --all-targets -- -D warnings,pixi run cargo fmt --checkandprek run --from-ref origin/main --to-ref HEAD: clean.pixi run test: 839 passed.The real commits. A bundle holds the three branches from that workspace. I made a scratch copy of a kinisi_ros clone, made local branches from the bundle, and ran the guard through a test binary that is not committed. With the PR branch at its last head (
aaf7a57c0b) asorigin/sim-gate-all-other-nothingand checked out, as the workspace had it:dlreports): 6;a7d877c077and0bf0560c5bdrop out. The 4 drafts (f859476c75,3c1997bb68,c01b0fde20,7587798c79) still count.With only the bundle's branches and no PR branch, the count goes from 29 to 27, and again only the pair drops out.
A second self-review (Defects, Types, Tests and one mutant) found no correctness defect. The mutant broke "one pair per commit, from the bottom", and
a_revert_of_a_revert_stays_countedkilled it. It led to these commits:92c80a86: a test with a held pair beside a pair that clears. An all-or-nothing hold passed every test before it.cdc917b3: aPair { reverted, revert }struct replaces two(String, String)tuples in opposite orders, and the walk keeps the pair instead of a flag it worked out again later.fd4de7b0: a test withlog.showSignature=trueand a signed commit pins--no-show-signatureon the graph verb.8211895f: the revert-of-a-revert test now checks which commit stays counted, not only the count. Pairing from the top gave the same count.24d1fe34: the graph verb's doc comment said a commit that only a tag reaches is never counted. A local tag is counted.Not fixed here:
log.showSignatureon the count itself. The revert rule reads its graph correctly withlog.showSignature=true, butunpushed_commits, which runs before every rule, has no--no-show-signature. Its lines then cannot be read, so every commit stays counted. That is onmaintoo. It refuses more, never less, so it is left for its own PR.Review notes
--allwith nothing excluded, so a commit that only a tag orrefs/originalreaches can still hold a pair back. That is the safe direction.--alldoes not see. A linked worktree'srefs/worktree/*andrefs/bisect/*, and the stash's older reflog entries, are not seen by--all. The count had this gap before this change. I did not change it.🤖 Generated with Claude Code