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

fix: ignoring symlinks #528

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

fix: ignoring symlinks #528

wants to merge 10 commits into from

Conversation

gabyx
Copy link

@gabyx gabyx commented Mar 3, 2025

@brianmcgee : I tried the following:

  • Ignore symlinks on the Git reader.
  • Adding a symlink to the examples, to test if it gets ignored everywhere this examples are used.
  • Add resolving symlinks on the Run functions to pass the resolved paths to the readers, such that the readers Filesystem or Git can ignore symlinks always. treefmt symlink.yaml or treefmt /a-symlink/b/c/d.yaml should be allowed.

See some comments I need help on which is unclear to me.

Fixes: #495

@gabyx
Copy link
Author

gabyx commented Mar 3, 2025

@brianmcgee : I have question to the test here:

Why do we even test to run in the .git folder? You will never want that IMO, or is this really a feature?
Shouldn't treefmt ignore any .git directory (got from git rev-parse --git-common-dir in the tree root)

// walk with filesystem instead of with git

@gabyx gabyx changed the title fix: first tests of ignoring symlinks fix: ignoring symlinks Mar 3, 2025
@gabyx gabyx force-pushed the feat/fix-symlinkgs branch from 5555359 to 20fea9d Compare March 3, 2025 11:20
@brianmcgee
Copy link
Member

Why do we even test to run in the .git folder? You will never want that IMO, or is this really a feature? Shouldn't treefmt ignore any .git directory (got from git rev-parse --git-common-dir in the tree root)

In that particular test, it's checking that if we momentarily switch to the filesystem walker, the .git directory gets picked up and traversed. It's not that we want to traverse the .git directory per se, but that it should traverse any directory when the filesystem walker is enabled.

@gabyx
Copy link
Author

gabyx commented Mar 4, 2025

Why do we even test to run in the .git folder? You will never want that IMO, or is this really a feature? Shouldn't treefmt ignore any .git directory (got from git rev-parse --git-common-dir in the tree root)

In that particular test, it's checking that if we momentarily switch to the filesystem walker, the .git directory gets picked up and traversed. It's not that we want to traverse the .git directory per se, but that it should traverse any directory when the filesystem walker is enabled.

Ah ok.

I need to still refactor the symlink resolving code in the Run function, its not good yet. I think the only correct way is to us EvalSymlinks/ or only os.Stat and check for errors and make the test pass. As this is only done on path inputs it should not degrade performance. ki

@gabyx
Copy link
Author

gabyx commented Mar 5, 2025

@brianmcgee: The thins is done. Happy for a review what you think about the behavior. Hope its ok like that.

@brianmcgee brianmcgee self-assigned this Mar 5, 2025
@brianmcgee brianmcgee self-requested a review March 5, 2025 17:18
@brianmcgee
Copy link
Member

Thanks for this, I'll try and review it in the next few days.

@brianmcgee
Copy link
Member

@gabyx please rebase on latest main

@gabyx gabyx force-pushed the feat/fix-symlinkgs branch from 237342a to dfeda94 Compare March 12, 2025 16:21
@gabyx
Copy link
Author

gabyx commented Mar 12, 2025

@brianmcgee: Did the changes and rebased.

@brianmcgee
Copy link
Member

Thanks 🙏 I'll give it another review in the next day or so

@brianmcgee brianmcgee self-requested a review March 12, 2025 16:33
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.

Don't generate warnings on missing formatters for symlinks to directories
2 participants