Skip to content

Conversation

@shinin-light
Copy link
Contributor

@shinin-light shinin-light commented Apr 19, 2021

COD-360

  • Ready for review
  • Linked to Jira ticket

What does this PR do?

Our current .gitignore parsing doesn't work with negative ignores (!-rules). Long story short, it seems that the library on which we base our glob processing (https://github.com/micromatch/micromatch) is not working as expected.

For example, an option we use called basename is described as "If set, then patterns without slashes will be matched against the basename of the path if it contains slashes", but in reality it always compares the pattern to the basename of the file, regardless if the pattern has a slash or not. This specific feature was already addressed by someone who wanted to use micromatch to parse .gitignore files, and it was briefly fixed in version 3.x, however they since reverted the change and now there is no mention of the tests that should have future-proofed this feature.
Another issue is that positive glob matching (i.e. finding all the files matching our filters) and negative glob matching (i.e. excluding files following the .gitignore rules) cannot be unified in a single call to micromatch. Trying to use intersections with negations-of-negations would not work properly due to Set Theory. Therefore we need to first filter based on extension and then again based on ignores.

Where should the reviewer start?

Check implementation of filterIgnoredFiles and parseFileIgnores and their occurrences.

How should this be manually tested?

  • have a running deepcode instance locally
  • SNYK_API_HOST=http://localhost:8080 SNYK_API_KEY=<a deepcode sessionToken from your GitHub account in the snyk org> npm run test

@shinin-light shinin-light requested review from a team and miiila April 19, 2021 12:19
@shinin-light shinin-light changed the title fix: fixed parsing of negative ignores and added tests [COD-360] fix: fixed parsing of negative ignores and added tests Apr 19, 2021
@shinin-light shinin-light force-pushed the fix/negative_ignores branch from 9204dec to 18373a1 Compare April 19, 2021 12:21
@shinin-light shinin-light force-pushed the fix/negative_ignores branch from 18373a1 to f44852c Compare April 19, 2021 12:26
@shinin-light shinin-light requested a review from j-sp4 April 20, 2021 09:59
}

function searchFiles(
async function* searchFiles(
Copy link
Contributor

Choose a reason for hiding this comment

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

I learn something new everyday function* cool thanks 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'a generator: instead of returning a value or a promise itself, it returns an async generator so you can then use for (await value of results)...

Copy link
Contributor

@j-sp4 j-sp4 left a comment

Choose a reason for hiding this comment

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

This really took me some time to get my head around! Great Job it looks good to me!

@Arvi3d Arvi3d self-requested a review April 22, 2021 11:40
@shinin-light shinin-light merged commit b9b4367 into develop Apr 26, 2021
@shinin-light shinin-light deleted the fix/negative_ignores branch April 26, 2021 13:12
@snyksec
Copy link

snyksec commented May 3, 2021

🎉 This PR is included in version 3.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants