Skip to content

Let a push ask the server to rebase instead of merge - #3

Closed
jochenhz wants to merge 4 commits into
fix/push-reparent-only-on-rewritefrom
feat/push-server-rebase
Closed

jochenhz wants to merge 4 commits into
fix/push-reparent-only-on-rewritefrom
feat/push-server-rebase

Conversation

@jochenhz

@jochenhz jochenhz commented Sep 15, 2026

Copy link
Copy Markdown

Stacked on #2 — base branch is fix/push-reparent-only-on-rewrite. Review that one first; this PR's diff shows only the rebase work once #2 is merged.

Integrating a push whose parent is no longer the branch head already applies the incoming revision's changes onto the head — a three-way diff against the revision's own parent, applied to the head's state:

state::apply_tree_changes(repository, state_current, &diff_result.changes)
state_current.set_parent_self(current_head);
state_current.set_parent_other(incoming_revision);   // ← the only thing making it a merge

So the machinery for a rebase is there. This adds a rebase flag to BranchPushRequest that asks for the other shape: same diff, same conflict handling, but the result records only the head as its parent, so the branch stays linear.

lore push                        → refuse on divergence      (unchanged)
lore push --fast-forward-merge   → integrate as a merge      (unchanged)
lore push --rebase               → integrate as a rebase     (new)
lore push --rebase --fast-forward-merge → error

Design notes

Mutually exclusive, not ordered by precedence. The two ask for different histories, and quietly applying one would leave the caller unable to tell which it got. Rejected in three places because each has callers the others do not see: clap (conflicts_with), branch::push for embedders that never touch the CLI, and the v1 handler for anything arriving on the wire.

rebase does not imply fast_forward_merge. A server that predates the field ignores it, sees a push with no integration opt-in, and soft-rejects. That is the point: a client asking to rebase must never be answered with a merge it did not ask for. The client turns that refusal into a message naming the likely cause, since a server that did understand the field would have rebased or reported conflicts instead — so the refusal is diagnostic.

Metadata. A rebase is stamped rebased-on-push rather than merged-by / fast-forward-merge. Nothing was merged, and a reader should not meet a "merge" with no second parent.

What it gives up, stated in try_integrate_onto_head: merging keeps the pushed revision reachable as a second parent, which is also what lets the next revision of the same push use it as a diff base. Rebasing drops it, so it survives only as long as the store holds it — long enough for the rest of that push, not beyond.

Verification

contrib/repro-push-reparent-drops-work.sh (from #2) grew a MODE=rebase arm. Run against a server built from this branch and against the released 0.9.0 server:

result
new server, --rebase OK — all files present, history is linear: no merge revision (revisions 1‑4)
new server, --fast-forward-merge OK — merge shape unchanged
0.9.0 server, --rebase REFUSED"the server did not perform the rebase…", A's file untouched, nothing merged
both flags error: the argument '--rebase' cannot be used with '--fast-forward-merge'

cargo test -p lore-revision green (700+ tests). cargo fmt --check and cargo clippy clean for the touched crates. Protobuf bindings regenerated with protoc 36.1; the diff is a pure addition.

Not covered

  • No test inside this repository, same reason as lore-revision: Only follow a parent the server renumbered #2: the path needs a client pushing against a live server and there is no harness for that here. The script under contrib/ is what I have.
  • Links and layers go through the same loop and are passed rebase: false from their call sites, so their behaviour is unchanged — but the reproduction does not exercise them.
  • The deprecated model.proto request has no rebase field; that handler always merges. Intentional, but worth a maintainer's eye.
  • GC interaction. A rebased revision leaves the pushed revision unreachable from the branch while it is still needed as the diff base for the rest of the same push. It is in the store, so this works — but it is an implicit assumption about GC not collecting mid-push that deserves a decision rather than my inference.
  • Whether this wants an LEP rather than a PR is your call; I did not file one.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

A push that carries more than one local revision into a branch head that
has moved silently drops the other side's work. The push succeeds, no
conflict is reported, and the overwritten commit is still in the history,
so nothing looks wrong afterwards.

`collect_fragments_and_push` bent a revision's parent pointer onto
whatever the previous iteration left in `current_latest`, keeping its
tree:

    if !current_latest.is_zero() && state.parent_self() != current_latest {
        state.set_parent_self(current_latest);
    }

That is right for the case the TODO describes: the server accepts the
revision as it stands and only rewrites its revision number, so the same
content is stored under a new signature and the next revision has to
follow it.

It is wrong after a server-side fast-forward merge. `current_latest` is
then a revision the server built, carrying the other side's work, and
re-pointing at it makes this revision's stale tree look like a direct
descendant. The server takes the ordinary fast-forward path — no merge,
no three-way diff, no conflict check — and the branch becomes that tree.
Everything the other side added is simply absent from it.

Track the rename instead: remember the pushed revision as (signature in
local history, signature on the server) and follow the parent only when
it is exactly that revision, and only when the two names differ. Clear it
whenever the head moved to a merge. Any other mismatch is now left alone,
which sends the revision as it stands and lets the server three-way merge
it against a base it already holds — the revision's own parent, which
reached the server as the previous merge's second parent. So a chain of N
local revisions pushes correctly, one server merge each, instead of the
last one winning outright.

The guard `!current_latest.is_zero()` is why this was never seen with a
single-revision push: the block cannot fire on the first revision of a
push, only from the second on.

Reproduction added at contrib/repro-push-reparent-drops-work.sh — CLI and
loreserver only, no other dependencies. Against 0.9.0 it reports

    RESULT: LOST — A's file is gone from the branch, with no error reported

and with this change

    RESULT: OK — A's file survived B's push

`cargo test -p lore-revision` is green (700+ tests, no failures).

Signed-off-by: Jochen Hunz <j.hunz@anchorpoint.app>
@jochenhz
jochenhz force-pushed the fix/push-reparent-only-on-rewrite branch from 300cce1 to a6eec66 Compare September 16, 2026 06:43
Integrating a push whose parent is no longer the branch head already
applies the incoming revision's changes onto the head — a three-way diff
against the revision's own parent, applied to the head's state. The only
thing that makes the result a merge rather than a rebase is that it also
records the pushed revision as `parent_other`.

Add `rebase` to BranchPushRequest to ask for the other shape. The diff and
the conflict handling are unchanged; the result keeps only the head as its
parent, so the branch stays linear, and it is stamped `rebased-on-push`
instead of `merged-by`/`fast-forward-merge` so a reader can tell the two
apart rather than seeing a merge with no second parent.

The two are mutually exclusive rather than ordered by precedence: they ask
for different histories, and quietly applying one would leave the caller
unable to tell which it got. Rejected in three places, because each has
callers the others do not see — clap (`conflicts_with`), `branch::push`
for embedders, and the v1 handler for anything on the wire.

`rebase` deliberately does NOT imply `fast_forward_merge`. A server that
predates the field ignores it, sees a push with no integration opt-in, and
soft-rejects. A client that asked to rebase must never be answered with a
merge it did not ask for — so the client also turns that refusal into a
message naming the likely cause, since a server that understood the field
would have rebased or reported conflicts instead.

What a rebase gives up is stated in try_integrate_onto_head: merging keeps
the pushed revision reachable as a second parent, which is also what lets
the next revision of the same push use it as a diff base. Rebasing drops
it, so it survives only as long as the store holds it — long enough for
the rest of the push, not beyond.

Verified with contrib/repro-push-reparent-drops-work.sh, which grew a
MODE=rebase arm:

  new server, --rebase              RESULT: OK, history is linear
                                   (revisions 1-4, no merge revision)
  new server, --fast-forward-merge  RESULT: OK, merge shape unchanged
  0.9.0 server, --rebase            RESULT: REFUSED, A's file untouched
  both flags                        error: the argument '--rebase' cannot
                                   be used with '--fast-forward-merge'

`cargo test -p lore-revision` green, fmt and clippy clean.

Stacked on the parent-pointer fix: without it a multi-revision push
arrives with its parents already bent onto the head, so revisions 2..N
never reach this path at all and there would be nothing to rebase.

Signed-off-by: Jochen Hunz <j.hunz@anchorpoint.app>
The server's push() gained the rebase parameter and BranchPushRequest
gained the field, which the workspace's test targets pick up: 43 test
call sites of branch_push::push pass it as false (unchanged merge
behaviour), and lore-proto's exhaustive-destructuring guard names it.
That guard is doing its job — a new field on a v1 message should not
land without someone looking at it.

Also adds the test that belongs with the validation: a request setting
both fast_forward_merge and rebase is refused with InvalidArgument.

Found by CI: I had only built the binaries locally, never
cargo check --workspace --all-targets, so none of this showed up.

Signed-off-by: Jochen Hunz <j.hunz@anchorpoint.app>
@jochenhz
jochenhz force-pushed the feat/push-server-rebase branch from 54041dd to 9c9e596 Compare September 16, 2026 06:44
clippy's fn_params_excessive_bools fired on push() once rebase made it a
fourth bool, and it is right to: fast_forward_merge and rebase are
mutually exclusive, so they were never two independent choices. Replace
the pair with an Integration enum — Refuse, Merge, Rebase — which makes
the invalid combination unrepresentable past the wire boundary instead
of something a runtime guard has to catch.

The v1 handler now maps the two request bits onto it and is the single
place that rejects both-set; the guard inside push() is gone because the
type no longer allows the state. The deprecated handler maps its one bit
to Merge or Refuse.

Also fixes the nightly rustfmt diff CI reported: the hook runs
cargo +nightly fmt, and the repo's rustfmt.toml uses nightly-only
options, so a stable-toolchain format is not the same check.

Signed-off-by: Jochen Hunz <j.hunz@anchorpoint.app>
@jochenhz
jochenhz force-pushed the fix/push-reparent-only-on-rewrite branch 2 times, most recently from abe7c41 to 212b90e Compare September 16, 2026 19:12
@jochenhz jochenhz closed this Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant