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

Replace grep in filter_ignore_files() with git ls-files --ignored #636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GlassGruber
Copy link

This is a follow up of #591

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. I didn't know about this option and it definitely seems to be the way to go. But I also think that we can now simplify this method a lot. The output of ls-files is a list of file names, not patterns. So we can use that list with easy comparison instead of using glob patterns. Unfortunately, I don't have time to do this myself. Would you like to get more into bash coding?

@GlassGruber
Copy link
Author

Hei there, yea I'm glad to contribute and hopefully I'll not break anything!

Took me a bit to grok the slice and dice in the filtering functions with the NUL separator bit, but eventually I remembered I did something similar before with rip-grep and adapted to grep.

My coding can be a bit verbose with comments and airy with spaces so if you prefer more terseness please edit as you like.

With a couple of very simple tests everything looks like working as expected.
I don't know the potential performance implications of grep in the filtering, I guess nothing really concerning, maybe in case of a very huge commit or similar? Anyway I haven't noticed anything slower in my few tests.

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