Improve git walking

[brianmcgee]

Replaces the use of go-git with a simple git ls-file for traversing the git index.

TODO

• rebase after feat: match before checking cache #447 is merged

[brianmcgee]

@jfly wouldn't mind your eye on this too 🙏

[jfly]

This is a neat idea! It'll be cool to see it in action.

I tried to give you a thorough review, but I'm a total beginner with go, and don't really have treefmt's architecture in my head.

That said, I think I understand enough of this PR to know that either something is missing, or I'm even more confused than I realize. Perhaps I'm just misunderstanding what this feature is meant to do.

I would have expected that if I ask treefmt to format my staged files, it would do some git acrobatics to only format the staged changes of those staged files. (You'll see this expectation reflected in at least one of my inline code comments). Now that I've read the whole diff, that doesn't seem to be the case: this really only affects which files we walk over, it doesn't do anything clever to separate the staged vs unstaged changes. Doing this right is tricky and is why writing a pre-commit hook correctly is so damn annoying.

If I understand correctly, this also doesn't seem to implement the thing asked for in #311. But maybe the goal changed since June? I'm also a little skeptical about the thing being described in #311. I'll leave a comment over there about that.

[brianmcgee]

If I understand correctly, this also doesn't seem to implement the thing asked for in #311. But maybe the goal changed since June? I'm also a little skeptical about the thing being described in #311. I'll leave a comment over there about that.

I tagged that issue as I thought I was solving the same problem. But you're right; I seem to have drifted.

[brianmcgee]

On reflection and continuing the discussion in #311, I will simplify this PR to just replacing go-git with git ls-files.

[jfly]

I will simplify this PR to just replacing go-git with git ls-files

Just curious, why are we dropping go-git? Coincidently, I had a conversation with @traxys a few weeks ago about an issue he was running into with some experimental git feature (core.fsmonitor, I think?) that (maybe?) isn't by go-git. I suspect that switching to git ls-files would have made that problem go away.

[brianmcgee]

Just curious, why are we dropping go-git?

Originally, this was motivated by the status in go-git being very slow on large repos, so when I was trying to implement the git_staged walk type, it was a non-starter as Nixpkgs is my reference. From there, if I was going to just shell out to git anyway, it felt like overkill to use go-git.

Having seen how clean and simple it looks just shelling out versus using go-git (which I could have optimized a bit more I guess), I decided to remove an extra dependency and keep things simple. We did get burned previously by a bug in go-git nix-community/infra#1266 (comment).

TLDR; less dependencies, less indirection, less code 🙂

[brianmcgee]

I'll rebase this once #447 is merged and then mark it ready for review.