From b18575b5355962a1ce382bad50cc575286ab61f6 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 6 Sep 2022 16:04:21 +0530 Subject: [PATCH 1/6] fix behind-base cache logic --- lib/util/cache/repository/types.ts | 1 + lib/util/git/behind-base-branch-cache.spec.ts | 4 ++-- lib/util/git/behind-base-branch-cache.ts | 4 ++-- lib/util/git/index.spec.ts | 10 ++++++---- lib/util/git/index.ts | 11 ++++++----- lib/workers/repository/cache.ts | 3 +++ lib/workers/repository/onboarding/branch/rebase.ts | 3 ++- lib/workers/repository/update/branch/reuse.ts | 3 ++- 8 files changed, 24 insertions(+), 15 deletions(-) diff --git a/lib/util/cache/repository/types.ts b/lib/util/cache/repository/types.ts index dd8df63d2cfed3..e4797dba66fbec 100644 --- a/lib/util/cache/repository/types.ts +++ b/lib/util/cache/repository/types.ts @@ -35,6 +35,7 @@ export interface BranchCache { parentSha: string | null; upgrades: BranchUpgradeCache[]; branchFingerprint?: string; + baseBranchSha: string | null; } export interface RepoCacheData { diff --git a/lib/util/git/behind-base-branch-cache.spec.ts b/lib/util/git/behind-base-branch-cache.spec.ts index 30d7fa9a6b1320..80876455fe14e7 100644 --- a/lib/util/git/behind-base-branch-cache.spec.ts +++ b/lib/util/git/behind-base-branch-cache.spec.ts @@ -36,14 +36,14 @@ describe('util/git/behind-base-branch-cache', () => { 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(); }); it('returns false if target SHA has not changed', () => { repoCache.branches = [ - { branchName: 'foo', sha: 'aaa', parentSha: '111' } as BranchCache, + { branchName: 'foo', sha: 'aaa', baseBranchSha: '111' } as BranchCache, ]; expect(getCachedBehindBaseResult('foo', '111')).toBeFalse(); }); diff --git a/lib/util/git/behind-base-branch-cache.ts b/lib/util/git/behind-base-branch-cache.ts index 6283062df78fac..af87f067026e52 100644 --- a/lib/util/git/behind-base-branch-cache.ts +++ b/lib/util/git/behind-base-branch-cache.ts @@ -12,9 +12,9 @@ export function getCachedBehindBaseResult( (branch) => branch.branchName === branchName ); - if (!cachedBranch?.parentSha) { + if (!cachedBranch?.baseBranchSha) { return null; } - return currentBaseBranchSha !== cachedBranch.parentSha; + return currentBaseBranchSha !== cachedBranch.baseBranchSha; } diff --git a/lib/util/git/index.spec.ts b/lib/util/git/index.spec.ts index 8822a6eb1062b4..ce2fb14ba432c4 100644 --- a/lib/util/git/index.spec.ts +++ b/lib/util/git/index.spec.ts @@ -249,13 +249,15 @@ 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 () => { @@ -267,8 +269,8 @@ describe('util/git/index', () => { 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)).toBeTrue(); // cache }); }); diff --git a/lib/util/git/index.ts b/lib/util/git/index.ts index 00dd79e1adf3dc..0ede9de28c14a8 100644 --- a/lib/util/git/index.ts +++ b/lib/util/git/index.ts @@ -558,10 +558,11 @@ export function getBranchList(): string[] { return Object.keys(config.branchCommits); } -export async function isBranchBehindBase(branchName: string): Promise { - const { currentBranchSha } = config; - - let isBehind = getCachedBehindBaseResult(branchName, currentBranchSha); +export async function isBranchBehindBase( + branchName: string, + baseBranch: string +): Promise { + let isBehind = getCachedBehindBaseResult(branchName, baseBranch); if (isBehind !== null) { return isBehind; } @@ -573,7 +574,7 @@ export async function isBranchBehindBase(branchName: string): Promise { '--remotes', '--verbose', '--contains', - config.currentBranchSha, + currentBranchSha, ]); isBehind = !branches.all.map(localName).includes(branchName); logger.debug( diff --git a/lib/workers/repository/cache.ts b/lib/workers/repository/cache.ts index 5211d576681462..063875db310f76 100644 --- a/lib/workers/repository/cache.ts +++ b/lib/workers/repository/cache.ts @@ -49,6 +49,8 @@ async function generateBranchCache( ): Promise { const { branchName } = branch; try { + const baseBranchName = branch.baseBranch ?? branch.defaultBranch; + const baseBranchSha = getBranchCommit(baseBranchName!) ?? null; const sha = getBranchCommit(branchName) ?? null; let prNo = null; let parentSha = null; @@ -81,6 +83,7 @@ async function generateBranchCache( isModified, upgrades, branchFingerprint, + baseBranchSha, }; } catch (error) { const err = error.err || error; // external host error nests err diff --git a/lib/workers/repository/onboarding/branch/rebase.ts b/lib/workers/repository/onboarding/branch/rebase.ts index 61b7073530c56d..4fedf11ef90298 100644 --- a/lib/workers/repository/onboarding/branch/rebase.ts +++ b/lib/workers/repository/onboarding/branch/rebase.ts @@ -31,7 +31,8 @@ export async function rebaseOnboardingBranch( // TODO #7154 if ( contents === existingContents && - !(await isBranchBehindBase(config.onboardingBranch!)) + // TODO: (fix types) #7154 + !(await isBranchBehindBase(config.onboardingBranch!, config.defaultBranch!)) ) { logger.debug('Onboarding branch is up to date'); return null; diff --git a/lib/workers/repository/update/branch/reuse.ts b/lib/workers/repository/update/branch/reuse.ts index 386f7c62a04393..fafd2ebf361c4e 100644 --- a/lib/workers/repository/update/branch/reuse.ts +++ b/lib/workers/repository/update/branch/reuse.ts @@ -62,7 +62,8 @@ export async function shouldReuseExistingBranch( (config.rebaseWhen === 'auto' && (config.automerge || (await platform.getRepoForceRebase()))) ) { - if (await isBranchBehindBase(branchName)) { + // TODO: (fix types) #7154 + 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)) { From ae43f70dba8fa0c8c80515092522f6a8a792099a Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 6 Sep 2022 17:40:25 +0530 Subject: [PATCH 2/6] fix coverage --- lib/util/git/behind-base-branch-cache.ts | 6 ++++-- lib/util/git/index.spec.ts | 7 ++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/util/git/behind-base-branch-cache.ts b/lib/util/git/behind-base-branch-cache.ts index af87f067026e52..c70c4998019e21 100644 --- a/lib/util/git/behind-base-branch-cache.ts +++ b/lib/util/git/behind-base-branch-cache.ts @@ -1,12 +1,14 @@ 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. 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 @@ -16,5 +18,5 @@ export function getCachedBehindBaseResult( return null; } - return currentBaseBranchSha !== cachedBranch.baseBranchSha; + return baseBranchSha !== cachedBranch.baseBranchSha; } diff --git a/lib/util/git/index.spec.ts b/lib/util/git/index.spec.ts index ce2fb14ba432c4..3b171104c8ceb3 100644 --- a/lib/util/git/index.spec.ts +++ b/lib/util/git/index.spec.ts @@ -261,16 +261,17 @@ describe('util/git/index', () => { }); it('should return result even if non-default and not under branchPrefix', async () => { - const parentSha = 'SHA'; const branchCache = partial({ branchName: 'develop', - parentSha: parentSha, + baseBranchSha: git.getBranchCommit(defaultBranch), }); repoCache.getCache.mockReturnValueOnce({}).mockReturnValueOnce({ branches: [branchCache], }); expect(await git.isBranchBehindBase('develop', defaultBranch)).toBeTrue(); - expect(await git.isBranchBehindBase('develop', defaultBranch)).toBeTrue(); // cache + expect( + await git.isBranchBehindBase('develop', defaultBranch) + ).toBeFalse(); // cache }); }); From f9b0b3279b1cdb3e344ffe28f1fece3ade99239c Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 6 Sep 2022 18:05:52 +0530 Subject: [PATCH 3/6] update tests --- lib/util/git/behind-base-branch-cache.spec.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/util/git/behind-base-branch-cache.spec.ts b/lib/util/git/behind-base-branch-cache.spec.ts index 80876455fe14e7..141fbc9fc1e9af 100644 --- a/lib/util/git/behind-base-branch-cache.spec.ts +++ b/lib/util/git/behind-base-branch-cache.spec.ts @@ -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 = {}; @@ -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', () => { @@ -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', 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', baseBranchSha: '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(); }); }); }); From 9e152e020fd1f63ddce62d0a9919b0316fbbb42c Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 6 Sep 2022 18:36:48 +0530 Subject: [PATCH 4/6] update and add comments --- lib/util/cache/repository/types.ts | 3 +++ lib/util/git/behind-base-branch-cache.ts | 4 ++-- lib/workers/repository/cache.ts | 1 + lib/workers/repository/onboarding/branch/rebase.ts | 5 ++--- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/util/cache/repository/types.ts b/lib/util/cache/repository/types.ts index e4797dba66fbec..055696eb9e7ba9 100644 --- a/lib/util/cache/repository/types.ts +++ b/lib/util/cache/repository/types.ts @@ -32,6 +32,9 @@ export interface BranchCache { isModified: boolean; prNo: number | null; sha: string | null; + /** + * Parent commit branch sha + */ parentSha: string | null; upgrades: BranchUpgradeCache[]; branchFingerprint?: string; diff --git a/lib/util/git/behind-base-branch-cache.ts b/lib/util/git/behind-base-branch-cache.ts index c70c4998019e21..28dd6859e9b286 100644 --- a/lib/util/git/behind-base-branch-cache.ts +++ b/lib/util/git/behind-base-branch-cache.ts @@ -1,8 +1,8 @@ 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, baseBranchName: string diff --git a/lib/workers/repository/cache.ts b/lib/workers/repository/cache.ts index 063875db310f76..5c7d07b573f944 100644 --- a/lib/workers/repository/cache.ts +++ b/lib/workers/repository/cache.ts @@ -50,6 +50,7 @@ async function generateBranchCache( const { branchName } = branch; try { const baseBranchName = branch.baseBranch ?? branch.defaultBranch; + // TODO: (fix types) #7154 const baseBranchSha = getBranchCommit(baseBranchName!) ?? null; const sha = getBranchCommit(branchName) ?? null; let prNo = null; diff --git a/lib/workers/repository/onboarding/branch/rebase.ts b/lib/workers/repository/onboarding/branch/rebase.ts index 4fedf11ef90298..f6b19a78304b49 100644 --- a/lib/workers/repository/onboarding/branch/rebase.ts +++ b/lib/workers/repository/onboarding/branch/rebase.ts @@ -20,7 +20,7 @@ export async function rebaseOnboardingBranch( config: RenovateConfig ): Promise { logger.debug('Checking if onboarding branch needs rebasing'); - // TODO #7154 + // TODO: (fix types) #7154 if (await isBranchModified(config.onboardingBranch!)) { logger.debug('Onboarding branch has been edited and cannot be rebased'); return null; @@ -28,10 +28,9 @@ export async function rebaseOnboardingBranch( const configFile = defaultConfigFile(config); const existingContents = await getFile(configFile, config.onboardingBranch); const contents = await getOnboardingConfigContents(config, configFile); - // TODO #7154 + // TODO: (fix types) #7154 if ( contents === existingContents && - // TODO: (fix types) #7154 !(await isBranchBehindBase(config.onboardingBranch!, config.defaultBranch!)) ) { logger.debug('Onboarding branch is up to date'); From b37f591f50c9b9719af28fbe8845187b41ee1dae Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 6 Sep 2022 19:32:08 +0530 Subject: [PATCH 5/6] Update types.ts --- lib/util/cache/repository/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/util/cache/repository/types.ts b/lib/util/cache/repository/types.ts index 055696eb9e7ba9..c92142d00ce792 100644 --- a/lib/util/cache/repository/types.ts +++ b/lib/util/cache/repository/types.ts @@ -33,7 +33,7 @@ export interface BranchCache { prNo: number | null; sha: string | null; /** - * Parent commit branch sha + * Parent commit of branch sha(latest branch commit) */ parentSha: string | null; upgrades: BranchUpgradeCache[]; From 4449803724f21ca1e888c78e4f48474c693dda19 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Wed, 7 Sep 2022 11:43:59 +0530 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Michael Kriese --- lib/workers/repository/cache.ts | 2 +- lib/workers/repository/onboarding/branch/rebase.ts | 4 ++-- lib/workers/repository/update/branch/reuse.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/workers/repository/cache.ts b/lib/workers/repository/cache.ts index 5c7d07b573f944..e45c2766f88fdf 100644 --- a/lib/workers/repository/cache.ts +++ b/lib/workers/repository/cache.ts @@ -50,7 +50,7 @@ async function generateBranchCache( const { branchName } = branch; try { const baseBranchName = branch.baseBranch ?? branch.defaultBranch; - // TODO: (fix types) #7154 + // TODO: fix types (#7154) const baseBranchSha = getBranchCommit(baseBranchName!) ?? null; const sha = getBranchCommit(branchName) ?? null; let prNo = null; diff --git a/lib/workers/repository/onboarding/branch/rebase.ts b/lib/workers/repository/onboarding/branch/rebase.ts index f6b19a78304b49..9535c187ca4654 100644 --- a/lib/workers/repository/onboarding/branch/rebase.ts +++ b/lib/workers/repository/onboarding/branch/rebase.ts @@ -20,7 +20,7 @@ export async function rebaseOnboardingBranch( config: RenovateConfig ): Promise { logger.debug('Checking if onboarding branch needs rebasing'); - // TODO: (fix types) #7154 + // TODO: fix types (#7154) if (await isBranchModified(config.onboardingBranch!)) { logger.debug('Onboarding branch has been edited and cannot be rebased'); return null; @@ -28,7 +28,7 @@ export async function rebaseOnboardingBranch( const configFile = defaultConfigFile(config); const existingContents = await getFile(configFile, config.onboardingBranch); const contents = await getOnboardingConfigContents(config, configFile); - // TODO: (fix types) #7154 + // TODO: fix types (#7154) if ( contents === existingContents && !(await isBranchBehindBase(config.onboardingBranch!, config.defaultBranch!)) diff --git a/lib/workers/repository/update/branch/reuse.ts b/lib/workers/repository/update/branch/reuse.ts index fafd2ebf361c4e..39e4e55f11bfd8 100644 --- a/lib/workers/repository/update/branch/reuse.ts +++ b/lib/workers/repository/update/branch/reuse.ts @@ -62,7 +62,7 @@ export async function shouldReuseExistingBranch( (config.rebaseWhen === 'auto' && (config.automerge || (await platform.getRepoForceRebase()))) ) { - // TODO: (fix types) #7154 + // TODO: fix types (#7154) 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