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(git/cache): getCachedBehindBaseResult returns true when branch is not modified #16752

Conversation

Gabriel-Ladzaretti
Copy link
Collaborator

@Gabriel-Ladzaretti Gabriel-Ladzaretti commented Jul 25, 2022

Changes

In isBranchModified

export async function isBranchModified(branchName: string): Promise<boolean> {
if (!branchExists(branchName)) {
logger.debug(

we cache branch data regardless of repositoryCache -

setCachedModifiedResult(
branchName,
config.branchCommits[branchName],
false
);

and
setCachedModifiedResult(branchName, config.branchCommits[branchName], true);

therefore, if we later query getCachedBehindBaseResult it might return true regardless if the branch have been modified or not.

this is because cacheBranch is defined, but cachedBranch.parentSha is not.
if which case the return statement will always be true.

if (!cachedBranch) {
return null;
}
return currentBaseBranchSha !== cachedBranch.parentSha;
}

This in turn effects isBehindBase
due to:

export async function isBranchBehindBase(branchName: string): Promise<boolean> {
const { currentBranchSha } = config;
let isBehind = getCachedBehindBaseResult(branchName, currentBranchSha);
if (isBehind !== null) {
return isBehind;
}

Context

closes #16751

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@Gabriel-Ladzaretti Gabriel-Ladzaretti force-pushed the getCachedBehindBaseResult_null_check_parant_sha branch from 2d1d34d to 3f39f42 Compare July 25, 2022 11:25
viceice
viceice previously approved these changes Jul 25, 2022
@Gabriel-Ladzaretti
Copy link
Collaborator Author

Gabriel-Ladzaretti commented Jul 25, 2022

dont merge it yet, there is some git/index failing test, i want test it some further
coverage related, but it is a bit strage nonetheless

@viceice viceice marked this pull request as draft July 25, 2022 12:28
@rarkins
Copy link
Collaborator

rarkins commented Jul 25, 2022

image

@Gabriel-Ladzaretti
Copy link
Collaborator Author

git.getBranchParentSha was always returning null, which supposed to be a parent sha in one of the tests which caused coverage issues, as we handle this exact case and we end up with branch is not being covered.

im just using HEAD's sha instead.

@Gabriel-Ladzaretti Gabriel-Ladzaretti marked this pull request as ready for review July 25, 2022 13:34
@rarkins rarkins enabled auto-merge (squash) July 26, 2022 04:30
@rarkins rarkins merged commit 2446f44 into renovatebot:main Jul 26, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.127.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isBehindBase might return true when repo cache is disabled and branch is not modified
4 participants