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.

Sign up for GitHub

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

Jump to bottom

feat: compare all branch authors when deciding if a branch is modified #21558

Merged
merged 27 commits into from
Oct 17, 2023
Merged

feat: compare all branch authors when deciding if a branch is modified #21558

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
1f9eae1
fix: isBranchModified now compares commit authors to defaultBranch
Keysox Mar 3, 2023
b932f56
Switch to using baseBranch instead of defaultBranch
Keysox Mar 6, 2023
98964bb
Update unit tests with isBranchModified's first parameter
Keysox Mar 6, 2023
1786954
Update types for baseBranch/defaultBranch to be always defined so we …
Keysox Mar 6, 2023
bddb6c0
Revert "Update types for baseBranch/defaultBranch to be always define…
Keysox Mar 6, 2023
18f2597
Simplify branchIsModified logic
Keysox Mar 7, 2023
df774b5
Merge branch 'main' into 19994
Keysox Mar 20, 2023
adb0698
Refactor to for-of loop
Keysox Mar 27, 2023
f679af9
Use old isBranchModified logic if baseBranch is unknown
Keysox Mar 29, 2023
a4c5dc7
Update types and improve debug message
Keysox Mar 29, 2023
4f32999
Don't use baseBranch for cleanUpBranches since it's only called once,…
Keysox Mar 30, 2023
75ddd0b
Commit suggested changes
Keysox Apr 3, 2023
62905b0
Merge branch 'main' into 19994
Keysox Apr 3, 2023
11bf388
Merge branch 'main' into 19994
Keysox Apr 17, 2023
ab74915
Fixes #21504, see https://git-scm.com/docs/git-log\#_description for …
Keysox Apr 17, 2023
64e8c96
Revert test since '..' relies on branches being pushed, which isn't w…
Keysox Apr 17, 2023
6a513b2
ignore coverage for if block since .. relies on a git push
Keysox Apr 17, 2023
2a03f7d
Merge branch 'main' into 19994
Keysox May 22, 2023
0ae4756
Merge branch 'main' into 19994
rarkins Sep 16, 2023
5794b9e
Add test per @rarkins and @viceice
Keysox Sep 18, 2023
362caee
Apply suggestions from code review
Keysox Sep 25, 2023
6db9a13
pnpm run prettier-fix
Keysox Sep 25, 2023
7c28a2e
Log git authors
Keysox Sep 25, 2023
f124927
Merge branch 'main' into 19994
rarkins Sep 26, 2023
ddc85d6
Merge branch 'main' into 19994
rarkins Oct 15, 2023
3d486eb
newlineRegex per @viceice
Keysox Oct 16, 2023
3524e70
Merge branch 'main' into 19994
rarkins Oct 17, 2023
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
View file
Open in desktop
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');
await defaultGitScm.isBranchModified('branchName', 'main');
expect(git.isBranchModified).toHaveBeenCalledTimes(1);
});

Expand Down
4 changes: 2 additions & 2 deletions lib/modules/platform/default-scm.ts
View file
Open in desktop
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): Promise<boolean> {
return git.isBranchModified(branchName);
isBranchModified(branchName: string, baseBranch?: string): Promise<boolean> {
return git.isBranchModified(branchName, baseBranch);
}

getFileList(): Promise<string[]> {
Expand Down
2 changes: 1 addition & 1 deletion lib/modules/platform/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,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>;
Expand Down
16 changes: 15 additions & 1 deletion lib/util/git/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,27 +275,41 @@ 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 () => {
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 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
86 changes: 55 additions & 31 deletions lib/util/git/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,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;
Expand All @@ -631,21 +634,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();
// istanbul ignore if
rarkins marked this conversation as resolved.
Show resolved Hide resolved
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}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax here is the wrong way.
This check means "all from baseBranch except those on branchName".

So basically this would return all commits that are on the base but NOT on the renovate-bot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the right command?

Copy link

@kindlich kindlich Oct 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viceice Either switch the operands

Suggested change
`origin/${branchName}..origin/${baseBranch}`,
`origin/${baseBranch}..origin/${branchName}`,

Or use the explicit

Suggested change
`origin/${branchName}..origin/${baseBranch}`,
`origin/${branchName} ^origin/${baseBranch}`,

Both should show all commits on the branchName (e.g. renovate/dep-abc), but without any commits on the baseBranch (e.g. main).

So if we were to have the following git tree (I hope the mermaid rendering is supported in all browsers):

gitGraph
       commit id: "A"
       commit id: "B"
       commit id: "C"
       branch renovate/x
       checkout main
       commit id: "D"
       commit id: "E"
       checkout renovate/x
       commit id: "X"

Then baseBranch would be main and branchName would be renovate/x
We'd probably only want to look at the commit X, but the way this MR was written would return the D and E

Then the result would be:

command result
main..renovate/x X ✅
renovate/x..main D, E ❌
renovate/x ^main X ✅
main ^renovate/x D, E ❌

Of course, if there were some rebasing done in between, it would be a bit different.
Let's say we rebased C, D, E on the main branch (if for whatever reason you were to rebase on main).
Or well, pretty much anything that rewrites history, like someone else force-pushing over someone else's changes.

gitGraph
       commit id: "A"
       commit id: "B"
       branch renovate/x
       checkout renovate/x
       commit id: "C"
       commit id: "X"
       checkout main
       commit id: "C - rebased"
       commit id: "D - rebased"
       commit id: "E - rebased"

Then the result would be:

command result
main..renovate/x C, X
renovate/x..main C - rebased, D - rebased, E - rebased
renovate/x ^main C, X
main ^renovate/x C - rebased, D - rebased, E - rebased

I don't know what the expected behaviour should be in the case the main was force-pushed, but you can at least use these tables as reference if you wanted this feature again 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebase would be an issue now too. I'm not too worried about that. it shouldn't be a normal workflow with renovate. can we detect that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'll try to create this PR again with proper tests

'--',
])
)
.trim()
.split('\n')
rarkins marked this conversation as resolved.
Show resolved Hide resolved
),
];
Keysox marked this conversation as resolved.
Show resolved Hide resolved
} 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('\n')
rarkins marked this conversation as resolved.
Show resolved Hide resolved
),
];
}
} catch (err) /* istanbul ignore next */ {
if (err.message?.includes('fatal: bad revision')) {
logger.debug(
Expand All @@ -654,26 +685,19 @@ 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)) {
branchIsModified = true;
Keysox marked this conversation as resolved.
Show resolved Hide resolved
}
}
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(
Expand Down
2 changes: 1 addition & 1 deletion lib/workers/repository/config-migration/branch/rebase.ts
View file
Open in desktop
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)) {
if (await scm.isBranchModified(branchName, config.baseBranch)) {
logger.debug('Migration branch has been edited and cannot be rebased');
return null;
}
Expand Down
5 changes: 4 additions & 1 deletion lib/workers/repository/update/branch/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,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') {
Expand Down
4 changes: 2 additions & 2 deletions lib/workers/repository/update/branch/reuse.ts
View file
Open in desktop
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)) {
if (await scm.isBranchModified(branchName, baseBranch)) {
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)) === 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');
Expand Down
2 changes: 1 addition & 1 deletion lib/workers/repository/update/pr/automerge.ts
View file
Open in desktop
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)) {
if (await scm.isBranchModified(branchName, config.baseBranch)) {
logger.debug('PR is ready for automerge but has been modified');
return {
automerged: false,
Expand Down