Skip to content

Commit c21e347

Browse files
bpaserobpasero
committed
Two similar local history entries show up on renaming a file (fix microsoft#145881)
1 parent 001fb95 commit c21e347

File tree

5 files changed

+223
-39
lines changed

5 files changed

+223
-39
lines changed

‎src/vs/workbench/services/workingCopy/common/workingCopyHistory.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,14 @@ export interface IWorkingCopyHistoryService {
120120
*/
121121
removeEntry(entry: IWorkingCopyHistoryEntry, token: CancellationToken): Promise<boolean>;
122122

123+
/**
124+
* Moves entries that either match the `source` or are a child
125+
* of `source` to the `target`.
126+
*
127+
* @returns a list of resources for entries that have moved.
128+
*/
129+
moveEntries(source: URI, target: URI): Promise<URI[]>;
130+
123131
/**
124132
* Gets all history entries for the provided resource.
125133
*/

‎src/vs/workbench/services/workingCopy/common/workingCopyHistoryService.ts

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { LifecyclePhase } from 'vs/workbench/services/lifecycle/common/lifecycle
1212
import { WorkingCopyHistoryTracker } from 'vs/workbench/services/workingCopy/common/workingCopyHistoryTracker';
1313
import { Disposable } from 'vs/base/common/lifecycle';
1414
import { IWorkingCopyHistoryEntry, IWorkingCopyHistoryEntryDescriptor, IWorkingCopyHistoryEvent, IWorkingCopyHistoryService, MAX_PARALLEL_HISTORY_IO_OPS } from 'vs/workbench/services/workingCopy/common/workingCopyHistory';
15-
import { FileOperation, FileOperationError, FileOperationEvent, FileOperationResult, IFileService, IFileStatWithMetadata } from 'vs/platform/files/common/files';
15+
import { FileOperationError, FileOperationResult, IFileService, IFileStatWithMetadata } from 'vs/platform/files/common/files';
1616
import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService';
1717
import { URI } from 'vs/base/common/uri';
1818
import { DeferredPromise, Limiter } from 'vs/base/common/async';
@@ -525,8 +525,6 @@ export abstract class WorkingCopyHistoryService extends Disposable implements IW
525525
super();
526526

527527
this.resolveLocalHistoryHome();
528-
529-
this._register(this.fileService.onDidRunOperation(e => this.onDidRunFileOperation(e)));
530528
}
531529

532530
private async resolveLocalHistoryHome(): Promise<void> {
@@ -550,26 +548,18 @@ export abstract class WorkingCopyHistoryService extends Disposable implements IW
550548
this.localHistoryHome.complete(historyHome);
551549
}
552550

553-
private async onDidRunFileOperation(e: FileOperationEvent): Promise<void> {
554-
if (!e.isOperation(FileOperation.MOVE)) {
555-
return; // only interested in move operations
556-
}
557-
558-
const source = e.resource;
559-
const target = e.target.resource;
560-
561-
const limiter = new Limiter(MAX_PARALLEL_HISTORY_IO_OPS);
562-
const promises = [];
551+
async moveEntries(source: URI, target: URI): Promise<URI[]> {
552+
const limiter = new Limiter<URI>(MAX_PARALLEL_HISTORY_IO_OPS);
553+
const promises: Promise<URI>[] = [];
563554

564555
for (const [resource, model] of this.models) {
565556
if (!this.uriIdentityService.extUri.isEqualOrParent(resource, source)) {
566557
continue; // model does not match moved resource
567558
}
568559

569-
570560
// Determine new resulting target resource
571561
let targetResource: URI;
572-
if (isEqual(source, resource)) {
562+
if (this.uriIdentityService.extUri.isEqual(source, resource)) {
573563
targetResource = target; // file got moved
574564
} else {
575565
const index = indexOfPath(resource.path, source.path);
@@ -578,35 +568,39 @@ export abstract class WorkingCopyHistoryService extends Disposable implements IW
578568

579569
// Figure out save source
580570
let saveSource: SaveSource;
581-
if (isEqual(dirname(resource), dirname(targetResource))) {
571+
if (this.uriIdentityService.extUri.isEqual(dirname(resource), dirname(targetResource))) {
582572
saveSource = WorkingCopyHistoryService.FILE_RENAMED_SOURCE;
583573
} else {
584574
saveSource = WorkingCopyHistoryService.FILE_MOVED_SOURCE;
585575
}
586576

587577
// Move entries to target queued
588-
promises.push(limiter.queue(() => this.moveEntries(model, saveSource, resource, targetResource)));
578+
promises.push(limiter.queue(() => this.doMoveEntries(model, saveSource, resource, targetResource)));
589579
}
590580

591581
if (!promises.length) {
592-
return;
582+
return [];
593583
}
594584

595585
// Await move operations
596-
await Promise.all(promises);
586+
const resources = await Promise.all(promises);
597587

598588
// Events
599589
this._onDidMoveEntries.fire();
590+
591+
return resources;
600592
}
601593

602-
private async moveEntries(model: WorkingCopyHistoryModel, source: SaveSource, sourceWorkingCopyResource: URI, targetWorkingCopyResource: URI): Promise<void> {
594+
private async doMoveEntries(model: WorkingCopyHistoryModel, source: SaveSource, sourceWorkingCopyResource: URI, targetWorkingCopyResource: URI): Promise<URI> {
603595

604596
// Move to target via model
605597
await model.moveEntries(targetWorkingCopyResource, source, CancellationToken.None);
606598

607599
// Update model in our map
608600
this.models.delete(sourceWorkingCopyResource);
609601
this.models.set(targetWorkingCopyResource, model);
602+
603+
return targetWorkingCopyResource;
610604
}
611605

612606
async addEntry({ resource, source, timestamp }: IWorkingCopyHistoryEntryDescriptor, token: CancellationToken): Promise<IWorkingCopyHistoryEntry | undefined> {

‎src/vs/workbench/services/workingCopy/common/workingCopyHistoryTracker.ts

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { localize } from 'vs/nls';
7+
import { URI } from 'vs/base/common/uri';
78
import { IdleValue, Limiter } from 'vs/base/common/async';
89
import { CancellationTokenSource } from 'vs/base/common/cancellation';
910
import { Disposable } from 'vs/base/common/lifecycle';
@@ -22,6 +23,7 @@ import { IWorkingCopySaveEvent, IWorkingCopyService } from 'vs/workbench/service
2223
import { Schemas } from 'vs/base/common/network';
2324
import { ResourceGlobMatcher } from 'vs/workbench/common/resources';
2425
import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
26+
import { FileOperation, FileOperationEvent, IFileService } from 'vs/platform/files/common/files';
2527

2628
export class WorkingCopyHistoryTracker extends Disposable implements IWorkbenchContribution {
2729

@@ -58,7 +60,8 @@ export class WorkingCopyHistoryTracker extends Disposable implements IWorkbenchC
5860
@IPathService private readonly pathService: IPathService,
5961
@IConfigurationService private readonly configurationService: IConfigurationService,
6062
@IUndoRedoService private readonly undoRedoService: IUndoRedoService,
61-
@IWorkspaceContextService private readonly contextService: IWorkspaceContextService
63+
@IWorkspaceContextService private readonly contextService: IWorkspaceContextService,
64+
@IFileService private readonly fileService: IFileService
6265
) {
6366
super();
6467

@@ -67,28 +70,52 @@ export class WorkingCopyHistoryTracker extends Disposable implements IWorkbenchC
6770

6871
private registerListeners() {
6972

70-
// Working Copy events
73+
// File Events
74+
this._register(this.fileService.onDidRunOperation(e => this.onDidRunFileOperation(e)));
75+
76+
// Working Copy Events
7177
this._register(this.workingCopyService.onDidChangeContent(workingCopy => this.onDidChangeContent(workingCopy)));
7278
this._register(this.workingCopyService.onDidSave(e => this.onDidSave(e)));
7379
}
7480

81+
private async onDidRunFileOperation(e: FileOperationEvent): Promise<void> {
82+
if (!e.isOperation(FileOperation.MOVE)) {
83+
return; // only interested in move operations
84+
}
85+
86+
const source = e.resource;
87+
const target = e.target.resource;
88+
89+
// Move working copy history entries for this file move event
90+
const resources = await this.workingCopyHistoryService.moveEntries(source, target);
91+
92+
// Make sure to track the content version of each entry that
93+
// was moved in our map. This ensures that a subsequent save
94+
// without a content change does not add a redundant entry
95+
// (https://github.com/microsoft/vscode/issues/145881)
96+
for (const resource of resources) {
97+
const contentVersion = this.getContentVersion(resource);
98+
this.historyEntryContentVersion.set(resource, contentVersion);
99+
}
100+
}
101+
75102
private onDidChangeContent(workingCopy: IWorkingCopy): void {
76103

77104
// Increment content version ID for resource
78-
const contentVersionId = this.getContentVersion(workingCopy);
105+
const contentVersionId = this.getContentVersion(workingCopy.resource);
79106
this.workingCopyContentVersion.set(workingCopy.resource, contentVersionId + 1);
80107
}
81108

82-
private getContentVersion(workingCopy: IWorkingCopy): number {
83-
return this.workingCopyContentVersion.get(workingCopy.resource) || 0;
109+
private getContentVersion(resource: URI): number {
110+
return this.workingCopyContentVersion.get(resource) || 0;
84111
}
85112

86113
private onDidSave(e: IWorkingCopySaveEvent): void {
87114
if (!this.shouldTrackHistory(e)) {
88115
return; // return early for working copies we are not interested in
89116
}
90117

91-
const contentVersion = this.getContentVersion(e.workingCopy);
118+
const contentVersion = this.getContentVersion(e.workingCopy.resource);
92119
if (this.historyEntryContentVersion.get(e.workingCopy.resource) === contentVersion) {
93120
return; // return early when content version already has associated history entry
94121
}
@@ -106,7 +133,7 @@ export class WorkingCopyHistoryTracker extends Disposable implements IWorkbenchC
106133
return;
107134
}
108135

109-
const contentVersion = this.getContentVersion(e.workingCopy);
136+
const contentVersion = this.getContentVersion(e.workingCopy.resource);
110137

111138
// Figure out source of save operation if not provided already
112139
let source = e.source;

‎src/vs/workbench/services/workingCopy/test/electron-browser/workingCopyHistoryService.test.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import * as assert from 'assert';
7-
import { Event } from 'vs/base/common/event';
87
import { NativeWorkbenchEnvironmentService } from 'vs/workbench/services/environment/electron-sandbox/environmentService';
98
import { TestNativePathService, TestNativeWindowConfiguration } from 'vs/workbench/test/electron-browser/workbenchTestServices';
109
import { TestContextService, TestProductService, TestWorkingCopy } from 'vs/workbench/test/common/workbenchTestServices';
@@ -662,9 +661,7 @@ flakySuite('WorkingCopyHistoryService', () => {
662661
assert.strictEqual(entries.length, 3);
663662
});
664663

665-
test('entries moved (file rename)', async () => {
666-
const entriesMoved = Event.toPromise(service.onDidMoveEntries);
667-
664+
test('move entries (file rename)', async () => {
668665
const workingCopy = new TestWorkingCopy(URI.file(testFile1Path));
669666

670667
const entry1 = await addEntry({ resource: workingCopy.resource, source: 'test-source' }, CancellationToken.None);
@@ -677,7 +674,10 @@ flakySuite('WorkingCopyHistoryService', () => {
677674
const renamedWorkingCopyResource = joinPath(resourcesDirname(workingCopy.resource), 'renamed.txt');
678675
await service._fileService.move(workingCopy.resource, renamedWorkingCopyResource);
679676

680-
await entriesMoved;
677+
const result = await service.moveEntries(workingCopy.resource, renamedWorkingCopyResource);
678+
679+
assert.strictEqual(result.length, 1);
680+
assert.strictEqual(result[0].toString(), renamedWorkingCopyResource.toString());
681681

682682
entries = await service.getEntries(workingCopy.resource, CancellationToken.None);
683683
assert.strictEqual(entries.length, 0);
@@ -709,8 +709,6 @@ flakySuite('WorkingCopyHistoryService', () => {
709709
});
710710

711711
test('entries moved (folder rename)', async () => {
712-
const entriesMoved = Event.toPromise(service.onDidMoveEntries);
713-
714712
const workingCopy1 = new TestWorkingCopy(URI.file(testFile1Path));
715713
const workingCopy2 = new TestWorkingCopy(URI.file(testFile2Path));
716714

@@ -731,10 +729,17 @@ flakySuite('WorkingCopyHistoryService', () => {
731729
const renamedWorkHome = joinPath(resourcesDirname(URI.file(workHome)), 'renamed');
732730
await service._fileService.move(URI.file(workHome), renamedWorkHome);
733731

732+
const resources = await service.moveEntries(URI.file(workHome), renamedWorkHome);
733+
734734
const renamedWorkingCopy1Resource = joinPath(renamedWorkHome, basename(workingCopy1.resource));
735735
const renamedWorkingCopy2Resource = joinPath(renamedWorkHome, basename(workingCopy2.resource));
736736

737-
await entriesMoved;
737+
assert.strictEqual(resources.length, 2);
738+
for (const resource of resources) {
739+
if (resource.toString() !== renamedWorkingCopy1Resource.toString() && resource.toString() !== renamedWorkingCopy2Resource.toString()) {
740+
assert.fail(`Unexpected history resource: ${resource.toString()}`);
741+
}
742+
}
738743

739744
entries = await service.getEntries(workingCopy1.resource, CancellationToken.None);
740745
assert.strictEqual(entries.length, 0);

0 commit comments

Comments
 (0)