fix: stop stale checkout diff subscriptions (#2317)
* Fix pending checkout diff subscriptions * Reduce checkout status Git spawns * fix(checkout): own pending diff cancellation Checkout sessions could invalidate a subscription before its shared watch had finished opening. Register targets synchronously and let the diff manager honor cancellation so pending and concurrent subscriptions share one teardown path. --------- Co-authored-by: Mohamed Boudra <boudra.moha@gmail.com>
This commit is contained in:
co-authored by
Mohamed Boudra
parent
bd937850b3
commit
9952615c33
@@ -14,6 +14,56 @@ import type pino from "pino";
|
|||||||
import { CheckoutDiffManager } from "./checkout-diff-manager.js";
|
import { CheckoutDiffManager } from "./checkout-diff-manager.js";
|
||||||
import type { WorkspaceGitService } from "./workspace-git-service.js";
|
import type { WorkspaceGitService } from "./workspace-git-service.js";
|
||||||
|
|
||||||
|
interface Deferred<T> {
|
||||||
|
promise: Promise<T>;
|
||||||
|
resolve(value: T): void;
|
||||||
|
}
|
||||||
|
|
||||||
|
function createDeferred<T>(): Deferred<T> {
|
||||||
|
let resolve!: (value: T) => void;
|
||||||
|
const promise = new Promise<T>((resolvePromise) => {
|
||||||
|
resolve = resolvePromise;
|
||||||
|
});
|
||||||
|
return { promise, resolve };
|
||||||
|
}
|
||||||
|
|
||||||
|
function createPendingManager() {
|
||||||
|
const watches: Array<{
|
||||||
|
cwd: string;
|
||||||
|
onChange: () => void;
|
||||||
|
unsubscribeCalls: number;
|
||||||
|
resolve(): void;
|
||||||
|
}> = [];
|
||||||
|
const workspaceGitService = {
|
||||||
|
getCheckoutDiff: async () => ({ diff: "", structured: [] }),
|
||||||
|
requestWorkingTreeWatch: (cwd: string, onChange: () => void) => {
|
||||||
|
const pending = createDeferred<{ repoRoot: string | null; unsubscribe: () => void }>();
|
||||||
|
const watch = {
|
||||||
|
cwd,
|
||||||
|
onChange,
|
||||||
|
unsubscribeCalls: 0,
|
||||||
|
resolve: () => {
|
||||||
|
pending.resolve({
|
||||||
|
repoRoot: "/tmp/repo",
|
||||||
|
unsubscribe: () => {
|
||||||
|
watch.unsubscribeCalls += 1;
|
||||||
|
},
|
||||||
|
});
|
||||||
|
},
|
||||||
|
};
|
||||||
|
watches.push(watch);
|
||||||
|
return pending.promise;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const logger = { child: () => logger, warn: () => {} };
|
||||||
|
const manager = new CheckoutDiffManager({
|
||||||
|
logger: logger as unknown as pino.Logger,
|
||||||
|
paseoHome: "/tmp/paseo-test",
|
||||||
|
workspaceGitService,
|
||||||
|
});
|
||||||
|
return { manager, watches };
|
||||||
|
}
|
||||||
|
|
||||||
describe("CheckoutDiffManager", () => {
|
describe("CheckoutDiffManager", () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
vi.useFakeTimers();
|
vi.useFakeTimers();
|
||||||
@@ -103,6 +153,54 @@ describe("CheckoutDiffManager", () => {
|
|||||||
expect(unsubscribe).toHaveBeenCalledTimes(1);
|
expect(unsubscribe).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("cancels a subscription while its working tree watch is still opening", async () => {
|
||||||
|
const { manager, watches } = createPendingManager();
|
||||||
|
const abort = new AbortController();
|
||||||
|
|
||||||
|
const pendingSubscription = manager.subscribe(
|
||||||
|
{
|
||||||
|
cwd: "/tmp/repo/packages/server",
|
||||||
|
compare: { mode: "uncommitted" },
|
||||||
|
signal: abort.signal,
|
||||||
|
},
|
||||||
|
() => {},
|
||||||
|
);
|
||||||
|
abort.abort();
|
||||||
|
watches[0].resolve();
|
||||||
|
await pendingSubscription;
|
||||||
|
|
||||||
|
expect(watches[0].unsubscribeCalls).toBe(1);
|
||||||
|
expect(manager.getMetrics()).toEqual({
|
||||||
|
checkoutDiffTargetCount: 0,
|
||||||
|
checkoutDiffSubscriptionCount: 0,
|
||||||
|
checkoutDiffWatcherCount: 0,
|
||||||
|
checkoutDiffFallbackRefreshTargetCount: 0,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
test("shares one opening target between concurrent subscriptions", async () => {
|
||||||
|
const { manager, watches } = createPendingManager();
|
||||||
|
|
||||||
|
const firstSubscription = manager.subscribe(
|
||||||
|
{ cwd: "/tmp/repo/packages/server", compare: { mode: "uncommitted" } },
|
||||||
|
() => {},
|
||||||
|
);
|
||||||
|
const secondSubscription = manager.subscribe(
|
||||||
|
{ cwd: "/tmp/repo/packages/server", compare: { mode: "uncommitted" } },
|
||||||
|
() => {},
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(watches).toHaveLength(1);
|
||||||
|
watches[0].resolve();
|
||||||
|
const [first, second] = await Promise.all([firstSubscription, secondSubscription]);
|
||||||
|
expect(manager.getMetrics().checkoutDiffSubscriptionCount).toBe(2);
|
||||||
|
|
||||||
|
first.unsubscribe();
|
||||||
|
expect(watches[0].unsubscribeCalls).toBe(0);
|
||||||
|
second.unsubscribe();
|
||||||
|
expect(watches[0].unsubscribeCalls).toBe(1);
|
||||||
|
});
|
||||||
|
|
||||||
test("diffCwd uses repoRoot from the working tree watch result", async () => {
|
test("diffCwd uses repoRoot from the working tree watch result", async () => {
|
||||||
const { manager, workspaceGitService } = createManager({ repoRoot: "/tmp/repo" });
|
const { manager, workspaceGitService } = createManager({ repoRoot: "/tmp/repo" });
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,11 @@ import { toCheckoutError } from "./checkout-git-utils.js";
|
|||||||
|
|
||||||
const CHECKOUT_DIFF_WATCH_DEBOUNCE_MS = 150;
|
const CHECKOUT_DIFF_WATCH_DEBOUNCE_MS = 150;
|
||||||
|
|
||||||
|
type CheckoutDiffWorkspace = Pick<
|
||||||
|
WorkspaceGitService,
|
||||||
|
"getCheckoutDiff" | "requestWorkingTreeWatch"
|
||||||
|
>;
|
||||||
|
|
||||||
export type CheckoutDiffCompareInput = SubscribeCheckoutDiffRequest["compare"];
|
export type CheckoutDiffCompareInput = SubscribeCheckoutDiffRequest["compare"];
|
||||||
|
|
||||||
export type CheckoutDiffSnapshotPayload = Omit<
|
export type CheckoutDiffSnapshotPayload = Omit<
|
||||||
@@ -32,45 +37,70 @@ interface CheckoutDiffWatchTarget {
|
|||||||
refreshQueued: boolean;
|
refreshQueued: boolean;
|
||||||
latestPayload: CheckoutDiffSnapshotPayload | null;
|
latestPayload: CheckoutDiffSnapshotPayload | null;
|
||||||
latestFingerprint: string | null;
|
latestFingerprint: string | null;
|
||||||
|
openPromise: Promise<void> | null;
|
||||||
|
}
|
||||||
|
|
||||||
|
export interface CheckoutDiffSubscriptionRequest {
|
||||||
|
cwd: string;
|
||||||
|
compare: CheckoutDiffCompareInput;
|
||||||
|
signal?: AbortSignal;
|
||||||
|
}
|
||||||
|
|
||||||
|
export interface CheckoutDiffSubscription {
|
||||||
|
initial: CheckoutDiffSnapshotPayload;
|
||||||
|
unsubscribe: () => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export class CheckoutDiffManager {
|
export class CheckoutDiffManager {
|
||||||
private readonly workspaceGitService: WorkspaceGitService;
|
private readonly workspaceGitService: CheckoutDiffWorkspace;
|
||||||
private readonly targets = new Map<string, CheckoutDiffWatchTarget>();
|
private readonly targets = new Map<string, CheckoutDiffWatchTarget>();
|
||||||
|
|
||||||
constructor(options: {
|
constructor(options: {
|
||||||
logger: pino.Logger;
|
logger: pino.Logger;
|
||||||
paseoHome: string;
|
paseoHome: string;
|
||||||
workspaceGitService: WorkspaceGitService;
|
workspaceGitService: CheckoutDiffWorkspace;
|
||||||
}) {
|
}) {
|
||||||
this.workspaceGitService = options.workspaceGitService;
|
this.workspaceGitService = options.workspaceGitService;
|
||||||
}
|
}
|
||||||
|
|
||||||
async subscribe(
|
async subscribe(
|
||||||
params: {
|
params: CheckoutDiffSubscriptionRequest,
|
||||||
cwd: string;
|
|
||||||
compare: CheckoutDiffCompareInput;
|
|
||||||
},
|
|
||||||
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
||||||
): Promise<{ initial: CheckoutDiffSnapshotPayload; unsubscribe: () => void }> {
|
): Promise<CheckoutDiffSubscription> {
|
||||||
const cwd = params.cwd;
|
const cwd = params.cwd;
|
||||||
const compare = this.normalizeCompare(params.compare);
|
const compare = this.normalizeCompare(params.compare);
|
||||||
const target = await this.ensureTarget(cwd, compare);
|
const target = this.ensureTarget(cwd, compare);
|
||||||
target.listeners.add(listener);
|
target.listeners.add(listener);
|
||||||
|
target.openPromise ??= this.openTarget(target);
|
||||||
|
|
||||||
const initial =
|
let isSubscribed = true;
|
||||||
target.latestPayload ??
|
const unsubscribe = () => {
|
||||||
(await this.computeCheckoutDiffSnapshot(target.cwd, target.compare, {
|
if (!isSubscribed) {
|
||||||
diffCwd: target.diffCwd,
|
return;
|
||||||
}));
|
}
|
||||||
target.latestPayload = initial;
|
isSubscribed = false;
|
||||||
target.latestFingerprint = JSON.stringify(initial);
|
params.signal?.removeEventListener("abort", unsubscribe);
|
||||||
return {
|
this.removeListener(target, listener);
|
||||||
initial,
|
|
||||||
unsubscribe: () => {
|
|
||||||
this.removeListener(target.key, listener);
|
|
||||||
},
|
|
||||||
};
|
};
|
||||||
|
params.signal?.addEventListener("abort", unsubscribe, { once: true });
|
||||||
|
if (params.signal?.aborted) {
|
||||||
|
unsubscribe();
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
await target.openPromise;
|
||||||
|
const initial =
|
||||||
|
target.latestPayload ??
|
||||||
|
(await this.computeCheckoutDiffSnapshot(target.cwd, target.compare, {
|
||||||
|
diffCwd: target.diffCwd,
|
||||||
|
}));
|
||||||
|
target.latestPayload = initial;
|
||||||
|
target.latestFingerprint = JSON.stringify(initial);
|
||||||
|
return { initial, unsubscribe };
|
||||||
|
} catch (error) {
|
||||||
|
unsubscribe();
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
scheduleRefreshForCwd(cwd: string): void {
|
scheduleRefreshForCwd(cwd: string): void {
|
||||||
@@ -136,19 +166,17 @@ export class CheckoutDiffManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private removeListener(
|
private removeListener(
|
||||||
targetKey: string,
|
target: CheckoutDiffWatchTarget,
|
||||||
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
||||||
): void {
|
): void {
|
||||||
const target = this.targets.get(targetKey);
|
|
||||||
if (!target) {
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
target.listeners.delete(listener);
|
target.listeners.delete(listener);
|
||||||
if (target.listeners.size > 0) {
|
if (target.listeners.size > 0) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
this.closeTarget(target);
|
this.closeTarget(target);
|
||||||
this.targets.delete(targetKey);
|
if (this.targets.get(target.key) === target) {
|
||||||
|
this.targets.delete(target.key);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private scheduleTargetRefresh(target: CheckoutDiffWatchTarget): void {
|
private scheduleTargetRefresh(target: CheckoutDiffWatchTarget): void {
|
||||||
@@ -231,10 +259,7 @@ export class CheckoutDiffManager {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private async ensureTarget(
|
private ensureTarget(cwd: string, compare: CheckoutDiffCompareInput): CheckoutDiffWatchTarget {
|
||||||
cwd: string,
|
|
||||||
compare: CheckoutDiffCompareInput,
|
|
||||||
): Promise<CheckoutDiffWatchTarget> {
|
|
||||||
const targetKey = this.buildTargetKey(cwd, compare);
|
const targetKey = this.buildTargetKey(cwd, compare);
|
||||||
const existing = this.targets.get(targetKey);
|
const existing = this.targets.get(targetKey);
|
||||||
if (existing) {
|
if (existing) {
|
||||||
@@ -253,15 +278,22 @@ export class CheckoutDiffManager {
|
|||||||
refreshQueued: false,
|
refreshQueued: false,
|
||||||
latestPayload: null,
|
latestPayload: null,
|
||||||
latestFingerprint: null,
|
latestFingerprint: null,
|
||||||
|
openPromise: null,
|
||||||
};
|
};
|
||||||
const { repoRoot, unsubscribe } = await this.workspaceGitService.requestWorkingTreeWatch(
|
|
||||||
cwd,
|
|
||||||
() => this.scheduleTargetRefresh(target),
|
|
||||||
);
|
|
||||||
target.diffCwd = repoRoot ?? cwd;
|
|
||||||
target.workingTreeWatchUnsubscribe = unsubscribe;
|
|
||||||
|
|
||||||
this.targets.set(targetKey, target);
|
this.targets.set(targetKey, target);
|
||||||
return target;
|
return target;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private async openTarget(target: CheckoutDiffWatchTarget): Promise<void> {
|
||||||
|
const { repoRoot, unsubscribe } = await this.workspaceGitService.requestWorkingTreeWatch(
|
||||||
|
target.cwd,
|
||||||
|
() => this.scheduleTargetRefresh(target),
|
||||||
|
);
|
||||||
|
target.diffCwd = repoRoot ?? target.cwd;
|
||||||
|
if (this.targets.get(target.key) !== target || target.listeners.size === 0) {
|
||||||
|
unsubscribe();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
target.workingTreeWatchUnsubscribe = unsubscribe;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ function isTimelineResponse(msg: SessionOutboundMessage): boolean {
|
|||||||
interface FakeDiffSubscription {
|
interface FakeDiffSubscription {
|
||||||
cwd: string;
|
cwd: string;
|
||||||
compare: CheckoutDiffCompareInput;
|
compare: CheckoutDiffCompareInput;
|
||||||
listener: (snapshot: CheckoutDiffSnapshotPayload) => void;
|
emit(snapshot: CheckoutDiffSnapshotPayload): void;
|
||||||
unsubscribeCalls: number;
|
unsubscribeCalls: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -45,18 +45,29 @@ function createFakeDiffSubscriber(initial: CheckoutDiffSnapshotPayload) {
|
|||||||
const refreshedCwds: string[] = [];
|
const refreshedCwds: string[] = [];
|
||||||
const subscriber: CheckoutDiffSubscriber = {
|
const subscriber: CheckoutDiffSubscriber = {
|
||||||
subscribe: async (params, listener) => {
|
subscribe: async (params, listener) => {
|
||||||
|
let isSubscribed = true;
|
||||||
const subscription: FakeDiffSubscription = {
|
const subscription: FakeDiffSubscription = {
|
||||||
cwd: params.cwd,
|
cwd: params.cwd,
|
||||||
compare: params.compare,
|
compare: params.compare,
|
||||||
listener,
|
|
||||||
unsubscribeCalls: 0,
|
unsubscribeCalls: 0,
|
||||||
|
emit: (snapshot) => {
|
||||||
|
if (isSubscribed) {
|
||||||
|
listener(snapshot);
|
||||||
|
}
|
||||||
|
},
|
||||||
};
|
};
|
||||||
|
const unsubscribe = () => {
|
||||||
|
if (!isSubscribed) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
isSubscribed = false;
|
||||||
|
subscription.unsubscribeCalls += 1;
|
||||||
|
};
|
||||||
|
params.signal?.addEventListener("abort", unsubscribe, { once: true });
|
||||||
subscriptions.push(subscription);
|
subscriptions.push(subscription);
|
||||||
return {
|
return {
|
||||||
initial: { ...initial, cwd: params.cwd },
|
initial: { ...initial, cwd: params.cwd },
|
||||||
unsubscribe: () => {
|
unsubscribe,
|
||||||
subscription.unsubscribeCalls += 1;
|
|
||||||
},
|
|
||||||
};
|
};
|
||||||
},
|
},
|
||||||
scheduleRefreshForCwd: (cwd) => {
|
scheduleRefreshForCwd: (cwd) => {
|
||||||
@@ -453,7 +464,7 @@ describe("CheckoutSession", () => {
|
|||||||
]);
|
]);
|
||||||
expect(subscriptions).toHaveLength(1);
|
expect(subscriptions).toHaveLength(1);
|
||||||
|
|
||||||
subscriptions[0].listener({
|
subscriptions[0].emit({
|
||||||
cwd: "/repo",
|
cwd: "/repo",
|
||||||
files: [],
|
files: [],
|
||||||
error: { code: "UNKNOWN", message: "transient" },
|
error: { code: "UNKNOWN", message: "transient" },
|
||||||
|
|||||||
@@ -17,8 +17,9 @@ import type {
|
|||||||
ValidateBranchRequest,
|
ValidateBranchRequest,
|
||||||
} from "../../messages.js";
|
} from "../../messages.js";
|
||||||
import type {
|
import type {
|
||||||
CheckoutDiffCompareInput,
|
|
||||||
CheckoutDiffSnapshotPayload,
|
CheckoutDiffSnapshotPayload,
|
||||||
|
CheckoutDiffSubscription,
|
||||||
|
CheckoutDiffSubscriptionRequest,
|
||||||
} from "../../checkout-diff-manager.js";
|
} from "../../checkout-diff-manager.js";
|
||||||
import { toCheckoutError } from "../../checkout-git-utils.js";
|
import { toCheckoutError } from "../../checkout-git-utils.js";
|
||||||
import {
|
import {
|
||||||
@@ -107,9 +108,9 @@ function toLegacyGithubSearchItems(items: ForgeSearchResultItem[]): LegacyGithub
|
|||||||
*/
|
*/
|
||||||
export interface CheckoutDiffSubscriber {
|
export interface CheckoutDiffSubscriber {
|
||||||
subscribe(
|
subscribe(
|
||||||
params: { cwd: string; compare: CheckoutDiffCompareInput },
|
params: CheckoutDiffSubscriptionRequest,
|
||||||
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
listener: (snapshot: CheckoutDiffSnapshotPayload) => void,
|
||||||
): Promise<{ initial: CheckoutDiffSnapshotPayload; unsubscribe: () => void }>;
|
): Promise<CheckoutDiffSubscription>;
|
||||||
scheduleRefreshForCwd(cwd: string): void;
|
scheduleRefreshForCwd(cwd: string): void;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -400,34 +401,45 @@ export class CheckoutSession {
|
|||||||
async handleSubscribeDiffRequest(msg: SubscribeCheckoutDiffRequest): Promise<void> {
|
async handleSubscribeDiffRequest(msg: SubscribeCheckoutDiffRequest): Promise<void> {
|
||||||
const cwd = expandTilde(msg.cwd);
|
const cwd = expandTilde(msg.cwd);
|
||||||
this.diffSubscriptions.get(msg.subscriptionId)?.();
|
this.diffSubscriptions.get(msg.subscriptionId)?.();
|
||||||
this.diffSubscriptions.delete(msg.subscriptionId);
|
const abort = new AbortController();
|
||||||
const subscription = await this.checkoutDiffManager.subscribe(
|
const unsubscribe = () => abort.abort();
|
||||||
{ cwd, compare: msg.compare },
|
this.diffSubscriptions.set(msg.subscriptionId, unsubscribe);
|
||||||
(snapshot) => {
|
|
||||||
this.host.emit({
|
|
||||||
type: "checkout_diff_update",
|
|
||||||
payload: {
|
|
||||||
subscriptionId: msg.subscriptionId,
|
|
||||||
...snapshot,
|
|
||||||
},
|
|
||||||
});
|
|
||||||
},
|
|
||||||
);
|
|
||||||
this.diffSubscriptions.set(msg.subscriptionId, subscription.unsubscribe);
|
|
||||||
|
|
||||||
this.host.emit({
|
try {
|
||||||
type: "subscribe_checkout_diff_response",
|
const subscription = await this.checkoutDiffManager.subscribe(
|
||||||
payload: {
|
{ cwd, compare: msg.compare, signal: abort.signal },
|
||||||
subscriptionId: msg.subscriptionId,
|
(snapshot) => {
|
||||||
...subscription.initial,
|
this.host.emit({
|
||||||
requestId: msg.requestId,
|
type: "checkout_diff_update",
|
||||||
},
|
payload: {
|
||||||
});
|
subscriptionId: msg.subscriptionId,
|
||||||
|
...snapshot,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
this.host.emit({
|
||||||
|
type: "subscribe_checkout_diff_response",
|
||||||
|
payload: {
|
||||||
|
subscriptionId: msg.subscriptionId,
|
||||||
|
...subscription.initial,
|
||||||
|
requestId: msg.requestId,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
} catch (error) {
|
||||||
|
if (this.diffSubscriptions.get(msg.subscriptionId) === unsubscribe) {
|
||||||
|
this.diffSubscriptions.delete(msg.subscriptionId);
|
||||||
|
}
|
||||||
|
unsubscribe();
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
handleUnsubscribeDiffRequest(msg: UnsubscribeCheckoutDiffRequest): void {
|
handleUnsubscribeDiffRequest(msg: UnsubscribeCheckoutDiffRequest): void {
|
||||||
this.diffSubscriptions.get(msg.subscriptionId)?.();
|
const unsubscribe = this.diffSubscriptions.get(msg.subscriptionId);
|
||||||
this.diffSubscriptions.delete(msg.subscriptionId);
|
this.diffSubscriptions.delete(msg.subscriptionId);
|
||||||
|
unsubscribe?.();
|
||||||
}
|
}
|
||||||
|
|
||||||
async handleRefreshRequest(msg: CheckoutRefreshRequest): Promise<void> {
|
async handleRefreshRequest(msg: CheckoutRefreshRequest): Promise<void> {
|
||||||
|
|||||||
@@ -510,6 +510,45 @@ describe("checkout git utilities", () => {
|
|||||||
expect(message).toBe("update file");
|
expect(message).toBe("update file");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("reads the origin URL once when collecting facts for an origin-tracking branch", async () => {
|
||||||
|
setupRemoteTrackingMain(repoDir, tempDir);
|
||||||
|
|
||||||
|
startGitCommandMetrics();
|
||||||
|
const facts = await getCheckoutSnapshotFacts(repoDir, { paseoHome });
|
||||||
|
const metrics = stopGitCommandMetrics();
|
||||||
|
const originUrlCommands = metrics.commands.filter(
|
||||||
|
(command) => command.args.join(" ") === "config --get remote.origin.url",
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(facts.isGit).toBe(true);
|
||||||
|
expect(originUrlCommands).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("reads a non-origin branch remote without replacing it with the origin URL", async () => {
|
||||||
|
setupRemoteTrackingMain(repoDir, tempDir);
|
||||||
|
execFileSync("git", ["remote", "set-url", "origin", "git@github.com:upstream/repo.git"], {
|
||||||
|
cwd: repoDir,
|
||||||
|
});
|
||||||
|
execFileSync("git", ["remote", "add", "fork", "git@github.com:contributor/repo.git"], {
|
||||||
|
cwd: repoDir,
|
||||||
|
});
|
||||||
|
execFileSync("git", ["config", "branch.main.remote", "fork"], { cwd: repoDir });
|
||||||
|
execFileSync("git", ["config", "branch.main.merge", "refs/heads/main"], { cwd: repoDir });
|
||||||
|
|
||||||
|
startGitCommandMetrics();
|
||||||
|
const facts = await getCheckoutSnapshotFacts(repoDir, { paseoHome });
|
||||||
|
const metrics = stopGitCommandMetrics();
|
||||||
|
const commands = metrics.commands.map((command) => command.args.join(" "));
|
||||||
|
|
||||||
|
expect(facts.isGit).toBe(true);
|
||||||
|
expect(commands.filter((command) => command === "config --get remote.origin.url")).toHaveLength(
|
||||||
|
1,
|
||||||
|
);
|
||||||
|
expect(commands.filter((command) => command === "config --get remote.fork.url")).toHaveLength(
|
||||||
|
1,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("reuses checkout snapshot facts across status, shortstat, and PR status reads", async () => {
|
it("reuses checkout snapshot facts across status, shortstat, and PR status reads", async () => {
|
||||||
setupRemoteTrackingMain(repoDir, tempDir);
|
setupRemoteTrackingMain(repoDir, tempDir);
|
||||||
execFileSync("git", ["checkout", "-b", "feature/facts"], { cwd: repoDir });
|
execFileSync("git", ["checkout", "-b", "feature/facts"], { cwd: repoDir });
|
||||||
@@ -685,19 +724,34 @@ const x = 1;
|
|||||||
expect(behindStatus.aheadOfOrigin).toBe(0);
|
expect(behindStatus.aheadOfOrigin).toBe(0);
|
||||||
expect(behindStatus.behindOfOrigin).toBe(1);
|
expect(behindStatus.behindOfOrigin).toBe(1);
|
||||||
|
|
||||||
writeFileSync(join(repoDir, "local.txt"), "local\n");
|
commitFile(repoDir, "local-1.txt", "local 1\n", "local update 1");
|
||||||
execFileSync("git", ["add", "local.txt"], { cwd: repoDir });
|
commitFile(repoDir, "local-2.txt", "local 2\n", "local update 2");
|
||||||
execFileSync("git", ["-c", "commit.gpgsign=false", "commit", "-m", "local update"], {
|
commitFile(repoDir, "local-3.txt", "local 3\n", "local update 3");
|
||||||
cwd: repoDir,
|
commitFile(cloneDir, "remote-2.txt", "remote 2\n", "remote update 2");
|
||||||
});
|
execFileSync("git", ["push"], { cwd: cloneDir });
|
||||||
|
execFileSync("git", ["fetch", "origin"], { cwd: repoDir });
|
||||||
|
|
||||||
|
const facts = await getCheckoutSnapshotFacts(repoDir);
|
||||||
|
startGitCommandMetrics();
|
||||||
|
const divergedStatus = await getCheckoutStatus(repoDir, { facts });
|
||||||
|
const metrics = stopGitCommandMetrics();
|
||||||
|
const upstreamCountCommands = metrics.commands.filter(
|
||||||
|
(command) => command.args[0] === "rev-list" && command.args.join(" ").includes("main"),
|
||||||
|
);
|
||||||
|
|
||||||
const divergedStatus = await getCheckoutStatus(repoDir);
|
|
||||||
expect(divergedStatus.isGit).toBe(true);
|
expect(divergedStatus.isGit).toBe(true);
|
||||||
if (!divergedStatus.isGit) {
|
if (!divergedStatus.isGit) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
expect(divergedStatus.aheadOfOrigin).toBe(1);
|
expect(divergedStatus.aheadOfOrigin).toBe(3);
|
||||||
expect(divergedStatus.behindOfOrigin).toBe(1);
|
expect(divergedStatus.behindOfOrigin).toBe(2);
|
||||||
|
expect(upstreamCountCommands).toHaveLength(1);
|
||||||
|
expect(upstreamCountCommands[0]?.args).toEqual([
|
||||||
|
"rev-list",
|
||||||
|
"--left-right",
|
||||||
|
"--count",
|
||||||
|
"main...origin/main",
|
||||||
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("reports a PR worktree as not ahead when its branch is pushed to the configured PR remote", async () => {
|
it("reports a PR worktree as not ahead when its branch is pushed to the configured PR remote", async () => {
|
||||||
@@ -800,6 +854,7 @@ const x = 1;
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
expect(status.aheadOfOrigin).toBeNull();
|
expect(status.aheadOfOrigin).toBeNull();
|
||||||
|
expect(status.behindOfOrigin).toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not report full history as unpushed for fresh no-track Paseo worktrees", async () => {
|
it("does not report full history as unpushed for fresh no-track Paseo worktrees", async () => {
|
||||||
@@ -1419,6 +1474,26 @@ const x = 1;
|
|||||||
expect(diff.diff).toContain("# untracked-large.txt: diff too large omitted");
|
expect(diff.diff).toContain("# untracked-large.txt: diff too large omitted");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("resolves the Git common directory once when reading Paseo worktree facts", async () => {
|
||||||
|
const result = await createLegacyWorktreeForTest({
|
||||||
|
branchName: "main",
|
||||||
|
cwd: repoDir,
|
||||||
|
baseBranch: "main",
|
||||||
|
worktreeSlug: "common-dir",
|
||||||
|
paseoHome,
|
||||||
|
});
|
||||||
|
|
||||||
|
startGitCommandMetrics();
|
||||||
|
const facts = await getCheckoutSnapshotFacts(result.worktreePath, { paseoHome });
|
||||||
|
const metrics = stopGitCommandMetrics();
|
||||||
|
const commonDirCommands = metrics.commands.filter(
|
||||||
|
(command) => command.args.join(" ") === "rev-parse --git-common-dir",
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(facts.isGit).toBe(true);
|
||||||
|
expect(commonDirCommands).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
it("handles status/diff/commit in a .paseo worktree", async () => {
|
it("handles status/diff/commit in a .paseo worktree", async () => {
|
||||||
const result = await createLegacyWorktreeForTest({
|
const result = await createLegacyWorktreeForTest({
|
||||||
branchName: "main",
|
branchName: "main",
|
||||||
|
|||||||
@@ -1052,10 +1052,15 @@ type PaseoWorktreeForCwd =
|
|||||||
| { isPaseoOwnedWorktree: false }
|
| { isPaseoOwnedWorktree: false }
|
||||||
| { isPaseoOwnedWorktree: true; worktreeRoot: string };
|
| { isPaseoOwnedWorktree: true; worktreeRoot: string };
|
||||||
|
|
||||||
|
interface PaseoWorktreeLookupOptions {
|
||||||
|
context?: CheckoutContext;
|
||||||
|
knownWorktreeRoot?: string | null;
|
||||||
|
knownGitCommonDir?: string | null;
|
||||||
|
}
|
||||||
|
|
||||||
async function getPaseoWorktreeForCwd(
|
async function getPaseoWorktreeForCwd(
|
||||||
cwd: string,
|
cwd: string,
|
||||||
context?: CheckoutContext,
|
options: PaseoWorktreeLookupOptions = {},
|
||||||
knownWorktreeRoot?: string | null,
|
|
||||||
): Promise<PaseoWorktreeForCwd> {
|
): Promise<PaseoWorktreeForCwd> {
|
||||||
// Fast-path reject: non-worktree paths do not need expensive ownership checks.
|
// Fast-path reject: non-worktree paths do not need expensive ownership checks.
|
||||||
if (!/[\\/]worktrees[\\/]/.test(cwd)) {
|
if (!/[\\/]worktrees[\\/]/.test(cwd)) {
|
||||||
@@ -1063,8 +1068,9 @@ async function getPaseoWorktreeForCwd(
|
|||||||
}
|
}
|
||||||
|
|
||||||
const ownership = await isPaseoOwnedWorktreeCwd(cwd, {
|
const ownership = await isPaseoOwnedWorktreeCwd(cwd, {
|
||||||
paseoHome: context?.paseoHome,
|
paseoHome: options.context?.paseoHome,
|
||||||
worktreesRoot: context?.worktreesRoot,
|
worktreesRoot: options.context?.worktreesRoot,
|
||||||
|
knownGitCommonDir: options.knownGitCommonDir,
|
||||||
});
|
});
|
||||||
if (!ownership.allowed) {
|
if (!ownership.allowed) {
|
||||||
return { isPaseoOwnedWorktree: false };
|
return { isPaseoOwnedWorktree: false };
|
||||||
@@ -1072,7 +1078,7 @@ async function getPaseoWorktreeForCwd(
|
|||||||
|
|
||||||
return {
|
return {
|
||||||
isPaseoOwnedWorktree: true,
|
isPaseoOwnedWorktree: true,
|
||||||
worktreeRoot: knownWorktreeRoot ?? (await getWorktreeRoot(cwd, context)) ?? cwd,
|
worktreeRoot: options.knownWorktreeRoot ?? (await getWorktreeRoot(cwd, options.context)) ?? cwd,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1087,7 +1093,7 @@ async function getStoredBaseRefForCwd(
|
|||||||
if (context?.facts?.isGit) {
|
if (context?.facts?.isGit) {
|
||||||
return context.facts.storedBaseRef;
|
return context.facts.storedBaseRef;
|
||||||
}
|
}
|
||||||
const paseoWorktree = await getPaseoWorktreeForCwd(cwd, context);
|
const paseoWorktree = await getPaseoWorktreeForCwd(cwd, { context });
|
||||||
if (!paseoWorktree.isPaseoOwnedWorktree) {
|
if (!paseoWorktree.isPaseoOwnedWorktree) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
@@ -1492,30 +1498,6 @@ async function getAheadBehind(
|
|||||||
return { ahead, behind };
|
return { ahead, behind };
|
||||||
}
|
}
|
||||||
|
|
||||||
async function getAheadOfOrigin(
|
|
||||||
cwd: string,
|
|
||||||
currentBranch: string,
|
|
||||||
context?: CheckoutContext,
|
|
||||||
): Promise<number | null> {
|
|
||||||
if (!currentBranch) {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
const upstreamRef = await getConfiguredUpstreamRef(cwd, currentBranch, context);
|
|
||||||
if (!upstreamRef) {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
try {
|
|
||||||
const { stdout } = await runGitCommand(
|
|
||||||
["rev-list", "--count", `${upstreamRef}..${currentBranch}`],
|
|
||||||
{ cwd, envOverlay: READ_ONLY_GIT_ENV, logger: context?.logger },
|
|
||||||
);
|
|
||||||
const count = Number.parseInt(stdout.trim(), 10);
|
|
||||||
return Number.isNaN(count) ? null : count;
|
|
||||||
} catch {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
async function getConfiguredUpstreamRef(
|
async function getConfiguredUpstreamRef(
|
||||||
cwd: string,
|
cwd: string,
|
||||||
currentBranch: string,
|
currentBranch: string,
|
||||||
@@ -1537,11 +1519,11 @@ async function getConfiguredUpstreamRef(
|
|||||||
return upstreamBranch ? `${remoteName}/${upstreamBranch}` : null;
|
return upstreamBranch ? `${remoteName}/${upstreamBranch}` : null;
|
||||||
}
|
}
|
||||||
|
|
||||||
async function getBehindOfOrigin(
|
async function getOriginAheadBehind(
|
||||||
cwd: string,
|
cwd: string,
|
||||||
currentBranch: string,
|
currentBranch: string,
|
||||||
context?: CheckoutContext,
|
context?: CheckoutContext,
|
||||||
): Promise<number | null> {
|
): Promise<AheadBehind | null> {
|
||||||
if (!currentBranch) {
|
if (!currentBranch) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
@@ -1551,11 +1533,13 @@ async function getBehindOfOrigin(
|
|||||||
}
|
}
|
||||||
try {
|
try {
|
||||||
const { stdout } = await runGitCommand(
|
const { stdout } = await runGitCommand(
|
||||||
["rev-list", "--count", `${currentBranch}..${upstreamRef}`],
|
["rev-list", "--left-right", "--count", `${currentBranch}...${upstreamRef}`],
|
||||||
{ cwd, envOverlay: READ_ONLY_GIT_ENV, logger: context?.logger },
|
{ cwd, envOverlay: READ_ONLY_GIT_ENV, logger: context?.logger },
|
||||||
);
|
);
|
||||||
const count = Number.parseInt(stdout.trim(), 10);
|
const [aheadRaw, behindRaw] = stdout.trim().split(/\s+/);
|
||||||
return Number.isNaN(count) ? null : count;
|
const ahead = Number.parseInt(aheadRaw ?? "", 10);
|
||||||
|
const behind = Number.parseInt(behindRaw ?? "", 10);
|
||||||
|
return Number.isNaN(ahead) || Number.isNaN(behind) ? null : { ahead, behind };
|
||||||
} catch {
|
} catch {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
@@ -1579,15 +1563,17 @@ async function inspectCheckoutContext(
|
|||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
const [currentBranch, remoteUrl, absoluteGitDir, gitCommonDir, paseoWorktree] = await Promise.all(
|
const [currentBranch, remoteUrl, absoluteGitDir, gitCommonDir] = await Promise.all([
|
||||||
[
|
getCurrentBranch(cwd),
|
||||||
getCurrentBranch(cwd),
|
getOriginRemoteUrl(cwd),
|
||||||
getOriginRemoteUrl(cwd),
|
resolveAbsoluteGitDir(cwd),
|
||||||
resolveAbsoluteGitDir(cwd),
|
resolveGitCommonDir(cwd),
|
||||||
resolveGitCommonDir(cwd),
|
]);
|
||||||
getPaseoWorktreeForCwd(cwd, context, root),
|
const paseoWorktree = await getPaseoWorktreeForCwd(cwd, {
|
||||||
],
|
context,
|
||||||
);
|
knownWorktreeRoot: root,
|
||||||
|
knownGitCommonDir: gitCommonDir,
|
||||||
|
});
|
||||||
|
|
||||||
return {
|
return {
|
||||||
worktreeRoot: root,
|
worktreeRoot: root,
|
||||||
@@ -1776,7 +1762,9 @@ export async function getCheckoutSnapshotFacts(
|
|||||||
if (branchRemoteName) {
|
if (branchRemoteName) {
|
||||||
[branchMergeRef, branchRemoteUrl] = await Promise.all([
|
[branchMergeRef, branchRemoteUrl] = await Promise.all([
|
||||||
getGitConfigValue(cwd, `branch.${inspected.currentBranch}.merge`, context),
|
getGitConfigValue(cwd, `branch.${inspected.currentBranch}.merge`, context),
|
||||||
getGitConfigValue(cwd, `remote.${branchRemoteName}.url`, context),
|
branchRemoteName === "origin"
|
||||||
|
? inspected.remoteUrl
|
||||||
|
: getGitConfigValue(cwd, `remote.${branchRemoteName}.url`, context),
|
||||||
]);
|
]);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1955,17 +1943,16 @@ export async function getCheckoutStatus(
|
|||||||
const baseRef = facts.resolvedBaseRef;
|
const baseRef = facts.resolvedBaseRef;
|
||||||
const mainRepoRoot = facts.mainRepoRoot;
|
const mainRepoRoot = facts.mainRepoRoot;
|
||||||
const factsContext = { ...context, facts };
|
const factsContext = { ...context, facts };
|
||||||
const [aheadBehind, aheadOfOrigin, behindOfOrigin] = await Promise.all([
|
const [aheadBehind, originAheadBehind] = await Promise.all([
|
||||||
baseRef && currentBranch
|
baseRef && currentBranch
|
||||||
? getAheadBehind(cwd, baseRef, currentBranch, factsContext)
|
? getAheadBehind(cwd, baseRef, currentBranch, factsContext)
|
||||||
: Promise.resolve(null),
|
: Promise.resolve(null),
|
||||||
hasRemote && currentBranch
|
hasRemote && currentBranch
|
||||||
? getAheadOfOrigin(cwd, currentBranch, factsContext)
|
? getOriginAheadBehind(cwd, currentBranch, factsContext)
|
||||||
: Promise.resolve(null),
|
|
||||||
hasRemote && currentBranch
|
|
||||||
? getBehindOfOrigin(cwd, currentBranch, factsContext)
|
|
||||||
: Promise.resolve(null),
|
: Promise.resolve(null),
|
||||||
]);
|
]);
|
||||||
|
const aheadOfOrigin = originAheadBehind?.ahead ?? null;
|
||||||
|
const behindOfOrigin = originAheadBehind?.behind ?? null;
|
||||||
|
|
||||||
if (paseoWorktree.isPaseoOwnedWorktree && baseRef) {
|
if (paseoWorktree.isPaseoOwnedWorktree && baseRef) {
|
||||||
return {
|
return {
|
||||||
|
|||||||
@@ -155,6 +155,10 @@ export interface PaseoWorktreeOwnership {
|
|||||||
worktreePath?: string;
|
worktreePath?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export interface PaseoWorktreeOwnershipOptions extends WorktreeRootOptions {
|
||||||
|
knownGitCommonDir?: string | null;
|
||||||
|
}
|
||||||
|
|
||||||
export interface WorktreeRootOptions {
|
export interface WorktreeRootOptions {
|
||||||
paseoHome?: string;
|
paseoHome?: string;
|
||||||
worktreesRoot?: string;
|
worktreesRoot?: string;
|
||||||
@@ -913,7 +917,7 @@ function resolveRepoRootFromGitCommonDir(commonDir: string): string {
|
|||||||
|
|
||||||
export async function isPaseoOwnedWorktreeCwd(
|
export async function isPaseoOwnedWorktreeCwd(
|
||||||
cwd: string,
|
cwd: string,
|
||||||
options?: WorktreeRootOptions,
|
options?: PaseoWorktreeOwnershipOptions,
|
||||||
): Promise<PaseoWorktreeOwnership> {
|
): Promise<PaseoWorktreeOwnership> {
|
||||||
const resolvedCwd = normalizePathForOwnership(cwd);
|
const resolvedCwd = normalizePathForOwnership(cwd);
|
||||||
|
|
||||||
@@ -921,11 +925,15 @@ export async function isPaseoOwnedWorktreeCwd(
|
|||||||
// previous archive attempt removed the admin dir before the working tree
|
// previous archive attempt removed the admin dir before the working tree
|
||||||
// could be fully cleaned up). We still want to allow archiving in that case.
|
// could be fully cleaned up). We still want to allow archiving in that case.
|
||||||
let repoRoot: string | undefined;
|
let repoRoot: string | undefined;
|
||||||
try {
|
if (options?.knownGitCommonDir) {
|
||||||
const gitCommonDir = await getGitCommonDir(cwd);
|
repoRoot = resolveRepoRootFromGitCommonDir(options.knownGitCommonDir);
|
||||||
repoRoot = resolveRepoRootFromGitCommonDir(gitCommonDir);
|
} else if (options?.knownGitCommonDir === undefined) {
|
||||||
} catch {
|
try {
|
||||||
// ignore
|
const gitCommonDir = await getGitCommonDir(cwd);
|
||||||
|
repoRoot = resolveRepoRootFromGitCommonDir(gitCommonDir);
|
||||||
|
} catch {
|
||||||
|
// ignore
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
const worktreesBaseRoot = resolvePaseoWorktreesBaseRoot(options);
|
const worktreesBaseRoot = resolvePaseoWorktreesBaseRoot(options);
|
||||||
|
|||||||
Reference in New Issue
Block a user