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.

Sign up for GitHub

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

Allow ResourceCommandResolver.getRightResource() to return undefined #113364

Merged
merged 9 commits into from
Jan 5, 2021
Merged

Allow ResourceCommandResolver.getRightResource() to return undefined #113364

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3cf9d54
Update repository.ts
infinnie Dec 24, 2020
5c02a58
Update repository.ts
infinnie Dec 24, 2020
dfaf190
Update smoke.test.ts
infinnie Dec 25, 2020
9bc15e8
Update smoke.test.ts
infinnie Dec 25, 2020
2f72514
Update smoke.test.ts
infinnie Dec 25, 2020
b288320
Merge pull request #3 from infinnie/patch-2
infinnie Dec 25, 2020
a322d25
Update smoke.test.ts
infinnie Dec 25, 2020
b97aa86
Merge pull request #4 from infinnie/patch-2
infinnie Dec 25, 2020
0a5d6b0
Adds undefined
Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions extensions/git/src/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@ export class Resource implements SourceControlResourceState {
}

get rightUri(): Uri {
return this.resources[1];
return this.resources[1] as Uri;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea -- this should either return Uri | undefined or it should throw.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and adding undefined doesn't (visibly) cause any issues.

}

get command(): Command {
return this._commandResolver.resolveDefaultCommand(this);
}

@memoize
private get resources(): [Uri | undefined, Uri] {
private get resources(): [Uri | undefined, Uri | undefined] {
return this._commandResolver.getResources(this);
}

Expand Down Expand Up @@ -613,7 +613,7 @@ class ResourceCommandResolver {
}
}

getResources(resource: Resource): [Uri | undefined, Uri] {
getResources(resource: Resource): [Uri | undefined, Uri | undefined] {
for (const submodule of this.repository.submodules) {
if (path.join(this.repository.root, submodule.path) === resource.resourceUri.fsPath) {
return [undefined, toGitUri(resource.resourceUri, resource.resourceGroupType === ResourceGroupType.Index ? 'index' : 'wt', { submoduleOf: this.repository.root })];
Expand Down Expand Up @@ -641,7 +641,7 @@ class ResourceCommandResolver {
return undefined;
}

private getRightResource(resource: Resource): Uri {
private getRightResource(resource: Resource): Uri | undefined {
switch (resource.type) {
case Status.INDEX_MODIFIED:
case Status.INDEX_ADDED:
Expand Down Expand Up @@ -677,7 +677,7 @@ class ResourceCommandResolver {
return resource.resourceUri;
}

throw new Error('Should never happen');
return undefined;
}

private getTitle(resource: Resource): string {
Expand Down
27 changes: 27 additions & 0 deletions extensions/git/src/test/smoke.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,31 @@ suite('git smoke test', function () {
assert.equal(repository.state.workingTreeChanges.length, 0);
assert.equal(repository.state.indexChanges.length, 0);
});

test('rename/delete conflict', async function () {
cp.execSync('git branch test', { cwd });
cp.execSync('git checkout test', { cwd });

fs.unlinkSync(file('app.js'));
cp.execSync('git add .', { cwd });

await repository.commit('commit on test');
cp.execSync('git checkout master', { cwd });

fs.renameSync(file('app.js'), file('rename.js'));
cp.execSync('git add .', { cwd });
await repository.commit('commit on master');

try {
cp.execSync('git merge test', { cwd });
} catch (e) { }

setTimeout(() => {
commands.executeCommand('workbench.scm.focus');
}, 2e3);

await new Promise(resolve => {
setTimeout(resolve, 5e3);
});
});
});