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 file list caching bug for repo mode #5710

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Feb 6, 2024

What this PR does / why we need it:

This fixes an error that would come up e.g. when an action config in the project root excluded a subdirectory containing another action config.

In that setup, the file list for the action in the subdirectory would be empty because the file list had been cached using the exclude config applied when computing the file list for the action config at the root level (which included no files for the excluded subdirectory).

This was fixed by adding another level of caching to GitRepoHandler that accounts for different includes/excludes and filter functions for the same path.

Special notes for your reviewer:

Note the slightly awkward clarification section about the difference between the exclude behavior of subtree and repo when no includes are configured.

In a follow-up to this, we really should address #5582.

I think bringing our include/exclude behavior completely in line with Git's would make things easier to reason about for our users.

This fixes an error that would come up e.g. when an action config in the
project root excluded a subdirectory containing another action config.

In that setup, the file list for the action in the subdirectory would be
empty because the file list had been cached using the exclude config
applied when computing the file list for the action config at the root
level (which included no files for the excluded subdirectory).

This was fixed by adding another level of caching to `GitRepoHandler`
that accounts for different includes/excludes and filter functions for
the same path.
Copy link
Collaborator

@twelvemo twelvemo left a comment

Choose a reason for hiding this comment

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

Looks good! For others reviewing, we decided to favor the inconsistency between the two file scanning modes, to shimming the behavior of the subtree mode which excludes all occurrences foo.txt in every subdir leading to unexpected excludes.

@thsig thsig added this pull request to the merge queue Feb 7, 2024
Merged via the queue into main with commit ca7f997 Feb 7, 2024
44 checks passed
@thsig thsig deleted the repo-scan-exclude-fix branch February 7, 2024 11:15
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.

None yet

2 participants