codeowners icon indicating copy to clipboard operation
codeowners copied to clipboard

Hide files excluded by .gitignore

Open ti-mo opened this issue 1 year ago • 2 comments

Fixes #14

This commit adds support for hiding files from output that have been excluded
by .gitignore and friends. I've implemented a few version of this and settled
on just `git ls-files` since it's likely going to be the most correct and
maintainable solution.

I've tried github.com/boyter/gocodewalker but it's a complex piece of machinery
and was much slower on a repo of 15k files (Cilium) than just git ls-files.
I also tried out github.com/ianlewis/go-gitignore directly, but it doesn't pick
up .gitignore files in subdirs automatically, nor the system-wide gitignore.

I figured since the overwhelming majority of users will be running this in CI
where Git will always be present, relying on the canonical .gitignore
implementation (git itself) is the safest option.

ti-mo avatar Jul 24 '24 09:07 ti-mo

Imho relying on the git cli command is not optimal.

Refactoring findRepositoryRoot would completely remove the need for exec breakout.

rwese avatar Jul 24 '24 11:07 rwese

Honestly, I wouldn't reach for something much more complex. It's pretty involved to implement correctly, although the codeowners lib already has fnmatch -> regex compilation, which is a large chunk of what's needed. Hard to tell if it works exactly like git's, though.

If we use git as the source of truth for files to manage, there shouldn't be any surprises, it should just work. If someone wants to swap in a pure-Go implementation later, that's fine, but I personally don't think it's worth the engineering effort for a small tool like this.

ti-mo avatar Jul 24 '24 22:07 ti-mo

I am not the owner of this repo, but my 2 cents after reviewing the pr:

  1. test would be nice, could be accomplished by adding testdata/.gitignore and force add a testdata/ignored-file then have an example to check this.
  2. having a flag to enable it, to not break backward compatibility.

rwese avatar Oct 22 '24 16:10 rwese

having a flag to enable it, to not break backward compatibility.

@rwese Can you provide an example of something that this change would break?

icholy avatar Nov 19 '24 14:11 icholy

@hmarr Ping in case you missed it, please let me know if you're interested in taking this patch. Will close in a week if no answer.

ti-mo avatar Feb 28 '25 16:02 ti-mo