Skip to content

lore-server: Implement the thin-client ContentDiff RPC (2-way)#138

Open
bclarke123 wants to merge 1 commit into
EpicGames:mainfrom
bclarke123:implement-contentdiff-rpc
Open

lore-server: Implement the thin-client ContentDiff RPC (2-way)#138
bclarke123 wants to merge 1 commit into
EpicGames:mainfrom
bclarke123:implement-contentdiff-rpc

Conversation

@bclarke123

Copy link
Copy Markdown

Summary

Hi, and thank you to everybody who has contributed to Lore!

lore.thin_client.v1.ThinClientService.ContentDiff has been a stub returning
Unimplemented since the thin-client service was introduced, while its
request/response contract is fully specified in thin_client.proto. This PR
implements the 2-way mode of that documented contract.

Motivation: the thin-client service exists so web UIs and SDKs don't have to
reimplement fragment reassembly and codecs. Today a thin client can learn
that a file changed (RevisionDiff) but not what changed. Showing a
diff requires reassembling both sides client-side, including codecs that
are only practically available server-side. I hit this building a web UI
over Lore; commit/diff views were impossible against a stock server.

No proto changes. This implements the shipped contract as documented:
header-first stream, binary/truncated short-circuit flags,
context_lines, max_diff_size, whitespace options, and empty-address
add/delete semantics.

Changes

  • lore-server/src/grpc/thinclient/v1/content_diff.rs (new): the handler.
    Reads both sides by CAS address, classifies text vs binary with
    infer_is_diffable_by_slice, produces the unified diff, and streams
    header + UTF-8-safe 64 KiB text chunks. Inputs larger than 16 MiB are
    not reassembled: a ranged read of the leading 8 KiB still classifies
    them (binary sets binary: true; text sets truncated: true).
  • lore-server/src/grpc/thinclient/v1/service.rs + mod.rs: replace the
    Unimplemented stub with the handler; module registration.
  • lore-revision/src/file/diff.rs: build_unified_patch becomes pub
    (one line + doc comment). The handler deliberately reuses the client
    diff pipeline rather than reimplementing it, so server-side diffs are
    byte-identical to client-side diffs, including the
    whitespace-normalisation-with-original-text behaviour and encoding
    handling.

Decisions that deserve reviewer eyes

  1. Hash-only address resolution. DiffChange.content_from/content_to
    carry the CAS hash without the context half, so the handler builds
    hash-only addresses and opts these reads out of LOCAL_ISOLATION
    (no_isolation()) to allow the store's MatchHash fallback. Access is
    not widened: reads stay scoped to the caller's authorized partition,
    and the content hash proves the bytes. If you'd rather DiffChange
    carry full Addresses, that's a wire change I deliberately did not
    make here and would be happy to follow up on.
  2. 3-way mode is explicitly not implemented (address_base set is
    rejected with Unimplemented and a clear message). Nothing calls it
    yet, and wiring merge3_text deserves its own focused PR.

Test plan

  • New unit tests (upstream test_store_create pattern): modify/add/delete
    roundtrips with line-stat and text assertions; binary short-circuit;
    max_diff_size truncation (stats survive, chunks withheld); >16 MiB
    binary input classified via prefix sniff. Test blobs are written with a
    non-default context so the hash-only MatchHash path is exercised, and
    one over-cap case covers the ranged prefix read.
  • cargo test -p lore-server: 755 pass (753 pre-existing + 2 new).
  • cargo test -p lore-revision: unchanged, all pass.
  • cargo clippy -p lore-server -p lore-revision --all-targets -- -D warnings --no-deps: clean.
  • cargo +nightly fmt --all: applied.
  • Exercised end-to-end from an external gRPC client (web UI): text diffs
    render, binary files short-circuit, adds/deletes and oversized files
    behave per the contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant