From c2cb93ce44aa3319dff703f2041e47bfe033c1f4 Mon Sep 17 00:00:00 2001
From: Alex Kessock <akessock@gmail.com>
Date: Tue, 17 Oct 2023 02:45:53 -0600
Subject: [PATCH] feat: compare all branch authors when deciding if a branch is
modified (#21558)
Co-authored-by: Rhys Arkins <rhys@arkins.net>
---
lib/modules/platform/default-scm.spec.ts | 2 +-
lib/modules/platform/default-scm.ts | 4 +-
lib/modules/platform/types.ts | 2 +-
lib/util/git/index.spec.ts | 39 ++++++++-
lib/util/git/index.ts | 87 ++++++++++++-------
.../config-migration/branch/rebase.ts | 2 +-
lib/workers/repository/update/branch/index.ts | 5 +-
lib/workers/repository/update/branch/reuse.ts | 4 +-
lib/workers/repository/update/pr/automerge.ts | 2 +-
9 files changed, 106 insertions(+), 41 deletions(-)
diff --git a/lib/modules/platform/default-scm.spec.ts b/lib/modules/platform/default-scm.spec.ts
index 2f97a3c4ef..d7a7995bc9 100644
--- a/lib/modules/platform/default-scm.spec.ts
+++ b/lib/modules/platform/default-scm.spec.ts
@@ -45,7 +45,7 @@ describe('modules/platform/default-scm', () => {
it('delegate isBranchModified to util/git', async () => {
git.isBranchModified.mockResolvedValueOnce(true);
- await defaultGitScm.isBranchModified('branchName');
+ await defaultGitScm.isBranchModified('branchName', 'main');
expect(git.isBranchModified).toHaveBeenCalledTimes(1);
});
diff --git a/lib/modules/platform/default-scm.ts b/lib/modules/platform/default-scm.ts
index 92ceef5421..c105a0bd16 100644
--- a/lib/modules/platform/default-scm.ts
+++ b/lib/modules/platform/default-scm.ts
@@ -27,8 +27,8 @@ export class DefaultGitScm implements PlatformScm {
return git.isBranchConflicted(baseBranch, branch);
}
- isBranchModified(branchName: string): Promise<boolean> {
- return git.isBranchModified(branchName);
+ isBranchModified(branchName: string, baseBranch?: string): Promise<boolean> {
+ return git.isBranchModified(branchName, baseBranch);
}
getFileList(): Promise<string[]> {
diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts
index ca98ae3337..fb3ac95387 100644
--- a/lib/modules/platform/types.ts
+++ b/lib/modules/platform/types.ts
@@ -233,7 +233,7 @@ export interface Platform {
export interface PlatformScm {
isBranchBehindBase(branchName: string, baseBranch: string): Promise<boolean>;
- isBranchModified(branchName: string): Promise<boolean>;
+ isBranchModified(branchName: string, baseBranch?: string): Promise<boolean>;
isBranchConflicted(baseBranch: string, branch: string): Promise<boolean>;
branchExists(branchName: string): Promise<boolean>;
getBranchCommit(branchName: string): Promise<CommitSha | null>;
diff --git a/lib/util/git/index.spec.ts b/lib/util/git/index.spec.ts
index 3902d862a4..a8d8e426db 100644
--- a/lib/util/git/index.spec.ts
+++ b/lib/util/git/index.spec.ts
@@ -77,6 +77,20 @@ describe('util/git/index', () => {
await repo.addConfig('user.email', 'custom@example.com');
await repo.commit('custom message');
+ await repo.checkoutBranch('renovate/multiple_commits', defaultBranch);
+ await fs.writeFile(base.path + '/commit1', 'commit1');
+ await repo.add(['commit1']);
+ await repo.addConfig('user.email', 'author1@example.com');
+ await repo.commit('commit1 message');
+ await fs.writeFile(base.path + '/commit2', 'commit2');
+ await repo.add(['commit2']);
+ await repo.addConfig('user.email', 'author2@example.com');
+ await repo.commit('commit2 message');
+ await fs.writeFile(base.path + '/commit3', 'commit3');
+ await repo.add(['commit3']);
+ await repo.addConfig('user.email', 'author1@example.com');
+ await repo.commit('commit3 message');
+
await repo.checkoutBranch('renovate/nested_files', defaultBranch);
await fs.mkdirp(base.path + '/bin/');
await fs.writeFile(base.path + '/bin/nested', 'nested');
@@ -275,11 +289,16 @@ describe('util/git/index', () => {
it('should return false when branch is not found', async () => {
expect(await git.isBranchModified('renovate/not_found')).toBeFalse();
+ expect(
+ await git.isBranchModified('renovate/not_found', defaultBranch)
+ ).toBeFalse();
});
it('should return false when author matches', async () => {
expect(await git.isBranchModified('renovate/future_branch')).toBeFalse();
- expect(await git.isBranchModified('renovate/future_branch')).toBeFalse();
+ expect(
+ await git.isBranchModified('renovate/future_branch', defaultBranch)
+ ).toBeFalse();
});
it('should return false when author is ignored', async () => {
@@ -287,15 +306,33 @@ describe('util/git/index', () => {
gitIgnoredAuthors: ['custom@example.com'],
});
expect(await git.isBranchModified('renovate/custom_author')).toBeFalse();
+ expect(
+ await git.isBranchModified('renovate/custom_author', defaultBranch)
+ ).toBeFalse();
});
it('should return true when custom author is unknown', async () => {
expect(await git.isBranchModified('renovate/custom_author')).toBeTrue();
+ expect(
+ await git.isBranchModified('renovate/custom_author', defaultBranch)
+ ).toBeTrue();
+ });
+
+ it('should return true if any commit is modified', async () => {
+ git.setUserRepoConfig({
+ gitIgnoredAuthors: ['author1@example.com'],
+ });
+ expect(
+ await git.isBranchModified('renovate/multiple_commits', defaultBranch)
+ ).toBeTrue();
});
it('should return value stored in modifiedCacheResult', async () => {
modifiedCache.getCachedModifiedResult.mockReturnValue(true);
expect(await git.isBranchModified('renovate/future_branch')).toBeTrue();
+ expect(
+ await git.isBranchModified('renovate/future_branch', defaultBranch)
+ ).toBeTrue();
});
});
diff --git a/lib/util/git/index.ts b/lib/util/git/index.ts
index c919e559ef..587eef39d4 100644
--- a/lib/util/git/index.ts
+++ b/lib/util/git/index.ts
@@ -616,7 +616,10 @@ export async function isBranchBehindBase(
}
}
-export async function isBranchModified(branchName: string): Promise<boolean> {
+export async function isBranchModified(
+ branchName: string,
+ baseBranch?: string
+): Promise<boolean> {
if (!branchExists(branchName)) {
logger.debug('branch.isModified(): no cache');
return false;
@@ -636,21 +639,49 @@ export async function isBranchModified(branchName: string): Promise<boolean> {
return isModified;
}
- logger.debug('branch.isModified(): using git to calculate');
-
await syncGit();
- // Retrieve the author of the most recent commit
- let lastAuthor: string | undefined;
+ // Retrieve the commit authors
+ let branchAuthors: string[] = [];
try {
- lastAuthor = (
- await git.raw([
- 'log',
- '-1',
- '--pretty=format:%ae',
- `origin/${branchName}`,
- '--',
- ])
- ).trim();
+ if (baseBranch) {
+ logger.debug(
+ `branch.isModified(): using git to calculate authors between ${branchName} and ${baseBranch}`
+ );
+ branchAuthors = [
+ ...new Set(
+ (
+ await git.raw([
+ 'log',
+ '--pretty=format:%ae',
+ `origin/${branchName}..origin/${baseBranch}`,
+ '--',
+ ])
+ )
+ .trim()
+ .split(newlineRegex)
+ ),
+ ];
+ logger.debug(`Git authors in branch: ${branchAuthors.join(', ')}`);
+ } else {
+ logger.debug(
+ `branch.isModified(): checking last author of ${branchName}`
+ );
+ branchAuthors = [
+ ...new Set(
+ (
+ await git.raw([
+ 'log',
+ '-1',
+ '--pretty=format:%ae',
+ `origin/${branchName}`,
+ '--',
+ ])
+ )
+ .trim()
+ .split(newlineRegex)
+ ),
+ ];
+ }
} catch (err) /* istanbul ignore next */ {
if (err.message?.includes('fatal: bad revision')) {
logger.debug(
@@ -659,26 +690,20 @@ export async function isBranchModified(branchName: string): Promise<boolean> {
);
throw new Error(REPOSITORY_CHANGED);
}
- logger.warn({ err }, 'Error checking last author for isBranchModified');
+ logger.warn({ err }, 'Error retrieving git authors for isBranchModified');
}
const { gitAuthorEmail } = config;
- if (
- lastAuthor === gitAuthorEmail ||
- config.ignoredAuthors.some((ignoredAuthor) => lastAuthor === ignoredAuthor)
- ) {
- // author matches - branch has not been modified
- logger.debug('branch.isModified() = false');
- config.branchIsModified[branchName] = false;
- setCachedModifiedResult(branchName, false);
- return false;
+ let branchIsModified = false;
+ for (const author of branchAuthors) {
+ if (author !== gitAuthorEmail && !config.ignoredAuthors.includes(author)) {
+ logger.debug(`branch.isModified=true due to this git author: ${author}`);
+ branchIsModified = true;
+ }
}
- logger.debug(
- { branchName, lastAuthor, gitAuthorEmail },
- 'branch.isModified() = true'
- );
- config.branchIsModified[branchName] = true;
- setCachedModifiedResult(branchName, true);
- return true;
+ logger.debug(`branch.isModified() = ${branchIsModified}`);
+ config.branchIsModified[branchName] = branchIsModified;
+ setCachedModifiedResult(branchName, branchIsModified);
+ return branchIsModified;
}
export async function isBranchConflicted(
diff --git a/lib/workers/repository/config-migration/branch/rebase.ts b/lib/workers/repository/config-migration/branch/rebase.ts
index eb082c893a..4872685cb7 100644
--- a/lib/workers/repository/config-migration/branch/rebase.ts
+++ b/lib/workers/repository/config-migration/branch/rebase.ts
@@ -16,7 +16,7 @@ export async function rebaseMigrationBranch(
): Promise<string | null> {
logger.debug('Checking if migration branch needs rebasing');
const branchName = getMigrationBranchName(config);
- if (await scm.isBranchModified(branchName)) {
+ if (await scm.isBranchModified(branchName, config.baseBranch)) {
logger.debug('Migration branch has been edited and cannot be rebased');
return null;
}
diff --git a/lib/workers/repository/update/branch/index.ts b/lib/workers/repository/update/branch/index.ts
index a78d2f1fca..c3c381f883 100644
--- a/lib/workers/repository/update/branch/index.ts
+++ b/lib/workers/repository/update/branch/index.ts
@@ -242,7 +242,10 @@ export async function processBranch(
}
logger.debug('Checking if PR has been edited');
- const branchIsModified = await scm.isBranchModified(config.branchName);
+ const branchIsModified = await scm.isBranchModified(
+ config.branchName,
+ config.baseBranch
+ );
if (branchPr) {
logger.debug('Found existing branch PR');
if (branchPr.state !== 'open') {
diff --git a/lib/workers/repository/update/branch/reuse.ts b/lib/workers/repository/update/branch/reuse.ts
index d2b3a7b1ac..4abfbccd2a 100644
--- a/lib/workers/repository/update/branch/reuse.ts
+++ b/lib/workers/repository/update/branch/reuse.ts
@@ -29,7 +29,7 @@ export async function shouldReuseExistingBranch(
if (await scm.isBranchBehindBase(branchName, baseBranch)) {
logger.debug(`Branch is behind base branch and needs rebasing`);
// We can rebase the branch only if no PR or PR can be rebased
- if (await scm.isBranchModified(branchName)) {
+ if (await scm.isBranchModified(branchName, baseBranch)) {
logger.debug('Cannot rebase branch as it has been modified');
result.reuseExistingBranch = true;
result.isModified = true;
@@ -50,7 +50,7 @@ export async function shouldReuseExistingBranch(
if (result.isConflicted) {
logger.debug('Branch is conflicted');
- if ((await scm.isBranchModified(branchName)) === false) {
+ if ((await scm.isBranchModified(branchName, baseBranch)) === false) {
logger.debug(`Branch is not mergeable and needs rebasing`);
if (config.rebaseWhen === 'never') {
logger.debug('Rebasing disabled by config');
diff --git a/lib/workers/repository/update/pr/automerge.ts b/lib/workers/repository/update/pr/automerge.ts
index 2e9465ea97..b9baebcf28 100644
--- a/lib/workers/repository/update/pr/automerge.ts
+++ b/lib/workers/repository/update/pr/automerge.ts
@@ -82,7 +82,7 @@ export async function checkAutoMerge(
};
}
// Check if it's been touched
- if (await scm.isBranchModified(branchName)) {
+ if (await scm.isBranchModified(branchName, config.baseBranch)) {
logger.debug('PR is ready for automerge but has been modified');
return {
automerged: false,
--
GitLab