Skip to content

Commit

Permalink
fix: Revert "feat: compare all branch authors when deciding if a bran…
Browse files Browse the repository at this point in the history
…ch is modified" (#25254)
  • Loading branch information
rarkins committed Oct 17, 2023
1 parent 68ca88a commit 59d0149
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 106 deletions.
2 changes: 1 addition & 1 deletion lib/modules/platform/default-scm.spec.ts
Expand Up @@ -45,7 +45,7 @@ describe('modules/platform/default-scm', () => {

it('delegate isBranchModified to util/git', async () => {
git.isBranchModified.mockResolvedValueOnce(true);
await defaultGitScm.isBranchModified('branchName', 'main');
await defaultGitScm.isBranchModified('branchName');
expect(git.isBranchModified).toHaveBeenCalledTimes(1);
});

Expand Down
4 changes: 2 additions & 2 deletions lib/modules/platform/default-scm.ts
Expand Up @@ -27,8 +27,8 @@ export class DefaultGitScm implements PlatformScm {
return git.isBranchConflicted(baseBranch, branch);
}

isBranchModified(branchName: string, baseBranch?: string): Promise<boolean> {
return git.isBranchModified(branchName, baseBranch);
isBranchModified(branchName: string): Promise<boolean> {
return git.isBranchModified(branchName);
}

getFileList(): Promise<string[]> {
Expand Down
2 changes: 1 addition & 1 deletion lib/modules/platform/types.ts
Expand Up @@ -233,7 +233,7 @@ export interface Platform {

export interface PlatformScm {
isBranchBehindBase(branchName: string, baseBranch: string): Promise<boolean>;
isBranchModified(branchName: string, baseBranch?: string): Promise<boolean>;
isBranchModified(branchName: string): Promise<boolean>;
isBranchConflicted(baseBranch: string, branch: string): Promise<boolean>;
branchExists(branchName: string): Promise<boolean>;
getBranchCommit(branchName: string): Promise<CommitSha | null>;
Expand Down
39 changes: 1 addition & 38 deletions lib/util/git/index.spec.ts
Expand Up @@ -77,20 +77,6 @@ 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');
Expand Down Expand Up @@ -289,50 +275,27 @@ 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', defaultBranch)
).toBeFalse();
expect(await git.isBranchModified('renovate/future_branch')).toBeFalse();
});

it('should return false when author is ignored', async () => {
git.setUserRepoConfig({
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();
});
});

Expand Down
87 changes: 31 additions & 56 deletions lib/util/git/index.ts
Expand Up @@ -616,10 +616,7 @@ export async function isBranchBehindBase(
}
}

export async function isBranchModified(
branchName: string,
baseBranch?: string
): Promise<boolean> {
export async function isBranchModified(branchName: string): Promise<boolean> {
if (!branchExists(branchName)) {
logger.debug('branch.isModified(): no cache');
return false;
Expand All @@ -639,49 +636,21 @@ export async function isBranchModified(
return isModified;
}

logger.debug('branch.isModified(): using git to calculate');

await syncGit();
// Retrieve the commit authors
let branchAuthors: string[] = [];
// Retrieve the author of the most recent commit
let lastAuthor: string | undefined;
try {
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)
),
];
}
lastAuthor = (
await git.raw([
'log',
'-1',
'--pretty=format:%ae',
`origin/${branchName}`,
'--',
])
).trim();
} catch (err) /* istanbul ignore next */ {
if (err.message?.includes('fatal: bad revision')) {
logger.debug(
Expand All @@ -690,20 +659,26 @@ export async function isBranchModified(
);
throw new Error(REPOSITORY_CHANGED);
}
logger.warn({ err }, 'Error retrieving git authors for isBranchModified');
logger.warn({ err }, 'Error checking last author for isBranchModified');
}
const { gitAuthorEmail } = config;
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;
}
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;
}
logger.debug(`branch.isModified() = ${branchIsModified}`);
config.branchIsModified[branchName] = branchIsModified;
setCachedModifiedResult(branchName, branchIsModified);
return branchIsModified;
logger.debug(
{ branchName, lastAuthor, gitAuthorEmail },
'branch.isModified() = true'
);
config.branchIsModified[branchName] = true;
setCachedModifiedResult(branchName, true);
return true;
}

export async function isBranchConflicted(
Expand Down
2 changes: 1 addition & 1 deletion lib/workers/repository/config-migration/branch/rebase.ts
Expand Up @@ -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, config.baseBranch)) {
if (await scm.isBranchModified(branchName)) {
logger.debug('Migration branch has been edited and cannot be rebased');
return null;
}
Expand Down
5 changes: 1 addition & 4 deletions lib/workers/repository/update/branch/index.ts
Expand Up @@ -242,10 +242,7 @@ export async function processBranch(
}

logger.debug('Checking if PR has been edited');
const branchIsModified = await scm.isBranchModified(
config.branchName,
config.baseBranch
);
const branchIsModified = await scm.isBranchModified(config.branchName);
if (branchPr) {
logger.debug('Found existing branch PR');
if (branchPr.state !== 'open') {
Expand Down
4 changes: 2 additions & 2 deletions lib/workers/repository/update/branch/reuse.ts
Expand Up @@ -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, baseBranch)) {
if (await scm.isBranchModified(branchName)) {
logger.debug('Cannot rebase branch as it has been modified');
result.reuseExistingBranch = true;
result.isModified = true;
Expand All @@ -50,7 +50,7 @@ export async function shouldReuseExistingBranch(
if (result.isConflicted) {
logger.debug('Branch is conflicted');

if ((await scm.isBranchModified(branchName, baseBranch)) === false) {
if ((await scm.isBranchModified(branchName)) === false) {
logger.debug(`Branch is not mergeable and needs rebasing`);
if (config.rebaseWhen === 'never') {
logger.debug('Rebasing disabled by config');
Expand Down
2 changes: 1 addition & 1 deletion lib/workers/repository/update/pr/automerge.ts
Expand Up @@ -82,7 +82,7 @@ export async function checkAutoMerge(
};
}
// Check if it's been touched
if (await scm.isBranchModified(branchName, config.baseBranch)) {
if (await scm.isBranchModified(branchName)) {
logger.debug('PR is ready for automerge but has been modified');
return {
automerged: false,
Expand Down

0 comments on commit 59d0149

Please sign in to comment.