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

feat: Allow recursing into submodules when using git walker #507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akshaymankar
Copy link
Contributor

I haven't added tests, but I'll add if this feature is deemed desirable.

@brianmcgee
Copy link
Member

brianmcgee commented Jan 7, 2025

Thanks for the PR!

I'm wondering if it might be better to allow passing through git args? 🤔

It would make it more general, and possibly help users to further customise their git behaviour in future.

@jfly @zimbatm thoughts?

@jfly
Copy link
Collaborator

jfly commented Jan 17, 2025

I'm wondering if it might be better to allow passing through git args?

Coupling ourselves to the git cli smells wrong to me. We used to use a pure-go implementation of git, right? Wouldn't it be hard to switch back to that in the future if we supported arbitrary git cli args?

@brianmcgee
Copy link
Member

I don't see us moving back to a git alternative; there is too much risk of feature drift/bugs like we experienced already.

@jfly
Copy link
Collaborator

jfly commented Jan 17, 2025

That's fair. I personally am not anxious about being coupled to the git cli, either, but I do think it's worth acknowledging that it's different than just being coupled with git.

More concretely, what's your idea? Is it having the ability to pass arbitrary args to git ls-files?

@brianmcgee
Copy link
Member

More concretely, what's your idea? Is it having the ability to pass arbitrary args to git ls-files?

Yeah. Straight pass through I guess.

@jfly
Copy link
Collaborator

jfly commented Jan 17, 2025

Looking through git ls-files --help, I see that some of these flags affect the formatting of the output. For example, see -z:

-z
    \0 line termination on output and do not quote filenames. See OUTPUT below for more
    information.

I imagine this could break our parsing of the output of git ls-files, and result in some pretty weird behavior.

Are there any existing options/flags that treefmt has that wouldn't need to exist if we had this generic flags feature? Or on the flip side, do you see any useful flags people might want to use (besides the --recurse-submodules in this PR)?

@akshaymankar
Copy link
Contributor Author

Makes me wonder if it is better to just create a separate walker which takes a command to get the list of files.

@jfly
Copy link
Collaborator

jfly commented Jan 18, 2025

Oooh that's a neat idea, as it would allow people to use non-git vcs tools.

@brianmcgee
Copy link
Member

I like this idea as well

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.

3 participants