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.

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

fix: handle update after force pushing base to a new commit #1307

Merged
merged 1 commit into from Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
132 changes: 132 additions & 0 deletions __test__/create-or-update-branch.int.test.ts
Expand Up @@ -631,6 +631,69 @@ describe('create-or-update-branch tests', () => {
).toBeTruthy()
})

it('tests create, force push of base branch, and update with identical changes', async () => {
// If the base branch is force pushed to a different commit when there is an open
// pull request, the branch must be reset to rebase the changes on the base.

// Create tracked and untracked file changes
const changes = await createChanges()
const commitMessage = uuidv4()
const result = await createOrUpdateBranch(
git,
commitMessage,
'',
BRANCH,
REMOTE_NAME,
false,
ADD_PATHS_DEFAULT
)
expect(result.action).toEqual('created')
expect(await getFileContent(TRACKED_FILE)).toEqual(changes.tracked)
expect(await getFileContent(UNTRACKED_FILE)).toEqual(changes.untracked)
expect(
await gitLogMatches([commitMessage, INIT_COMMIT_MESSAGE])
).toBeTruthy()

// Push pull request branch to remote
await git.push([
'--force-with-lease',
REMOTE_NAME,
`HEAD:refs/heads/${BRANCH}`
])

await afterTest(false)
await beforeTest()

// Force push the base branch to a different commit
const amendedCommitMessage = uuidv4()
await git.commit(['--amend', '-m', amendedCommitMessage])
await git.push([
'--force',
REMOTE_NAME,
`HEAD:refs/heads/${DEFAULT_BRANCH}`
])

// Create the same tracked and untracked file changes (no change on update)
const _changes = await createChanges(changes.tracked, changes.untracked)
const _commitMessage = uuidv4()
const _result = await createOrUpdateBranch(
git,
_commitMessage,
'',
BRANCH,
REMOTE_NAME,
false,
ADD_PATHS_DEFAULT
)
expect(_result.action).toEqual('updated')
expect(_result.hasDiffWithBase).toBeTruthy()
expect(await getFileContent(TRACKED_FILE)).toEqual(_changes.tracked)
expect(await getFileContent(UNTRACKED_FILE)).toEqual(_changes.untracked)
expect(
await gitLogMatches([_commitMessage, amendedCommitMessage])
).toBeTruthy()
})

it('tests create and update with commits on the working base (during the workflow)', async () => {
// Create commits on the working base
const commits = await createCommits(git)
Expand Down Expand Up @@ -1519,6 +1582,75 @@ describe('create-or-update-branch tests', () => {
).toBeTruthy()
})

it('tests create, force push of base branch, and update with identical changes (WBNB)', async () => {
// If the base branch is force pushed to a different commit when there is an open
// pull request, the branch must be reset to rebase the changes on the base.

// Set the working base to a branch that is not the pull request base
await git.checkout(NOT_BASE_BRANCH)

// Create tracked and untracked file changes
const changes = await createChanges()
const commitMessage = uuidv4()
const result = await createOrUpdateBranch(
git,
commitMessage,
BASE,
BRANCH,
REMOTE_NAME,
false,
ADD_PATHS_DEFAULT
)
expect(result.action).toEqual('created')
expect(await getFileContent(TRACKED_FILE)).toEqual(changes.tracked)
expect(await getFileContent(UNTRACKED_FILE)).toEqual(changes.untracked)
expect(
await gitLogMatches([commitMessage, INIT_COMMIT_MESSAGE])
).toBeTruthy()

// Push pull request branch to remote
await git.push([
'--force-with-lease',
REMOTE_NAME,
`HEAD:refs/heads/${BRANCH}`
])

await afterTest(false)
await beforeTest()

// Force push the base branch to a different commit
const amendedCommitMessage = uuidv4()
await git.commit(['--amend', '-m', amendedCommitMessage])
await git.push([
'--force',
REMOTE_NAME,
`HEAD:refs/heads/${DEFAULT_BRANCH}`
])

// Set the working base to a branch that is not the pull request base
await git.checkout(NOT_BASE_BRANCH)

// Create the same tracked and untracked file changes (no change on update)
const _changes = await createChanges(changes.tracked, changes.untracked)
const _commitMessage = uuidv4()
const _result = await createOrUpdateBranch(
git,
_commitMessage,
BASE,
BRANCH,
REMOTE_NAME,
false,
ADD_PATHS_DEFAULT
)
expect(_result.action).toEqual('updated')
expect(_result.hasDiffWithBase).toBeTruthy()
expect(await getFileContent(TRACKED_FILE)).toEqual(_changes.tracked)
expect(await getFileContent(UNTRACKED_FILE)).toEqual(_changes.untracked)
expect(
await gitLogMatches([_commitMessage, amendedCommitMessage])
).toBeTruthy()
})

it('tests create and update with commits on the working base (during the workflow) (WBNB)', async () => {
// Set the working base to a branch that is not the pull request base
await git.checkout(NOT_BASE_BRANCH)
Expand Down
29 changes: 24 additions & 5 deletions dist/index.js
Expand Up @@ -74,18 +74,30 @@ function tryFetch(git, remote, branch) {
});
}
exports.tryFetch = tryFetch;
// Return the number of commits that branch2 is ahead of branch1
function commitsAhead(git, branch1, branch2) {
return __awaiter(this, void 0, void 0, function* () {
const result = yield git.revList([`${branch1}...${branch2}`], ['--right-only', '--count']);
return Number(result);
});
}
// Return true if branch2 is ahead of branch1
function isAhead(git, branch1, branch2) {
return __awaiter(this, void 0, void 0, function* () {
const result = yield git.revList([`${branch1}...${branch2}`], ['--right-only', '--count']);
return Number(result) > 0;
return (yield commitsAhead(git, branch1, branch2)) > 0;
});
}
// Return the number of commits that branch2 is behind branch1
function commitsBehind(git, branch1, branch2) {
return __awaiter(this, void 0, void 0, function* () {
const result = yield git.revList([`${branch1}...${branch2}`], ['--left-only', '--count']);
return Number(result);
});
}
// Return true if branch2 is behind branch1
function isBehind(git, branch1, branch2) {
return __awaiter(this, void 0, void 0, function* () {
const result = yield git.revList([`${branch1}...${branch2}`], ['--left-only', '--count']);
return Number(result) > 0;
return (yield commitsBehind(git, branch1, branch2)) > 0;
});
}
// Return true if branch2 is even with branch1
Expand Down Expand Up @@ -214,9 +226,16 @@ function createOrUpdateBranch(git, commitMessage, base, branch, branchRemoteName
// branches after merging. In particular, it catches a case where the branch was
// squash merged but not deleted. We need to reset to make sure it doesn't appear
// to have a diff with the base due to different commits for the same changes.
// - If the number of commits ahead of the base branch differs between the branch and
// temp branch. This catches a case where the base branch has been force pushed to
// a new commit.
// For changes on base this reset is equivalent to a rebase of the pull request branch.
const tempBranchCommitsAhead = yield commitsAhead(git, base, tempBranch);
const branchCommitsAhead = yield commitsAhead(git, base, branch);
if ((yield git.hasDiff([`${branch}..${tempBranch}`])) ||
!(yield isAhead(git, base, tempBranch))) {
branchCommitsAhead != tempBranchCommitsAhead ||
!(tempBranchCommitsAhead > 0) // !isAhead
) {
core.info(`Resetting '${branch}'`);
// Alternatively, git switch -C branch tempBranch
yield git.checkout(branch, tempBranch);
Expand Down
40 changes: 32 additions & 8 deletions src/create-or-update-branch.ts
Expand Up @@ -43,30 +43,48 @@ export async function tryFetch(
}
}

// Return true if branch2 is ahead of branch1
async function isAhead(
// Return the number of commits that branch2 is ahead of branch1
async function commitsAhead(
git: GitCommandManager,
branch1: string,
branch2: string
): Promise<boolean> {
): Promise<number> {
const result = await git.revList(
[`${branch1}...${branch2}`],
['--right-only', '--count']
)
return Number(result) > 0
return Number(result)
}

// Return true if branch2 is behind branch1
async function isBehind(
// Return true if branch2 is ahead of branch1
async function isAhead(
git: GitCommandManager,
branch1: string,
branch2: string
): Promise<boolean> {
return (await commitsAhead(git, branch1, branch2)) > 0
}

// Return the number of commits that branch2 is behind branch1
async function commitsBehind(
git: GitCommandManager,
branch1: string,
branch2: string
): Promise<number> {
const result = await git.revList(
[`${branch1}...${branch2}`],
['--left-only', '--count']
)
return Number(result) > 0
return Number(result)
}

// Return true if branch2 is behind branch1
async function isBehind(
git: GitCommandManager,
branch1: string,
branch2: string
): Promise<boolean> {
return (await commitsBehind(git, branch1, branch2)) > 0
}

// Return true if branch2 is even with branch1
Expand Down Expand Up @@ -226,10 +244,16 @@ export async function createOrUpdateBranch(
// branches after merging. In particular, it catches a case where the branch was
// squash merged but not deleted. We need to reset to make sure it doesn't appear
// to have a diff with the base due to different commits for the same changes.
// - If the number of commits ahead of the base branch differs between the branch and
// temp branch. This catches a case where the base branch has been force pushed to
// a new commit.
// For changes on base this reset is equivalent to a rebase of the pull request branch.
const tempBranchCommitsAhead = await commitsAhead(git, base, tempBranch)
const branchCommitsAhead = await commitsAhead(git, base, branch)
if (
(await git.hasDiff([`${branch}..${tempBranch}`])) ||
!(await isAhead(git, base, tempBranch))
branchCommitsAhead != tempBranchCommitsAhead ||
!(tempBranchCommitsAhead > 0) // !isAhead
) {
core.info(`Resetting '${branch}'`)
// Alternatively, git switch -C branch tempBranch
Expand Down