feat: improve branch cache logic (#17848)

Co-authored-by: Rhys Arkins <[email protected]>

lib/util/cache/repository/types.ts

@@ -31,6 +31,14 @@ export interface BranchCache {
    *Whether this branch has automerge enabled
    */
   automerge: boolean;
+  /**
+   * Name of base branch
+   */
+  baseBranch: string;
+  /**
+   * The base branch's most recent commit SHA
+   */
+  baseBranchSha: string | null;
   /**
    * Hash of the manager fingerprints and the update branch config
    */
@@ -39,10 +47,14 @@ export interface BranchCache {
    * Branch name
    */
   branchName: string;
+  /**
+   * Whether the update branch is behind base branh
+   */
+  isBehindBase?: boolean;
   /**
    * Whether a person not listed in gitIgnoredAuthors updated the branch.
    */
-  isModified: boolean;
+  isModified?: boolean;
   /**
    * Parent commit of branch sha
    */

lib/util/git/behind-base-branch-cache.spec.ts

@@ -1,7 +1,10 @@
-import { mocked, partial } from '../../../test/util';
+import { logger, 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 {
+  getCachedBehindBaseResult,
+  setCachedBehindBaseResult,
+} from './behind-base-branch-cache';
 
 jest.mock('../cache/repository');
 const repositoryCache = mocked(_repositoryCache);
@@ -16,36 +19,260 @@ describe('util/git/behind-base-branch-cache', () => {
 
   describe('getCachedBehindBaseResult', () => {
     it('returns null if cache is not populated', () => {
-      expect(getCachedBehindBaseResult('foo', '111')).toBeNull();
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
     });
 
     it('returns null if branch not found', () => {
-      expect(getCachedBehindBaseResult('foo', '111')).toBeNull();
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'not_branch',
+            baseBranchSha: 'base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: 'branch_sha',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
     });
 
-    it('returns null if cache is partially defined', () => {
-      const branchName = 'branchName';
-      const branchCache = partial<BranchCache>({
-        branchName,
-        isModified: false,
-      });
-      const repoCache: RepoCacheData = { branches: [branchCache] };
+    it('returns null if base branch SHA is different', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: 'not_base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: 'branch_sha',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
+    });
+
+    it('returns null if branch sha is different', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: 'base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: 'not_branch_sha',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
+    });
+
+    it('returns null if cached value is undefined', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: 'base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: 'not_branch_sha',
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
+    });
+
+    it('returns null if base branch SHA is null', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: null,
+            baseBranch: 'base_branch',
+            sha: 'branch_sha',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
+    });
+
+    it('returns null if branch SHA is null', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: 'base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: null,
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeNull();
+    });
+
+    it('returns cached value', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'branch',
+            baseBranchSha: 'base_branch_sha',
+            baseBranch: 'base_branch',
+            sha: 'branch_sha',
+            isBehindBase: true,
+          }),
+        ],
+      };
       repositoryCache.getCache.mockReturnValue(repoCache);
-      expect(getCachedBehindBaseResult(branchName, '111')).toBeNull();
+      expect(
+        getCachedBehindBaseResult(
+          'branch',
+          'branch_sha',
+          'base_branch',
+          'base_branch_sha'
+        )
+      ).toBeTrue();
+    });
+  });
+
+  describe('setCachedBehindBasedResult', () => {
+    it('returns without updating when cache not populated', () => {
+      setCachedBehindBaseResult('foo', false);
+      expect(repoCache).toEqual({});
+      expect(logger.logger.debug).toHaveBeenCalledWith(
+        'setCachedBehindBaseResult(): Branch cache not present'
+      );
     });
 
-    it('returns true if target SHA has changed', () => {
-      repoCache.branches = [
-        { branchName: 'foo', sha: 'aaa', parentSha: '222' } as BranchCache,
-      ];
-      expect(getCachedBehindBaseResult('foo', '111')).toBeTrue();
+    it('returns without updating when branch not found', () => {
+      setCachedBehindBaseResult('foo', false);
+      expect(repoCache).toEqual({});
+      expect(logger.logger.debug).toHaveBeenCalledWith(
+        'setCachedBehindBaseResult(): Branch cache not present'
+      );
     });
 
-    it('returns false if target SHA has not changed', () => {
-      repoCache.branches = [
-        { branchName: 'foo', sha: 'aaa', parentSha: '111' } as BranchCache,
-      ];
-      expect(getCachedBehindBaseResult('foo', '111')).toBeFalse();
+    it('updates cached value', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'foo',
+            sha: '121',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      setCachedBehindBaseResult('foo', false);
+      expect(repoCache).toEqual({
+        branches: [
+          {
+            branchName: 'foo',
+            sha: '121',
+            isBehindBase: false,
+          },
+        ],
+      });
+    });
+
+    it('handles multiple branches', () => {
+      repoCache = {
+        branches: [
+          partial<BranchCache>({
+            branchName: 'foo-1',
+            sha: '111',
+            isBehindBase: true,
+          }),
+          partial<BranchCache>({
+            branchName: 'foo-2',
+            sha: 'aaa',
+            isBehindBase: false,
+          }),
+          partial<BranchCache>({
+            branchName: 'foo-3',
+            sha: '222',
+            isBehindBase: true,
+          }),
+        ],
+      };
+      repositoryCache.getCache.mockReturnValue(repoCache);
+      setCachedBehindBaseResult('foo-1', false);
+      setCachedBehindBaseResult('foo-2', true);
+      setCachedBehindBaseResult('foo-3', false);
+      expect(repoCache).toEqual({
+        branches: [
+          {
+            branchName: 'foo-1',
+            sha: '111',
+            isBehindBase: false,
+          },
+          {
+            branchName: 'foo-2',
+            sha: 'aaa',
+            isBehindBase: true,
+          },
+          {
+            branchName: 'foo-3',
+            sha: '222',
+            isBehindBase: false,
+          },
+        ],
+      });
     });
   });
 });

lib/util/git/behind-base-branch-cache.ts

@@ -1,20 +1,43 @@
+import { logger } from '../../logger';
 import { getCache } from '../cache/repository';
 
-// 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
+  branchSha: string | null,
+  baseBranch: string,
+  baseBranchSha: string | null
 ): boolean | null {
   const cache = getCache();
-  const { branches = [] } = cache;
-  const cachedBranch = branches?.find(
+  const branch = cache.branches?.find(
     (branch) => branch.branchName === branchName
   );
 
-  if (!cachedBranch?.parentSha) {
-    return null;
+  if (
+    branch &&
+    branch.sha === branchSha &&
+    branch.baseBranch === baseBranch &&
+    branch.baseBranchSha === baseBranchSha &&
+    branch.isBehindBase !== undefined
+  ) {
+    return branch.isBehindBase;
   }
 
-  return currentBaseBranchSha !== cachedBranch.parentSha;
+  return null;
+}
+
+export function setCachedBehindBaseResult(
+  branchName: string,
+  isBehindBase: boolean
+): void {
+  const cache = getCache();
+  const branch = cache.branches?.find(
+    (branch) => branch.branchName === branchName
+  );
+
+  if (!branch) {
+    logger.debug(`setCachedBehindBaseResult(): Branch cache not present`);
+    return;
+  }
+
+  branch.isBehindBase = isBehindBase;
 }