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: Revert "feat: compare all branch authors when deciding if a branch is modified" #25254

Merged
merged 1 commit into from
Oct 17, 2023
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
2 changes: 1 addition & 1 deletion lib/modules/platform/default-scm.spec.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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