docs(tree): add spec for persisted commit metadata - #28047
Draft
Noah Encke (noencke) wants to merge 5 commits into
Draft
docs(tree): add spec for persisted commit metadata#28047Noah Encke (noencke) wants to merge 5 commits into
Noah Encke (noencke) wants to merge 5 commits into
Conversation
github-actions
Bot
added
area: tools
area: dds
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (391 lines, 1 files), I've queued these reviewers:
How this works
|
Store the metadata on the commits in the EditManager summary rather than separately. This gives the metadata the same lifetime as its commit with no additional garbage collection, keeps the persisted metadata consistent with the trunk by construction, and lets the metadata page alongside its commits if summary history is virtualized later. It also removes the need to encode revision tag keys, since an encoded commit already carries its revision and session ID. Introduce EditManagerFormatVersion v7 and MessageFormatVersion v7 to carry the field, so writing is gated on minVersionForCollab and a document containing metadata can only be opened by a client that preserves it. Add guidance to keep the metadata off GraphCommit: rebaseBranch rebuilds commits as fresh object literals, so extra properties on the commit object do not survive rebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2711c804-43fc-4414-9ab8-1d46d07b5f70
A transaction can run on any branch. An application that renders from a fork and publishes by merging that fork into main annotates its transactions on the fork, and those commits reach the wire only when the merge submits them, so submitCommit must find metadata that a different checkout recorded. Specify that the index lives on SharedTreeCore and is shared by every checkout of the tree, that keying by RevisionTag is what keeps lookups valid as a commit moves between branches, and that entries are removed only on rollback or eviction rather than on branch disposal. Add a lifecycle section for commits made on a branch and tests covering the fork-merge path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2711c804-43fc-4414-9ab8-1d46d07b5f70
Metadata describes the commit a transaction produces, so a transaction that produces no commit has nothing to describe and its metadata is discarded without error. Raising instead would convert a handled result into an exception on the explicit rollback path, where an application rolls a transaction back after detecting an invalid edit and returns an error to its own caller. It would also be unactionable, since whether a body will produce a change is not knowable before running it, and it would penalize exactly the applications whose "no change means no checkpoint" rule motivates annotating in the first place. Specify both paths that reach this, note that callers can detect the case by comparing the branch head across the call, and cover the no-change and explicit-rollback cases in the tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2711c804-43fc-4414-9ab8-1d46d07b5f70
Put the metadata directly on GraphCommit as a required property rather than in a revision-keyed index. Required, not optional: mintCommit and rebaseBranch rebuild commits from their parts instead of spreading them, so an optional property would be dropped silently, while a required one turns both into compile errors that have to be answered. This removes the index entirely. The metadata is reachable wherever a commit is, is collected with its commit so eviction needs no participation, cannot drift out of sync with the commit graph, and crosses branches with the commits a merge carries. Call out that the rebuild sites must propagate the property rather than write undefined to satisfy the compiler, since that would reintroduce the loss the required property prevents. Expose reads as persistedMetadata on TreeBranchCommitMetadata, so metadata arrives with history navigation instead of a separate lookup. There is deliberately no accessor by revision, which would require the index back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2711c804-43fc-4414-9ab8-1d46d07b5f70
Contributor
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Contributor
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
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.
Description
Adds a design specification for attaching arbitrary, application-defined metadata to a commit,
persisting it in the document, and querying it later.
The metadata shares the lifetime of the commit it is attached to, so it is dropped once that commit
leaves the collaboration window.
The spec covers the transaction API surface, keying by
RevisionTag, the op envelope change,a new summarizable index, and the behavior required across submit, remote receipt, resubmit,
stashed ops, rollback, eviction, and load.
This is documentation only; no product code is changed.
The design is written to avoid introducing a new persisted format version, and the compatibility
section records the consequences of that choice.
Reviewer Guidance
The review process is outlined on this wiki page.
This is a draft for design feedback rather than an implementation proposal.
Two areas would benefit most from scrutiny:
Messageschemas permitting additional properties.Existing clients tolerate this, but it is not currently documented as a guarantee, so the spec
proposes declaring the field explicitly and annotating the schemas so the behavior is preserved.
silently drops the metadata when it summarizes. The "Compatibility characteristics" section
spells this out; please confirm the best-effort guarantee is acceptable.