Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Determine branch a & b are different in clone time #23485

Merged
merged 10 commits into from
Jan 8, 2025
1 change: 1 addition & 0 deletions packages/dds/tree/src/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ export {
replaceChange,
type RebaseStats,
type RebaseStatsWithDuration,
isAncestor,
} from "./rebase/index.js";

export {
Expand Down
1 change: 1 addition & 0 deletions packages/dds/tree/src/core/rebase/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,5 @@ export {
type RebaseStats,
type RebaseStatsWithDuration,
replaceChange,
isAncestor,
} from "./utils.js";
27 changes: 27 additions & 0 deletions packages/dds/tree/src/core/rebase/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -651,3 +651,30 @@ namespace Rollback {
}
}
}

/**
* Checks if one node is an ancestor of another in a parent-linked tree structure.
* @param ancestor - The potential ancestor node
* @param descendant - The potential descendant node
* @param allowEqual - If true, returns true when ancestor === descendant
* @returns true if ancestor is an ancestor of descendant (or equal if allowEqual is true)
*/
export function isAncestor<TNode extends { readonly parent?: TNode }>(
ancestor: TNode,
descendant: TNode,
allowEqual: boolean,
): boolean {
if (allowEqual && ancestor === descendant) {
return true;
}

let current = descendant.parent;
while (current !== undefined) {
if (current === ancestor) {
return true;
}
current = current.parent;
}

return false;
}
31 changes: 19 additions & 12 deletions packages/dds/tree/src/shared-tree/treeCheckout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
type RevertibleAlphaFactory,
type RevertibleAlpha,
type GraphCommit,
isAncestor,
} from "../core/index.js";
import {
type FieldBatchCodec,
Expand Down Expand Up @@ -605,21 +606,27 @@ export class TreeCheckout implements ITreeCheckoutFork {
revertible.dispose();
}
},
clone: (forkedBranch: TreeBranch) => {
if (forkedBranch === undefined) {
return this.createRevertible(revision, kind, checkout, onRevertibleDisposed);
}

clone: (targetBranch: TreeBranch) => {
// TODO:#23442: When a revertible is cloned for a forked branch, optimize to create a fork of a revertible branch once per revision NOT once per revision per checkout.
const forkedCheckout = getCheckout(forkedBranch);
const targetCheckout = getCheckout(targetBranch);

const revertibleBranch = this.revertibleCommitBranches.get(revision);
assert(
revertibleBranch !== undefined,
0xa82 /* change to revert does not exist on the given forked branch */,
);
forkedCheckout.revertibleCommitBranches.set(revision, revertibleBranch.fork());
if (revertibleBranch === undefined) {
throw new UsageError("Unable to clone a revertible that has been disposed.");
}

const commitToRevert = revertibleBranch.getHead();
const activeBranchHead = targetCheckout.#transaction.activeBranch.getHead();

if (isAncestor(commitToRevert, activeBranchHead, true) === false) {
throw new UsageError(
"Cannot clone revertible for a commit that is not present on the given branch.",
);
}

targetCheckout.revertibleCommitBranches.set(revision, revertibleBranch.fork());

return this.createRevertible(revision, kind, forkedCheckout, onRevertibleDisposed);
return this.createRevertible(revision, kind, targetCheckout, onRevertibleDisposed);
},
dispose: () => {
if (revertible.status === RevertibleStatus.Disposed) {
Expand Down
5 changes: 4 additions & 1 deletion packages/dds/tree/src/test/shared-tree/undo.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,10 @@ describe("Undo and redo", () => {

const undoOriginalPropertyOne = undoStack.pop();

assert.throws(() => undoOriginalPropertyOne?.clone(viewB).revert(), "Error: 0x576");
assert.throws(
() => undoOriginalPropertyOne?.clone(viewB),
/Cannot clone revertible for a commit that is not present on the given branch./,
);
});

// TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1632,7 +1632,6 @@ export const shortCodeMap = {
"0xa7e": "Expected at least two types",
"0xa7f": "Delta manager does not have inbound/outbound queues.",
"0xa80": "Invalid delta manager",
"0xa82": "change to revert does not exist on the given forked branch",
"0xa83": "Expected commit(s) for a non no-op rebase",
"0xa84": "Commit must be in the branch's ancestry",
"0xa86": "Expected source commits in non no-op merge",
Expand Down
Loading