Fix fork deletion for squashed/closed PRs

src/should-delete-fork.js

@@ -1,5 +1,34 @@
 'use strict';
 
+const getBranchPRStatus = async (github, repo, branch) => {
+	try {
+		const {data: prs} = await github.pulls.list({
+			owner: repo.parent.owner.login,
+			repo: repo.parent.name,
+			state: 'all',
+			head: `${repo.owner.login}:${branch.name}`,
+			per_page: 1
+		});
+
+		if (prs.length === 0) {
+			return null;
+		}
+
+		const pr = prs[0];
+		if (pr.merged_at) {
+			return 'merged';
+		}
+
+		if (pr.state === 'closed') {
+			return 'closed';
+		}
+
+		return 'open';
+	} catch {
+		return null;
+	}
+};
+
 const branchIsUseful = async (github, repo, branch, parentBranches) => {
 	const someParentBranchAtThisSha =
 		parentBranches.filter(candidate => {
@@ -10,12 +39,33 @@ const branchIsUseful = async (github, repo, branch, parentBranches) => {
 		return false;
 	}
 
+	const prStatus = await getBranchPRStatus(github, repo, branch);
+	if (prStatus === 'merged') {
+		return false;
+	}
+
+	if (prStatus === 'open' || prStatus === 'closed') {
+		return true;
+	}
+
 	// Check if at least one parent branch contains this commit
 	// Looping through all parent branches is immensely slow,
 	// let's take a safe shortcut
-	const branchesToCheck = parentBranches.filter(b => {
-		return [branch.name, repo.default_branch].includes(b.name);
-	});
+	const sameName = parentBranches.find(b => b.name === branch.name);
+	const defaultBranch = parentBranches.find(b => b.name === repo.default_branch);
+
+	// If branch doesn't exist in parent and has no PR, it's likely a deleted
+	// upstream branch that was synced - consider it useless
+	if (!sameName) {
+		return false;
+	}
+
+	// Build list of branches to check, avoiding duplicates
+	const branchesToCheck = [sameName];
+	if (defaultBranch && defaultBranch.name !== sameName.name) {
+		branchesToCheck.push(defaultBranch);
+	}
+
 	for (const candidate of branchesToCheck) {
 		try {
 			const {data: diff} = await github.repos.compareCommits({
@@ -32,7 +82,8 @@ const branchIsUseful = async (github, repo, branch, parentBranches) => {
 		} catch (error) {
 			// If diff can't be found, means our commit is not on this
 			// parent branch candidate
-			if (error.status === 404) {
+			// Note: error.status can be a number or string depending on the error type
+			if (Number(error.status) === 404) {
 				continue;
 			}
 
@@ -73,7 +124,8 @@ module.exports = async (github, fork) => {
 		}));
 	} catch (error) {
 		// If parent repository was deleted, need to preserve the fork
-		if (error && error.status === 404) {
+		// Note: error.status can be a number or string depending on the error type
+		if (error && Number(error.status) === 404) {
 			return false;
 		}
 

test/helpers/index.js

@@ -103,10 +103,16 @@ exports.mock = responses => {
 		listForAuthenticatedUser: buildRejecter('listForAuthenticatedUser'),
 		listBranches: buildRejecter('listBranches')
 	};
-	Object.keys(responses).forEach(name => {
+	Object.keys(responses).filter(name => name !== 'pulls.list').forEach(name => {
 		repos[name] = buildResponder(name, responses[name]);
 	});
 
+	const pulls = {
+		list: responses['pulls.list'] ?
+			buildResponder('pulls.list', responses['pulls.list']) :
+			buildRejecter('pulls.list')
+	};
+
 	return {
 		calls() {
 			return calls;
@@ -115,7 +121,8 @@ exports.mock = responses => {
 			paginate(f, ...arguments_) {
 				return f(...arguments_);
 			},
-			repos
+			repos,
+			pulls
 		}
 	};
 };

test/squash-merge.js

@@ -0,0 +1,152 @@
+const test = require('ava');
+const removeGithubForks = require('..');
+const lib = require('./helpers');
+
+test.cb('should delete a fork with a merged PR (squash/rebase merge)', t => {
+	const mock = lib.mock({
+		listBranches(arguments_) {
+			if (arguments_.owner === lib.USER.login) {
+				return [
+					{name: 'master', commit: lib.COMMIT_A},
+					{name: 'feature', commit: lib.COMMIT_B}
+				];
+			}
+
+			return [
+				{name: 'master', commit: lib.COMMIT_C}
+			];
+		},
+		'pulls.list': [],
+		compareCommits: {behind_by: 0},
+		delete: true
+	});
+
+	const originalPullsList = mock.present.pulls.list;
+	mock.present.pulls.list = async arguments_ => {
+		if (arguments_.head === `${lib.USER.login}:feature`) {
+			await originalPullsList({
+				...arguments_,
+				_response: [{merged_at: '2024-01-01T00:00:00Z', state: 'closed'}]
+			});
+			return {data: [{merged_at: '2024-01-01T00:00:00Z', state: 'closed'}]};
+		}
+
+		return {data: []};
+	};
+
+	removeGithubForks(mock.present, error => {
+		if (error) {
+			return t.fail(error);
+		}
+
+		t.pass();
+		t.end();
+	});
+});
+
+test.cb('should not delete a fork with a declined (closed without merge) PR', t => {
+	const mock = lib.mock({
+		listBranches(arguments_) {
+			if (arguments_.owner === lib.USER.login) {
+				return [
+					{name: 'master', commit: lib.COMMIT_A},
+					{name: 'declined-feature', commit: lib.COMMIT_B}
+				];
+			}
+
+			return [
+				{name: 'master', commit: lib.COMMIT_C}
+			];
+		},
+		'pulls.list': [],
+		compareCommits: {behind_by: 0}
+	});
+
+	mock.present.pulls.list = arguments_ => {
+		if (arguments_.head === `${lib.USER.login}:declined-feature`) {
+			return Promise.resolve({data: [{merged_at: null, state: 'closed'}]});
+		}
+
+		return Promise.resolve({data: []});
+	};
+
+	removeGithubForks(mock.present, error => {
+		if (error) {
+			return t.fail(error);
+		}
+
+		const deleteCalls = mock.calls().filter(call => call[0] === 'delete');
+		t.is(deleteCalls.length, 0, 'Fork should not be deleted when it has a declined PR');
+		t.end();
+	});
+});
+
+test.cb('should not delete a fork with an open PR', t => {
+	const mock = lib.mock({
+		listBranches(arguments_) {
+			if (arguments_.owner === lib.USER.login) {
+				return [
+					{name: 'master', commit: lib.COMMIT_A},
+					{name: 'active-feature', commit: lib.COMMIT_B}
+				];
+			}
+
+			return [
+				{name: 'master', commit: lib.COMMIT_C}
+			];
+		},
+		'pulls.list': [],
+		compareCommits: {behind_by: 0}
+	});
+
+	mock.present.pulls.list = arguments_ => {
+		if (arguments_.head === `${lib.USER.login}:active-feature`) {
+			return Promise.resolve({data: [{merged_at: null, state: 'open'}]});
+		}
+
+		return Promise.resolve({data: []});
+	};
+
+	removeGithubForks(mock.present, error => {
+		if (error) {
+			return t.fail(error);
+		}
+
+		const deleteCalls = mock.calls().filter(call => call[0] === 'delete');
+		t.is(deleteCalls.length, 0, 'Fork should not be deleted when it has an open PR');
+		t.end();
+	});
+});
+
+test.cb('should use compareCommits fallback when no PR exists', t => {
+	const mock = lib.mock({
+		listBranches(arguments_) {
+			if (arguments_.owner === lib.USER.login) {
+				return [
+					{name: 'master', commit: lib.COMMIT_A},
+					{name: 'no-pr-branch', commit: lib.COMMIT_B}
+				];
+			}
+
+			return [
+				{name: 'master', commit: lib.COMMIT_C},
+				{name: 'no-pr-branch', commit: lib.COMMIT_C}
+			];
+		},
+		'pulls.list': [],
+		compareCommits: {behind_by: 0},
+		delete: true
+	});
+
+	mock.present.pulls.list = () => Promise.resolve({data: []});
+
+	removeGithubForks(mock.present, error => {
+		if (error) {
+			return t.fail(error);
+		}
+
+		const compareCommitsCalls = mock.calls().filter(call => call[0] === 'compareCommits');
+		t.true(compareCommitsCalls.length > 0, 'Should use compareCommits when no PR exists');
+		t.end();
+	});
+});