Skip to content

Commit f2fed53

Browse files
Copilotalexr00
Copilot
and
alexr00
authored
Fix wrong commit checkout when local branch exists with same name as PR (#7846)
* Initial plan * Initial plan for fixing checkout issue Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Fix issue with wrong commit being checked out when local branch exists Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Fix test compilation errors and update final implementation Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Add comprehensive fix for edge case and additional test coverage * Replace dangerous branch deletion with safe unique branch creation - Addressed feedback from @alexr00 about dangerous branch deletion - Instead of deleting user's local branch, create unique branch name - Use existing calculateUniqueBranchNameForPR function for naming - Preserve user's work by never touching their original branch - New approach follows existing PR branch naming patterns * Changes before error encountered Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com> * Fix tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>
1 parent dc08c79 commit f2fed53

File tree

3 files changed

+135
-22
lines changed

3 files changed

+135
-22
lines changed

‎src/github/pullRequestGitHelper.ts‎

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -87,50 +87,67 @@ export class PullRequestGitHelper {
8787
return PullRequestGitHelper.checkoutFromFork(repository, pullRequest, remote && remote.remoteName, progress);
8888
}
8989

90-
const branchName = pullRequest.head.ref;
90+
const originalBranchName = pullRequest.head.ref;
9191
const remoteName = remote.remoteName;
9292
let branch: Branch;
93+
let localBranchName = originalBranchName; // This will be the branch we actually checkout
94+
95+
// Always fetch the remote branch first to ensure we have the latest commits
96+
const trackedBranchName = `refs/remotes/${remoteName}/${originalBranchName}`;
97+
Logger.appendLine(`Fetch tracked branch ${trackedBranchName}`, PullRequestGitHelper.ID);
98+
progress.report({ message: vscode.l10n.t('Fetching branch {0}', originalBranchName) });
99+
await repository.fetch(remoteName, originalBranchName);
100+
const trackedBranch = await repository.getBranch(trackedBranchName);
93101

94102
try {
95-
branch = await repository.getBranch(branchName);
103+
branch = await repository.getBranch(localBranchName);
104+
// Check if local branch is pointing to the same commit as the remote
105+
if (branch.commit !== trackedBranch.commit) {
106+
Logger.appendLine(`Local branch ${localBranchName} commit ${branch.commit} differs from remote commit ${trackedBranch.commit}. Creating new branch to avoid overwriting user's work.`, PullRequestGitHelper.ID);
107+
// Instead of deleting the user's branch, create a unique branch name to avoid conflicts
108+
const uniqueBranchName = await PullRequestGitHelper.calculateUniqueBranchNameForPR(repository, pullRequest);
109+
Logger.appendLine(`Creating branch ${uniqueBranchName} for PR checkout`, PullRequestGitHelper.ID);
110+
progress.report({ message: vscode.l10n.t('Creating branch {0} for pull request', uniqueBranchName) });
111+
await repository.createBranch(uniqueBranchName, false, trackedBranch.commit);
112+
await repository.setBranchUpstream(uniqueBranchName, trackedBranchName);
113+
// Use the unique branch name for checkout
114+
localBranchName = uniqueBranchName;
115+
branch = await repository.getBranch(localBranchName);
116+
}
117+
96118
// Make sure we aren't already on this branch
97119
if (repository.state.HEAD?.name === branch.name) {
98-
Logger.appendLine(`Tried to checkout ${branchName}, but branch is already checked out.`, PullRequestGitHelper.ID);
120+
Logger.appendLine(`Tried to checkout ${localBranchName}, but branch is already checked out.`, PullRequestGitHelper.ID);
99121
return;
100122
}
101-
Logger.debug(`Checkout ${branchName}`, PullRequestGitHelper.ID);
102-
progress.report({ message: vscode.l10n.t('Checking out {0}', branchName) });
103-
await repository.checkout(branchName);
123+
124+
Logger.debug(`Checkout ${localBranchName}`, PullRequestGitHelper.ID);
125+
progress.report({ message: vscode.l10n.t('Checking out {0}', localBranchName) });
126+
await repository.checkout(localBranchName);
104127

105128
if (!branch.upstream) {
106129
// this branch is not associated with upstream yet
107-
const trackedBranchName = `refs/remotes/${remoteName}/${branchName}`;
108-
await repository.setBranchUpstream(branchName, trackedBranchName);
130+
await repository.setBranchUpstream(localBranchName, trackedBranchName);
109131
}
110132

111133
if (branch.behind !== undefined && branch.behind > 0 && branch.ahead === 0) {
112134
Logger.debug(`Pull from upstream`, PullRequestGitHelper.ID);
113-
progress.report({ message: vscode.l10n.t('Pulling {0}', branchName) });
135+
progress.report({ message: vscode.l10n.t('Pulling {0}', localBranchName) });
114136
await repository.pull();
115137
}
116138
} catch (err) {
117-
// there is no local branch with the same name, so we are good to fetch, create and checkout the remote branch.
139+
// there is no local branch with the same name, so we are good to create and checkout the remote branch.
118140
Logger.appendLine(
119-
`Branch ${remoteName}/${branchName} doesn't exist on local disk yet.`,
141+
`Branch ${localBranchName} doesn't exist on local disk yet. Creating from remote.`,
120142
PullRequestGitHelper.ID,
121143
);
122-
const trackedBranchName = `refs/remotes/${remoteName}/${branchName}`;
123-
Logger.appendLine(`Fetch tracked branch ${trackedBranchName}`, PullRequestGitHelper.ID);
124-
progress.report({ message: vscode.l10n.t('Fetching branch {0}', branchName) });
125-
await repository.fetch(remoteName, branchName);
126-
const trackedBranch = await repository.getBranch(trackedBranchName);
127144
// create branch
128-
progress.report({ message: vscode.l10n.t('Creating and checking out branch {0}', branchName) });
129-
await repository.createBranch(branchName, true, trackedBranch.commit);
130-
await repository.setBranchUpstream(branchName, trackedBranchName);
145+
progress.report({ message: vscode.l10n.t('Creating and checking out branch {0}', localBranchName) });
146+
await repository.createBranch(localBranchName, true, trackedBranch.commit);
147+
await repository.setBranchUpstream(localBranchName, trackedBranchName);
131148
}
132149

133-
await PullRequestGitHelper.associateBranchWithPullRequest(repository, pullRequest, branchName);
150+
await PullRequestGitHelper.associateBranchWithPullRequest(repository, pullRequest, localBranchName);
134151
}
135152

136153
static async checkoutExistingPullRequestBranch(repository: Repository, pullRequest: PullRequestModel, progress: vscode.Progress<{ message?: string; increment?: number }>) {

‎src/test/github/pullRequestGitHelper.test.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,98 @@ describe('PullRequestGitHelper', function () {
4444
sinon.restore();
4545
});
4646

47+
describe('fetchAndCheckout', function () {
48+
it('creates a unique branch when local branch exists with different commit to preserve user work', async function () {
49+
const url = 'git@github.com:owner/name.git';
50+
const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom);
51+
const gitHubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon);
52+
53+
const prItem = convertRESTPullRequestToRawPullRequest(
54+
new PullRequestBuilder()
55+
.number(100)
56+
.user(u => u.login('me'))
57+
.base(b => {
58+
(b.repo)(r => (<RepositoryBuilder>r).clone_url('git@github.com:owner/name.git'));
59+
})
60+
.head(h => {
61+
h.repo(r => (<RepositoryBuilder>r).clone_url('git@github.com:owner/name.git'));
62+
h.ref('my-branch');
63+
})
64+
.build(),
65+
gitHubRepository,
66+
);
67+
68+
const pullRequest = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem);
69+
70+
// Setup: local branch exists with different commit than remote
71+
await repository.createBranch('my-branch', false, 'local-commit-hash');
72+
73+
// Setup: remote branch has different commit
74+
await repository.createBranch('refs/remotes/origin/my-branch', false, 'remote-commit-hash');
75+
76+
const remotes = [remote];
77+
78+
// Expect fetch to be called
79+
repository.expectFetch('origin', 'my-branch');
80+
81+
await PullRequestGitHelper.fetchAndCheckout(repository, remotes, pullRequest, { report: () => undefined });
82+
83+
// Verify that the original local branch is preserved
84+
const originalBranch = await repository.getBranch('my-branch');
85+
assert.strictEqual(originalBranch.commit, 'local-commit-hash', 'Original branch should be preserved');
86+
87+
// Verify that a unique branch was created with the correct commit
88+
const uniqueBranch = await repository.getBranch('pr/me/100');
89+
assert.strictEqual(uniqueBranch.commit, 'remote-commit-hash', 'Unique branch should have remote commit');
90+
assert.strictEqual(repository.state.HEAD?.name, 'pr/me/100', 'Should check out the unique branch');
91+
});
92+
93+
it('creates a unique branch even when currently checked out on conflicting local branch', async function () {
94+
const url = 'git@github.com:owner/name.git';
95+
const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom);
96+
const gitHubRepository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon);
97+
98+
const prItem = convertRESTPullRequestToRawPullRequest(
99+
new PullRequestBuilder()
100+
.number(100)
101+
.user(u => u.login('me'))
102+
.base(b => {
103+
(b.repo)(r => (<RepositoryBuilder>r).clone_url('git@github.com:owner/name.git'));
104+
})
105+
.head(h => {
106+
h.repo(r => (<RepositoryBuilder>r).clone_url('git@github.com:owner/name.git'));
107+
h.ref('my-branch');
108+
})
109+
.build(),
110+
gitHubRepository,
111+
);
112+
113+
const pullRequest = new PullRequestModel(credentialStore, telemetry, gitHubRepository, remote, prItem);
114+
115+
// Setup: local branch exists with different commit than remote AND is currently checked out
116+
await repository.createBranch('my-branch', true, 'local-commit-hash'); // checkout = true
117+
118+
// Setup: remote branch has different commit
119+
await repository.createBranch('refs/remotes/origin/my-branch', false, 'remote-commit-hash');
120+
121+
const remotes = [remote];
122+
123+
// Expect fetch to be called
124+
repository.expectFetch('origin', 'my-branch');
125+
126+
await PullRequestGitHelper.fetchAndCheckout(repository, remotes, pullRequest, { report: () => undefined });
127+
128+
// Verify that the original local branch is preserved with its commit
129+
const originalBranch = await repository.getBranch('my-branch');
130+
assert.strictEqual(originalBranch.commit, 'local-commit-hash', 'Original branch should be preserved');
131+
132+
// Verify that a unique branch was created and checked out
133+
const uniqueBranch = await repository.getBranch('pr/me/100');
134+
assert.strictEqual(uniqueBranch.commit, 'remote-commit-hash', 'Unique branch should have remote commit');
135+
assert.strictEqual(repository.state.HEAD?.name, 'pr/me/100', 'Should check out the unique branch');
136+
});
137+
});
138+
47139
describe('checkoutFromFork', function () {
48140
it('fetches, checks out, and configures a branch from a fork', async function () {
49141
const url = 'git@github.com:owner/name.git';

‎src/test/mocks/mockRepository.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,12 @@ export class MockRepository implements Repository {
149149
return Promise.reject(new Error('Unexpected hashObject(...)'));
150150
}
151151

152+
private _hasBranch(ref: string) {
153+
return this._branches.some(b => b.name === ref);
154+
}
155+
152156
async createBranch(name: string, checkout: boolean, ref?: string | undefined): Promise<void> {
153-
if (this._branches.some(b => b.name === name)) {
157+
if (this._hasBranch(name)) {
154158
throw new Error(`A branch named ${name} already exists`);
155159
}
156160

@@ -275,7 +279,7 @@ export class MockRepository implements Repository {
275279
throw new Error(`Unexpected fetch(${remoteName}, ${ref}, ${depth})`);
276280
}
277281

278-
if (ref) {
282+
if (ref && !this._hasBranch(ref)) {
279283
const match = /^(?:\+?[^:]+\:)?(.*)$/.exec(ref);
280284
if (match) {
281285
const [, localRef] = match;

0 commit comments

Comments
 (0)