Skip to content

Commit c13611d

Browse files
committed
Avoids git cache polution on errors
- Adds `PromiseMap` to automatically remove failed promises from cache - Replaces manual cache management patterns with getOrCreate method
1 parent 6a9e3bc commit c13611d

File tree

13 files changed

+721
-670
lines changed

13 files changed

+721
-670
lines changed

src/env/node/git/sub-providers/branches.ts

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import { getLogScope } from '../../../../system/logger.scope';
3838
import { PageableResult } from '../../../../system/paging';
3939
import { normalizePath } from '../../../../system/path';
4040
import { getSettledValue } from '../../../../system/promise';
41+
import { PromiseMap } from '../../../../system/promiseCache';
4142
import { maybeStopWatch } from '../../../../system/stopwatch';
4243
import type { Git } from '../git';
4344
import { gitConfigsLog, GitError, GitErrors } from '../git';
@@ -62,17 +63,18 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
6263
return branch;
6364
}
6465

65-
let branchPromise = this.cache.branch?.get(repoPath);
66-
if (branchPromise == null) {
67-
async function load(this: BranchesGitSubProvider): Promise<GitBranch | undefined> {
68-
const {
69-
values: [branch],
70-
} = await this.getBranches(repoPath, { filter: b => b.current }, cancellation);
71-
return branch ?? this.getCurrentBranch(repoPath, cancellation);
72-
}
66+
const branchPromise = this.cache.branch?.getOrCreate(repoPath, async () => {
67+
const {
68+
values: [branch],
69+
} = await this.getBranches(repoPath, { filter: b => b.current }, cancellation);
70+
return branch ?? this.getCurrentBranch(repoPath, cancellation);
71+
});
7372

74-
branchPromise = load.call(this);
75-
this.cache.branch?.set(repoPath, branchPromise);
73+
if (branchPromise == null) {
74+
const {
75+
values: [branch],
76+
} = await this.getBranches(repoPath, { filter: b => b.current }, cancellation);
77+
return branch ?? this.getCurrentBranch(repoPath, cancellation);
7678
}
7779

7880
return branchPromise;
@@ -421,22 +423,16 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
421423

422424
remote ??= 'origin';
423425

424-
const cacheByRemote = this.cache.defaultBranchName?.get(repoPath);
425-
let promise = cacheByRemote?.get(remote);
426-
if (promise == null) {
427-
async function load(this: BranchesGitSubProvider): Promise<string | undefined> {
428-
return this.git.symbolic_ref__HEAD(repoPath!, remote!, cancellation);
429-
}
430-
431-
promise = load.call(this);
432-
433-
if (cacheByRemote == null) {
434-
this.cache.defaultBranchName?.set(repoPath, new Map([[remote, promise]]));
435-
} else {
436-
cacheByRemote.set(remote, promise);
437-
}
426+
let cacheByRemote = this.cache.defaultBranchName?.get(repoPath);
427+
if (cacheByRemote == null) {
428+
cacheByRemote = new PromiseMap<string, string | undefined>();
429+
this.cache.defaultBranchName?.set(repoPath, cacheByRemote);
438430
}
439431

432+
const promise = cacheByRemote.getOrCreate(remote, async () => {
433+
return this.git.symbolic_ref__HEAD(repoPath, remote, cancellation);
434+
});
435+
440436
return promise;
441437
}
442438

src/env/node/git/sub-providers/contributors.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export class ContributorsGitSubProvider implements GitContributorsSubProvider {
9494

9595
for await (const c of parser.parseAsync(stream)) {
9696
if (signal?.aborted || cancellation?.isCancellationRequested) {
97-
cancellable?.cancel();
97+
cancellable?.cancelled();
9898
break;
9999
}
100100

@@ -175,7 +175,7 @@ export class ContributorsGitSubProvider implements GitContributorsSubProvider {
175175
: undefined,
176176
};
177177
} catch (ex) {
178-
cancellable?.cancel();
178+
cancellable?.cancelled();
179179
Logger.error(ex, scope);
180180
debugger;
181181

@@ -274,7 +274,7 @@ export class ContributorsGitSubProvider implements GitContributorsSubProvider {
274274
const shortlog = parseShortlog(result.stdout, repoPath, await currentUserPromise);
275275
return shortlog.contributors;
276276
} catch (ex) {
277-
cancellable?.cancel();
277+
cancellable?.cancelled();
278278
Logger.error(ex, scope);
279279
debugger;
280280

src/env/node/git/sub-providers/remotes.ts

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -35,34 +35,29 @@ export class RemotesGitSubProvider extends RemotesGitProviderBase implements Git
3535

3636
const scope = getLogScope();
3737

38-
let remotesPromise = this.cache.remotes?.get(repoPath);
39-
if (remotesPromise == null) {
40-
async function load(this: RemotesGitSubProvider): Promise<GitRemote[]> {
41-
const providers = loadRemoteProviders(
42-
configuration.get('remotes', this.container.git.getRepository(repoPath!)?.folder?.uri ?? null),
43-
await this.container.integrations.getConfigured(),
44-
);
38+
const remotesPromise = this.cache.remotes?.getOrCreate(repoPath, async cancellable => {
39+
const providers = loadRemoteProviders(
40+
configuration.get('remotes', this.container.git.getRepository(repoPath)?.folder?.uri ?? null),
41+
await this.container.integrations.getConfigured(),
42+
);
4543

46-
try {
47-
const result = await this.git.exec({ cwd: repoPath }, 'remote', '-v');
48-
const remotes = parseGitRemotes(
49-
this.container,
50-
result.stdout,
51-
repoPath!,
52-
await getRemoteProviderMatcher(this.container, providers),
53-
);
54-
return remotes;
55-
} catch (ex) {
56-
this.cache.remotes?.delete(repoPath!);
57-
Logger.error(ex, scope);
58-
return [];
59-
}
44+
try {
45+
const result = await this.git.exec({ cwd: repoPath }, 'remote', '-v');
46+
const remotes = parseGitRemotes(
47+
this.container,
48+
result.stdout,
49+
repoPath,
50+
await getRemoteProviderMatcher(this.container, providers),
51+
);
52+
return remotes;
53+
} catch (ex) {
54+
cancellable.cancelled();
55+
Logger.error(ex, scope);
56+
return [];
6057
}
58+
});
6159

62-
remotesPromise = load.call(this);
63-
64-
this.cache.remotes?.set(repoPath, remotesPromise);
65-
}
60+
if (remotesPromise == null) return [];
6661

6762
let remotes = await remotesPromise;
6863
if (options?.filter != null) {

src/env/node/git/sub-providers/stash.ts

Lines changed: 62 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -77,87 +77,78 @@ export class StashGitSubProvider implements GitStashSubProvider {
7777
): Promise<GitStash | undefined> {
7878
if (repoPath == null) return undefined;
7979

80-
let stashPromise = this.cache.stashes?.get(repoPath);
81-
if (stashPromise == null) {
82-
async function load(this: StashGitSubProvider): Promise<GitStash> {
83-
const parser = getStashLogParser();
84-
const args = [...parser.arguments];
85-
86-
const similarityThreshold = configuration.get('advanced.similarityThreshold');
87-
args.push(`-M${similarityThreshold == null ? '' : `${similarityThreshold}%`}`);
88-
89-
const result = await this.git.exec(
90-
{ cwd: repoPath, cancellation: cancellation },
91-
'stash',
92-
'list',
93-
...args,
94-
);
95-
96-
const stashes = new Map<string, GitStashCommit>();
97-
const parentShas = new Set<string>();
98-
99-
// First pass: create stashes and collect parent SHAs
100-
for (const s of parser.parse(result.stdout)) {
101-
stashes.set(s.sha, createStash(this.container, s, repoPath));
102-
// Collect all parent SHAs for timestamp lookup
103-
if (s.parents) {
104-
for (const parentSha of s.parents.split(' ')) {
105-
if (parentSha.trim()) {
106-
parentShas.add(parentSha.trim());
107-
}
80+
const stashPromise = this.cache.stashes?.getOrCreate(repoPath, async _cancellable => {
81+
const parser = getStashLogParser();
82+
const args = [...parser.arguments];
83+
84+
const similarityThreshold = configuration.get('advanced.similarityThreshold');
85+
args.push(`-M${similarityThreshold == null ? '' : `${similarityThreshold}%`}`);
86+
87+
const result = await this.git.exec({ cwd: repoPath, cancellation: cancellation }, 'stash', 'list', ...args);
88+
89+
const stashes = new Map<string, GitStashCommit>();
90+
const parentShas = new Set<string>();
91+
92+
// First pass: create stashes and collect parent SHAs
93+
for (const s of parser.parse(result.stdout)) {
94+
stashes.set(s.sha, createStash(this.container, s, repoPath));
95+
// Collect all parent SHAs for timestamp lookup
96+
if (s.parents) {
97+
for (const parentSha of s.parents.split(' ')) {
98+
if (parentSha.trim()) {
99+
parentShas.add(parentSha.trim());
108100
}
109101
}
110102
}
103+
}
111104

112-
// Second pass: fetch parent timestamps if we have any parents
113-
const parentTimestamps = new Map<string, { authorDate: number; committerDate: number }>();
114-
if (parentShas.size > 0) {
115-
try {
116-
const datesParser = getShaAndDatesLogParser();
117-
const parentResult = await this.git.exec(
118-
{
119-
cwd: repoPath,
120-
cancellation: cancellation,
121-
stdin: Array.from(parentShas).join('\n'),
122-
},
123-
'log',
124-
...datesParser.arguments,
125-
'--no-walk',
126-
'--stdin',
127-
);
128-
129-
for (const entry of datesParser.parse(parentResult.stdout)) {
130-
parentTimestamps.set(entry.sha, {
131-
authorDate: Number(entry.authorDate),
132-
committerDate: Number(entry.committerDate),
133-
});
134-
}
135-
} catch (_ex) {
136-
// If we can't get parent timestamps, continue without them
137-
// This could happen if some parent commits are not available
105+
// Second pass: fetch parent timestamps if we have any parents
106+
const parentTimestamps = new Map<string, { authorDate: number; committerDate: number }>();
107+
if (parentShas.size > 0) {
108+
try {
109+
const datesParser = getShaAndDatesLogParser();
110+
const parentResult = await this.git.exec(
111+
{
112+
cwd: repoPath,
113+
cancellation: cancellation,
114+
stdin: Array.from(parentShas).join('\n'),
115+
},
116+
'log',
117+
...datesParser.arguments,
118+
'--no-walk',
119+
'--stdin',
120+
);
121+
122+
for (const entry of datesParser.parse(parentResult.stdout)) {
123+
parentTimestamps.set(entry.sha, {
124+
authorDate: Number(entry.authorDate),
125+
committerDate: Number(entry.committerDate),
126+
});
138127
}
128+
} catch (_ex) {
129+
// If we can't get parent timestamps, continue without them
130+
// This could happen if some parent commits are not available
139131
}
132+
}
140133

141-
// Third pass: update stashes with parent timestamp information
142-
for (const sha of stashes.keys()) {
143-
const stash = stashes.get(sha);
144-
if (stash?.parents.length) {
145-
const parentsWithTimestamps: GitStashParentInfo[] = stash.parents.map(parentSha => ({
146-
sha: parentSha,
147-
authorDate: parentTimestamps.get(parentSha)?.authorDate,
148-
committerDate: parentTimestamps.get(parentSha)?.committerDate,
149-
}));
150-
// Store the parent timestamp information on the stash
151-
stashes.set(sha, stash.with({ parentTimestamps: parentsWithTimestamps }));
152-
}
134+
// Third pass: update stashes with parent timestamp information
135+
for (const sha of stashes.keys()) {
136+
const stash = stashes.get(sha);
137+
if (stash?.parents.length) {
138+
const parentsWithTimestamps: GitStashParentInfo[] = stash.parents.map(parentSha => ({
139+
sha: parentSha,
140+
authorDate: parentTimestamps.get(parentSha)?.authorDate,
141+
committerDate: parentTimestamps.get(parentSha)?.committerDate,
142+
}));
143+
// Store the parent timestamp information on the stash
144+
stashes.set(sha, stash.with({ parentTimestamps: parentsWithTimestamps }));
153145
}
154-
155-
return { repoPath: repoPath, stashes: stashes };
156146
}
157147

158-
stashPromise = load.call(this);
159-
this.cache.stashes?.set(repoPath, stashPromise);
160-
}
148+
return { repoPath: repoPath, stashes: stashes };
149+
});
150+
151+
if (stashPromise == null) return undefined;
161152

162153
const stash = await stashPromise;
163154
if (!options?.reachableFrom || !stash?.stashes.size) return stash;

0 commit comments

Comments
 (0)