numtide/treefmt
Archived
8
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

feat: support .gitignore files #19

Merged
brianmcgee merged 3 commits from feat/support-gigignore into main 2024-01-11 20:52:23 +00:00
Conversation 5 Commits 3 Files Changed 12 +451 -34
brianmcgee commented 2024-01-08 12:52:30 +00:00
Member
Copy Link

Introduces a --walk flag which can be used to tell treefmt how to traverse the directory specified by --tree-root.

By default, it will attempt to use git ls-files. If this fails, it falls back to using the filesystem.

You can explicitly traverse the filesystem instead of using git by providing --walk filesystem.

Close #1

Introduces a `--walk` flag which can be used to tell `treefmt` how to traverse the directory specified by `--tree-root`. By default, it will attempt to use `git ls-files`. If this fails, it falls back to using the filesystem. You can explicitly traverse the filesystem instead of using git by providing `--walk filesystem`. Close #1
brianmcgee added 1 commit 2024-01-08 12:52:30 +00:00 
By default, any files which match `.gitignore` files anywhere within the tree root will be ignored.

This behaviour can be disabled by providing the `--no-gitignore` flag.

Signed-off-by: Brian McGee <brian@bmcgee.ie>
brianmcgee requested review from zimbatm 2024-01-08 12:52:34 +00:00
brianmcgee commented 2024-01-08 17:08:25 +00:00
Author
Member
Copy Link

@zimbatm ping

@zimbatm ping
zimbatm approved these changes 2024-01-08 17:09:07 +00:00
zimbatm left a comment
Owner

The rust ignore crate is a bit weird; it also loads .ignore files and generally doesn't traverse any files starting with a .. That has caused a few issues already. Eg: the latest: https://github.com/numtide/treefmt-nix/pull/146

Side point: there might be an argument that inside of a git repo, we could use git ls-files as the source of listing. This would be much faster than traversing the filesystem.

The rust ignore crate is a bit weird; it also loads `.ignore` files and generally doesn't traverse any files starting with a `.`. That has caused a few issues already. Eg: the latest: https://github.com/numtide/treefmt-nix/pull/146 Side point: there might be an argument that inside of a git repo, we could use `git ls-files` as the source of listing. This would be much faster than traversing the filesystem.
internal/cli/cli.go Outdated
@ -14,6 +14,7 @@ type Options struct {
ConfigFile string `type:"existingfile" default:"./treefmt.toml"`
FailOnChange bool `help:"Exit with error if any changes were made. Useful for CI"`
Formatters []string `help:"Specify formatters to apply. Defaults to all formatters"`
NoGitignore bool `help:"Ignore .gitignore files within the tree root" default:"false"`
zimbatm commented 2024-01-08 17:06:15 +00:00
Owner

The phrasing is a bit ambiguous. Maybe "Don't respect .gitignore files"?

The phrasing is a bit ambiguous. Maybe "Don't respect .gitignore files"?
brianmcgee commented 2024-01-08 17:10:45 +00:00
Author
Member

That's a fair point.

That's a fair point.
brianmcgee marked this conversation as resolved
brianmcgee commented 2024-01-08 17:12:13 +00:00
Author
Member
Copy Link

The rust ignore crate is a bit weird; it also loads .ignore files and generally doesn't traverse any files starting with a .. That has caused a few issues already. Eg: the latest: https://github.com/numtide/treefmt-nix/pull/146

Yeah, I'm currently processing everything and I'd like to keep it like that. Ignoring dot files was brought up on discourse as well.

Side point: there might be an argument that inside of a git repo, we could use git ls-files as the source of listing. This would be much faster than traversing the filesystem.

I think I prefer this rather than polluting things with git related code and logic. Keeps things cleaner

> The rust ignore crate is a bit weird; it also loads `.ignore` files and generally doesn't traverse any files starting with a `.`. That has caused a few issues already. Eg: the latest: https://github.com/numtide/treefmt-nix/pull/146 Yeah, I'm currently processing everything and I'd like to keep it like that. Ignoring dot files was brought up on discourse as well. > Side point: there might be an argument that inside of a git repo, we could use `git ls-files` as the source of listing. This would be much faster than traversing the filesystem. I think I prefer this rather than polluting things with git related code and logic. Keeps things cleaner
brianmcgee commented 2024-01-08 17:12:43 +00:00
Author
Member
Copy Link

@zimbatm if using git ls-files would you make that behaviour opt-in or opt-out?

@zimbatm if using `git ls-files` would you make that behaviour opt-in or opt-out?
zimbatm commented 2024-01-08 17:19:26 +00:00
Owner
Copy Link

If it's following git ls-files then I would not process the .gitignore files. If the user wants to ignore something, they can still ignore them by adding them to the global excludes list.

If it's following `git ls-files` then I would not process the .gitignore files. If the user wants to ignore something, they can still ignore them by adding them to the global excludes list.
brianmcgee force-pushed feat/support-gigignore from acd4997459 to dea0166e2b 2024-01-10 13:34:32 +00:00 Compare
brianmcgee force-pushed feat/support-gigignore from dea0166e2b to 9051aa6d0f 2024-01-10 13:35:26 +00:00 Compare
brianmcgee commented 2024-01-10 13:36:22 +00:00
Author
Member
Copy Link

@zimbatm I changed the approach, using git ls-files instead. I've updated the description and force-pushed.

@zimbatm I changed the approach, using `git ls-files` instead. I've updated the description and force-pushed.
brianmcgee requested review from zimbatm 2024-01-10 13:36:39 +00:00
brianmcgee added 1 commit 2024-01-10 13:38:35 +00:00 
Signed-off-by: Brian McGee <brian@bmcgee.ie>
zimbatm approved these changes 2024-01-10 14:36:24 +00:00
zimbatm left a comment
Owner

Yeah that's great, I think it's going to make the project so much nicer

Yeah that's great, I think it's going to make the project so much nicer
internal/cache/walk.go Outdated
@ -0,0 +80,4 @@
}
return eg.Wait()
}
zimbatm commented 2024-01-10 14:34:27 +00:00
Owner

I think a more idiomatic way would be to have an "internal/walker" package.

There you could have a walker.Walker interface, and host the FileSystemWalker and GitWalker implementations.

And walker.Detect(path string) (Walker, error) could detect and return a walker instance based on the path.

And a walker.New(type string, path string) (Walker, error) in the case where the walker type is named.

I think a more idiomatic way would be to have an "internal/walker" package. There you could have a `walker.Walker` interface, and host the FileSystemWalker and GitWalker implementations. And `walker.Detect(path string) (Walker, error)` could detect and return a walker instance based on the path. And a `walker.New(type string, path string) (Walker, error)` in the case where the walker type is named.
brianmcgee commented 2024-01-10 15:07:26 +00:00
Author
Member

Made some relevant changes in bcc4902ed8

Made some relevant changes in https://git.numtide.com/numtide/treefmt/commit/bcc4902ed849d0742e9be3f49c2ae327a709b375
brianmcgee marked this conversation as resolved
internal/cli/cli.go Outdated
@ -17,2 +16,3 @@
Formatters []string `help:"Specify formatters to apply. Defaults to all formatters."`
TreeRoot string `type:"existingdir" default:"."`
Verbosity int `name:"verbose" short:"v" type:"counter" default:"0" env:"LOG_LEVEL" help:"Set the verbosity of logs e.g. -vv"`
Walk string `enum:"filesystem,git" default:"git" help:"The method used to traverse the files within --tree-root. Currently supports 'git' or 'filesystem'."`
zimbatm commented 2024-01-10 14:28:02 +00:00
Owner

The default could be added to the config file, or detected.

The default could be added to the config file, or detected.
brianmcgee commented 2024-01-10 15:07:22 +00:00
Author
Member

Made some relevant changes in bcc4902ed8

Made some relevant changes in https://git.numtide.com/numtide/treefmt/commit/bcc4902ed849d0742e9be3f49c2ae327a709b375
brianmcgee marked this conversation as resolved
brianmcgee added 1 commit 2024-01-10 15:04:46 +00:00 
Signed-off-by: Brian McGee <brian@bmcgee.ie>
zimbatm approved these changes 2024-01-10 19:05:59 +00:00
zimbatm left a comment
Owner

LGTM

LGTM
internal/walk/git.go
@ -0,0 +23,4 @@
func (g *git) Walk(ctx context.Context, fn filepath.WalkFunc) error {
r, w := io.Pipe()
cmd := exec.Command("git", "-C", g.root, "ls-files")
zimbatm commented 2024-01-10 19:05:12 +00:00
Owner

You didn't want to use the go-git package for this?

You didn't want to use the go-git package for this?
brianmcgee commented 2024-01-11 20:52:10 +00:00
Author
Member

I tried with go-git but couldn't find an efficient way or traversing the worktree. It worked fine on a small repo, but on nixpkgs it broke down badly. Will revisit this again in the future.

I tried with `go-git` but couldn't find an efficient way or traversing the worktree. It worked fine on a small repo, but on nixpkgs it broke down badly. Will revisit this again in the future.
brianmcgee marked this conversation as resolved
brianmcgee merged commit 5711caebb9 into main 2024-01-11 20:52:23 +00:00
brianmcgee deleted branch feat/support-gigignore 2024-01-11 20:52:23 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
zimbatm
Labels
Clear labels   
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: numtide/treefmt#19
No description provided.
Delete Branch "feat/support-gigignore"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?