Generate revertibles for commits incoming from a merge

.changeset/small-months-heal.md

@@ -0,0 +1,10 @@
+---
+"@fluidframework/tree": minor
+---
+---
+"section": tree
+---
+
+Branch merges generate revertibles
+
+Merging any number of commits into a target branch (via the 'merge' method) now generates a revertible for each commit on the target branch.

packages/dds/tree/src/shared-tree-core/branch.ts

@@ -11,7 +11,6 @@ import {
 	type ChangeFamily,
 	type ChangeFamilyEditor,
 	CommitKind,
-	type CommitMetadata,
 	type GraphCommit,
 	type RevisionTag,
 	type TaggedChange,
@@ -46,6 +45,7 @@ import { fail } from "../util/index.js";
 export type SharedTreeBranchChange<TChange> =
 	| {
 			type: "append";
+			kind: CommitKind;
 			change: TaggedChange<TChange>;
 			newCommits: readonly GraphCommit<TChange>[];
 	  }
@@ -116,11 +116,6 @@ export interface SharedTreeBranchEvents<TEditor extends ChangeFamilyEditor, TCha
 	 */
 	afterChange(change: SharedTreeBranchChange<TChange>): void;
 
-	/**
-	 * {@inheritdoc TreeViewEvents.commitApplied}
-	 */
-	commitApplied(data: CommitMetadata): void;
-
 	/**
 	 * Fired when this branch forks
 	 * @param fork - the new branch that forked off of this branch
@@ -250,13 +245,13 @@ export class SharedTreeBranch<
 	 * Apply a change to this branch.
 	 * @param change - the change to apply
 	 * @param revision - the revision of the new head commit of the branch that contains `change`
-	 * @param changeKind - the kind of change to apply
+	 * @param kind - the kind of change to apply
 	 * @returns the change that was applied and the new head commit of the branch
 	 */
 	public apply(
 		change: TChange,
 		revision: RevisionTag,
-		changeKind: CommitKind = CommitKind.Default,
+		kind: CommitKind = CommitKind.Default,
 	): [change: TChange, newCommit: GraphCommit<TChange>] {
 		this.assertNotDisposed();
 
@@ -270,18 +265,13 @@ export class SharedTreeBranch<
 
 		const changeEvent = {
 			type: "append",
+			kind,
 			change: tagChange(changeWithRevision, revision),
 			newCommits: [newHead],
 		} as const;
 
 		this.emit("beforeChange", changeEvent);
 		this.head = newHead;
-
-		// If this is not part of a transaction, emit a commitApplied event
-		if (!this.isTransacting()) {
-			this.emit("commitApplied", { isLocal: true, kind: changeKind });
-		}
-
 		this.emit("afterChange", changeEvent);
 		return [changeWithRevision, newHead];
 	}
@@ -351,12 +341,6 @@ export class SharedTreeBranch<
 
 		this.emit("beforeChange", changeEvent);
 		this.head = newHead;
-
-		// If this transaction is not nested, emit a commitApplied event
-		if (!this.isTransacting()) {
-			this.emit("commitApplied", { isLocal: true, kind: CommitKind.Default });
-		}
-
 		this.emit("afterChange", changeEvent);
 		return [commits, newHead];
 	}
@@ -442,14 +426,14 @@ export class SharedTreeBranch<
 
 	/**
 	 * Spawn a new branch that is based off of the current state of this branch.
-	 * Changes made to the new branch will not be applied to this branch until the new branch is merged back in.
-	 *
-	 * @remarks Forks created during a transaction will be disposed when the transaction ends.
+	 * @param commit - the commit to base the new branch off of. Defaults to the head of this branch.
+	 * @remarks Changes made to the new branch will not be applied to this branch until the new branch is merged back in.
+	 * Forks created during a transaction will be disposed when the transaction ends.
 	 */
-	public fork(): SharedTreeBranch<TEditor, TChange> {
+	public fork(commit: GraphCommit<TChange> = this.head): SharedTreeBranch<TEditor, TChange> {
 		this.assertNotDisposed();
 		const fork = new SharedTreeBranch(
-			this.head,
+			commit,
 			this.changeFamily,
 			this.mintRevisionTag,
 			this.branchTrimmer,
@@ -501,9 +485,9 @@ export class SharedTreeBranch<
 			removedCommits: deletedSourceCommits,
 			newCommits,
 		} as const;
+
 		this.emit("beforeChange", changeEvent);
 		this.head = newSourceHead;
-
 		this.emit("afterChange", changeEvent);
 		return rebaseResult;
 	}
@@ -525,6 +509,10 @@ export class SharedTreeBranch<
 			0x597 /* Branch may not be merged while transaction is in progress */,
 		);
 
+		if (branch === this) {
+			return undefined;
+		}
+
 		// Rebase the given branch onto this branch
 		const rebaseResult = this.rebaseBranch(branch, this);
 		if (rebaseResult === undefined) {
@@ -537,6 +525,7 @@ export class SharedTreeBranch<
 		const taggedChange = makeAnonChange(change);
 		const changeEvent = {
 			type: "append",
+			kind: CommitKind.Default,
 			get change(): TaggedChange<TChange> {
 				return taggedChange;
 			},

packages/dds/tree/src/shared-tree/treeCheckout.ts

@@ -117,6 +117,7 @@ export interface TreeBranch extends ViewableTree {
 	 * @param view - a branch which was created by a call to `branch()`.
 	 * It is automatically disposed after the merge completes.
 	 * @remarks All ongoing transactions (if any) in `branch` will be committed before the merge.
+	 * A "commitApplied" event and a corresponding {@link Revertible} will be emitted on this branch for each new change merged from 'branch'.
 	 */
 	merge(branch: TreeBranchFork): void;
 
@@ -508,67 +509,79 @@ export class TreeCheckout implements ITreeCheckoutFork {
 				}
 			}
 		});
-		_branch.on("commitApplied", (data) => {
-			const commit = _branch.getHead();
-			const { change, revision } = commit;
-			let withinEventContext = true;
-
-			const getRevertible = hasSchemaChange(change)
-				? undefined
-				: (onRevertibleDisposed?: (revertible: Revertible) => void) => {
-						if (!withinEventContext) {
-							throw new UsageError(
-								"Cannot get a revertible outside of the context of a commitApplied event.",
-							);
-						}
-						if (this.revertibleCommitBranches.get(revision) !== undefined) {
-							throw new UsageError(
-								"Cannot generate the same revertible more than once. Note that this can happen when multiple commitApplied event listeners are registered.",
-							);
-						}
-						const revertibleCommits = this.revertibleCommitBranches;
-						const revertible: DisposableRevertible = {
-							get status(): RevertibleStatus {
-								const revertibleCommit = revertibleCommits.get(revision);
-								return revertibleCommit === undefined
-									? RevertibleStatus.Disposed
-									: RevertibleStatus.Valid;
-							},
-							revert: (release: boolean = true) => {
-								if (revertible.status === RevertibleStatus.Disposed) {
-									throw new UsageError(
-										"Unable to revert a revertible that has been disposed.",
-									);
-								}
-
-								const revertMetrics = this.revertRevertible(revision, data.kind);
-								this.logger?.sendTelemetryEvent({
-									eventName: TreeCheckout.revertTelemetryEventName,
-									...revertMetrics,
-								});
-
-								if (release) {
-									revertible.dispose();
-								}
-							},
-							dispose: () => {
-								if (revertible.status === RevertibleStatus.Disposed) {
-									throw new UsageError(
-										"Unable to dispose a revertible that has already been disposed.",
-									);
-								}
-								this.disposeRevertible(revertible, revision);
-								onRevertibleDisposed?.(revertible);
-							},
-						};
-
-						this.revertibleCommitBranches.set(revision, _branch.fork());
-						this.revertibles.add(revertible);
-						return revertible;
-					};
-
-			this.events.emit("commitApplied", data, getRevertible);
-			withinEventContext = false;
+		_branch.on("afterChange", (event) => {
+			// The following logic allows revertibles to be generated for the change.
+			// Currently only appends (including merges) and transaction commits are supported.
+			// In both cases they must not be part of an (outer) transaction.
+			if (!_branch.isTransacting()) {
+				if (
+					event.type === "append" ||
+					(event.type === "replace" && getChangeReplaceType(event) === "transactionCommit")
+				) {
+					for (const commit of event.newCommits) {
+						const kind = event.type === "append" ? event.kind : CommitKind.Default;
+						const { change, revision } = commit;
+
+						const getRevertible = hasSchemaChange(change)
+							? undefined
+							: (onRevertibleDisposed?: (revertible: Revertible) => void) => {
+									if (!withinEventContext) {
+										throw new UsageError(
+											"Cannot get a revertible outside of the context of a commitApplied event.",
+										);
+									}
+									if (this.revertibleCommitBranches.get(revision) !== undefined) {
+										throw new UsageError(
+											"Cannot generate the same revertible more than once. Note that this can happen when multiple commitApplied event listeners are registered.",
+										);
+									}
+									const revertibleCommits = this.revertibleCommitBranches;
+									const revertible: DisposableRevertible = {
+										get status(): RevertibleStatus {
+											const revertibleCommit = revertibleCommits.get(revision);
+											return revertibleCommit === undefined
+												? RevertibleStatus.Disposed
+												: RevertibleStatus.Valid;
+										},
+										revert: (release: boolean = true) => {
+											if (revertible.status === RevertibleStatus.Disposed) {
+												throw new UsageError(
+													"Unable to revert a revertible that has been disposed.",
+												);
+											}
+
+											const revertMetrics = this.revertRevertible(revision, kind);
+											this.logger?.sendTelemetryEvent({
+												eventName: TreeCheckout.revertTelemetryEventName,
+												...revertMetrics,
+											});
+
+											if (release) {
+												revertible.dispose();
+											}
+										},
+										dispose: () => {
+											if (revertible.status === RevertibleStatus.Disposed) {
+												throw new UsageError(
+													"Unable to dispose a revertible that has already been disposed.",
+												);
+											}
+											this.disposeRevertible(revertible, revision);
+											onRevertibleDisposed?.(revertible);
+										},
+									};
+
+									this.revertibleCommitBranches.set(revision, _branch.fork(commit));
+									this.revertibles.add(revertible);
+									return revertible;
+								};
+
+						let withinEventContext = true;
+						this.events.emit("commitApplied", { isLocal: true, kind }, getRevertible);
+						withinEventContext = false;
+					}
+				}
+			}
 		});
 
 		// When the branch is trimmed, we can garbage collect any repair data whose latest relevant revision is one of the

packages/dds/tree/src/test/shared-tree-core/branch.spec.ts

@@ -8,7 +8,6 @@ import { strict as assert } from "assert";
 import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal";
 
 import {
-	CommitKind,
 	type GraphCommit,
 	type RevisionTag,
 	findAncestor,
@@ -335,32 +334,6 @@ describe("Branches", () => {
 		assert.equal(changeEventCount, 0);
 	});
 
-	it("do not emit a commitApplied event for commits within transactions", () => {
-		// Create a branch and count the change events emitted
-		let commitEventCount = 0;
-		const branch = create();
-		const unsubscribe = branch.on("commitApplied", () => {
-			commitEventCount += 1;
-		});
-		// Start and immediately abort a transaction
-		branch.startTransaction();
-		change(branch);
-		branch.abortTransaction();
-		assert.equal(commitEventCount, 0);
-		unsubscribe();
-	});
-
-	it("commitApplied event includes metadata about the commit", () => {
-		// Create a branch and count the change events emitted
-		const branch = create();
-		const unsubscribe = branch.on("commitApplied", ({ isLocal, kind }) => {
-			assert.equal(isLocal, true);
-			assert.equal(kind, CommitKind.Default);
-		});
-		change(branch);
-		unsubscribe();
-	});
-
 	it("emit a fork event after forking", () => {
 		let fork: DefaultBranch | undefined;
 		const branch = create();