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): fix repo scan result caching #6179

Merged
merged 6 commits into from
Jun 19, 2024
Merged

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Jun 14, 2024

What this PR does / why we need it:

This PR fixes cache key construction in GitRepoHandler that accounts for different includes/excludes and filter functions for the same path.

Originally, the caching was implemented in #5710. This PR makes sure that 2 filter-objects are considered tom have the equal cache keys if the difference is only in the order of the augmentedExcludes and augmentedIncludes arrays, ad there is no difference between the sets of values of both arrays.

Which issue(s) this PR fixes:

A follow-up PR for #5526 and #5710

Special notes for your reviewer:

The changes made #5710 make the caching in GitRepoHandler.scanRoot(...) unnecessary. That caching logic will be removed in a separate PR. The caching in GitRepoHandler.scanRoot(...) might still make sense. It caches the results for the whole path, without taking include/exclude filters into account.

@vvagaytsev vvagaytsev requested a review from thsig June 14, 2024 10:09
@vvagaytsev vvagaytsev marked this pull request as draft June 14, 2024 11:18
@vvagaytsev vvagaytsev force-pushed the fix/git-repo-caching branch from efc4e12 to bc6e445 Compare June 17, 2024 09:43
@vvagaytsev vvagaytsev force-pushed the fix/git-repo-caching branch from bc6e445 to 0bb7efc Compare June 17, 2024 10:08
@vvagaytsev vvagaytsev force-pushed the fix/git-repo-caching branch from 0bb7efc to 1230f51 Compare June 17, 2024 10:16
@vvagaytsev vvagaytsev requested review from stefreak and twelvemo June 17, 2024 10:17
@vvagaytsev vvagaytsev marked this pull request as ready for review June 17, 2024 10:19
@vvagaytsev vvagaytsev enabled auto-merge June 17, 2024 13:14
@vvagaytsev vvagaytsev force-pushed the fix/git-repo-caching branch from 088fec6 to 667e5ba Compare June 17, 2024 13:16
@vvagaytsev vvagaytsev force-pushed the fix/git-repo-caching branch from 667e5ba to 905fa68 Compare June 17, 2024 13:37
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me and this is a good fix

I have one minor comment about the function signature of getHashedFilterParams; I want to make sure that we are not hiding details that are important later to understand implications of making changes to the code

}) {
return hashString(
stableStringify({
filter: filter ? filter.toString() : undefined, // We hash the source code of the filter function if provided.
Copy link
Member

Choose a reason for hiding this comment

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

Because it's such an unusual thing to do, and it has some implications (e.g. captured variables will not affect the hash key) I think it's better to not hide the fact that we are converting the function to a string, by changing the signature of the function, something like the following:

export function getHashedFilterParams({
  filterFnSource,
  augmentedIncludes,
  augmentedExcludes,
}: {
  filterFnSource: string | undefined
  augmentedIncludes: string[]
  augmentedExcludes: string[]
}) {

Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Let's merge this anyway, the nit can also be addressed later.

@vvagaytsev vvagaytsev added this pull request to the merge queue Jun 19, 2024
Merged via the queue into main with commit c276e86 Jun 19, 2024
40 checks passed
@vvagaytsev vvagaytsev deleted the fix/git-repo-caching branch June 19, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants