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: behind-base cache logic #17656

Closed
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/util/cache/repository/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ export interface BranchCache {
isModified: boolean;
prNo: number | null;
sha: string | null;
/**
* Parent commit of branch sha(latest branch commit)
*/
parentSha: string | null;
upgrades: BranchUpgradeCache[];
branchFingerprint?: string;
baseBranchSha: string | null;
}

export interface RepoCacheData {
Expand Down
19 changes: 12 additions & 7 deletions lib/util/git/behind-base-branch-cache.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import { mocked, partial } from '../../../test/util';
import * as _repositoryCache from '../cache/repository';
import type { BranchCache, RepoCacheData } from '../cache/repository/types';
import { getCachedBehindBaseResult } from './behind-base-branch-cache';
import * as _git from '.';

jest.mock('../cache/repository');
jest.mock('.');
const repositoryCache = mocked(_repositoryCache);
const git = mocked(_git);

describe('util/git/behind-base-branch-cache', () => {
let repoCache: RepoCacheData = {};
Expand All @@ -16,11 +19,11 @@ describe('util/git/behind-base-branch-cache', () => {

describe('getCachedBehindBaseResult', () => {
it('returns null if cache is not populated', () => {
expect(getCachedBehindBaseResult('foo', '111')).toBeNull();
expect(getCachedBehindBaseResult('foo', 'base_branch')).toBeNull();
});

it('returns null if branch not found', () => {
expect(getCachedBehindBaseResult('foo', '111')).toBeNull();
expect(getCachedBehindBaseResult('foo', 'base_branch')).toBeNull();
});

it('returns null if cache is partially defined', () => {
Expand All @@ -31,21 +34,23 @@ describe('util/git/behind-base-branch-cache', () => {
});
const repoCache: RepoCacheData = { branches: [branchCache] };
repositoryCache.getCache.mockReturnValue(repoCache);
expect(getCachedBehindBaseResult(branchName, '111')).toBeNull();
expect(getCachedBehindBaseResult(branchName, 'base_branch')).toBeNull();
});

it('returns true if target SHA has changed', () => {
repoCache.branches = [
{ branchName: 'foo', sha: 'aaa', parentSha: '222' } as BranchCache,
{ branchName: 'foo', sha: 'aaa', baseBranchSha: '222' } as BranchCache,
];
expect(getCachedBehindBaseResult('foo', '111')).toBeTrue();
git.getBranchCommit.mockReturnValue('111');
expect(getCachedBehindBaseResult('foo', 'base_branch')).toBeTrue();
});

it('returns false if target SHA has not changed', () => {
repoCache.branches = [
{ branchName: 'foo', sha: 'aaa', parentSha: '111' } as BranchCache,
{ branchName: 'foo', sha: 'aaa', baseBranchSha: '222' } as BranchCache,
];
expect(getCachedBehindBaseResult('foo', '111')).toBeFalse();
git.getBranchCommit.mockReturnValue('222');
expect(getCachedBehindBaseResult('foo', 'base_branch')).toBeFalse();
});
});
});
12 changes: 7 additions & 5 deletions lib/util/git/behind-base-branch-cache.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import { getCache } from '../cache/repository';
import { getBranchCommit } from '.';

// Compare cached parent Sha of a branch to the fetched base-branch sha to determine whether the branch is behind the base
// Since cache is updated after each run, this will be sufficient to determine whether a branch is behind its parent.
// Compare cached baseBranchSha of a branch and fetched baseBranchSha to determine if the branch is behind base
// since we update cache on each run, it is sufficient to determine whether a branch is behind its parent
export function getCachedBehindBaseResult(
branchName: string,
currentBaseBranchSha: string
baseBranchName: string
): boolean | null {
const cache = getCache();
const baseBranchSha = getBranchCommit(baseBranchName);
const { branches = [] } = cache;
const cachedBranch = branches?.find(
(branch) => branch.branchName === branchName
);

if (!cachedBranch?.parentSha) {
if (!cachedBranch?.baseBranchSha) {
return null;
}

return currentBaseBranchSha !== cachedBranch.parentSha;
return baseBranchSha !== cachedBranch.baseBranchSha;
}
15 changes: 9 additions & 6 deletions lib/util/git/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,26 +249,29 @@ describe('util/git/index', () => {
it('should return false if same SHA as master', async () => {
repoCache.getCache.mockReturnValueOnce({});
expect(
await git.isBranchBehindBase('renovate/future_branch')
await git.isBranchBehindBase('renovate/future_branch', defaultBranch)
).toBeFalse();
});

it('should return true if SHA different from master', async () => {
repoCache.getCache.mockReturnValueOnce({});
expect(await git.isBranchBehindBase('renovate/past_branch')).toBeTrue();
expect(
await git.isBranchBehindBase('renovate/past_branch', defaultBranch)
).toBeTrue();
});

it('should return result even if non-default and not under branchPrefix', async () => {
const parentSha = 'SHA';
const branchCache = partial<BranchCache>({
branchName: 'develop',
parentSha: parentSha,
baseBranchSha: git.getBranchCommit(defaultBranch),
});
repoCache.getCache.mockReturnValueOnce({}).mockReturnValueOnce({
branches: [branchCache],
});
expect(await git.isBranchBehindBase('develop')).toBeTrue();
expect(await git.isBranchBehindBase('develop')).toBeTrue(); // cache
expect(await git.isBranchBehindBase('develop', defaultBranch)).toBeTrue();
expect(
await git.isBranchBehindBase('develop', defaultBranch)
).toBeFalse(); // cache
});
});

Expand Down
11 changes: 6 additions & 5 deletions lib/util/git/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -558,10 +558,11 @@ export function getBranchList(): string[] {
return Object.keys(config.branchCommits);
}

export async function isBranchBehindBase(branchName: string): Promise<boolean> {
const { currentBranchSha } = config;

let isBehind = getCachedBehindBaseResult(branchName, currentBranchSha);
export async function isBranchBehindBase(
branchName: string,
baseBranch: string
): Promise<boolean> {
let isBehind = getCachedBehindBaseResult(branchName, baseBranch);
if (isBehind !== null) {
return isBehind;
}
Expand All @@ -573,7 +574,7 @@ export async function isBranchBehindBase(branchName: string): Promise<boolean> {
'--remotes',
'--verbose',
'--contains',
config.currentBranchSha,
currentBranchSha,
]);
isBehind = !branches.all.map(localName).includes(branchName);
logger.debug(
Expand Down
4 changes: 4 additions & 0 deletions lib/workers/repository/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ async function generateBranchCache(
): Promise<BranchCache | null> {
const { branchName } = branch;
try {
const baseBranchName = branch.baseBranch ?? branch.defaultBranch;
// TODO: (fix types) #7154
RahulGautamSingh marked this conversation as resolved.
Show resolved Hide resolved
const baseBranchSha = getBranchCommit(baseBranchName!) ?? null;
const sha = getBranchCommit(branchName) ?? null;
let prNo = null;
let parentSha = null;
Expand Down Expand Up @@ -81,6 +84,7 @@ async function generateBranchCache(
isModified,
upgrades,
branchFingerprint,
baseBranchSha,
};
} catch (error) {
const err = error.err || error; // external host error nests err
Expand Down
6 changes: 3 additions & 3 deletions lib/workers/repository/onboarding/branch/rebase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,18 @@ export async function rebaseOnboardingBranch(
config: RenovateConfig
): Promise<string | null> {
logger.debug('Checking if onboarding branch needs rebasing');
// TODO #7154
// TODO: (fix types) #7154
RahulGautamSingh marked this conversation as resolved.
Show resolved Hide resolved
if (await isBranchModified(config.onboardingBranch!)) {
logger.debug('Onboarding branch has been edited and cannot be rebased');
return null;
}
const configFile = defaultConfigFile(config);
const existingContents = await getFile(configFile, config.onboardingBranch);
const contents = await getOnboardingConfigContents(config, configFile);
// TODO #7154
// TODO: (fix types) #7154
RahulGautamSingh marked this conversation as resolved.
Show resolved Hide resolved
if (
contents === existingContents &&
!(await isBranchBehindBase(config.onboardingBranch!))
!(await isBranchBehindBase(config.onboardingBranch!, config.defaultBranch!))
) {
logger.debug('Onboarding branch is up to date');
return null;
Expand Down
3 changes: 2 additions & 1 deletion lib/workers/repository/update/branch/reuse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ export async function shouldReuseExistingBranch(
(config.rebaseWhen === 'auto' &&
(config.automerge || (await platform.getRepoForceRebase())))
) {
if (await isBranchBehindBase(branchName)) {
// TODO: (fix types) #7154
RahulGautamSingh marked this conversation as resolved.
Show resolved Hide resolved
if (await 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 isBranchModified(branchName)) {
Expand Down