fix: normalize stdin path to be relative to tree root

[adrian-gierakowski]

When using --stdin with an absolute path, the RelPath was incorrectly
set to the absolute path, causing pattern matching for includes and
excludes to fail against project-relative globs.

This change ensures that the path provided with --stdin is converted
to a relative path from the tree root before processing.

[jfly]

Thanks for the contribution!

[jfly]

This smells weird to me. Could we just make do without trying to call resolvePath at all?

[adrian-gierakowski]

I use resolvePath to match existing behaviour when used without --stdin. IMHO the behaviour should be the same between these 2 cases when main.go is a symlink to another file which matches a different formatter than the path of the symlink iself:

treefmt ./main.go
cat main.go | treefmt --stdin ./main.go

the first command (before and after this PR) follows the symlink and dispatches to formatter based on the path the symlink is pointing at

if we don't use resolvePath here, the second command would format based on the location of the symlink, not the file it is pointing at

The fallback is in case the path provided to --stdin is simply a hint, and does not point at an existing file.

[adrian-gierakowski]

here's an implementation without the symlink resolution in case you want to try it for yourself:

adrian-gierakowski@9f74345

NOTE: I based this PR on last commit for which CI was green (CI failure here must be due to main being broken and github merging main into PR branch before running CI)

[brianmcgee]

I like the non symlink resolving version better. Unless there's something I'm missing, I'd like to merge that version if you can create the PR and we can close this one.

[adrian-gierakowski]

Can I also remove the symlink resolution when formatting a file given it's path? Otherwise we'll have inconsistent behaviour.

[adrian-gierakowski]

I would have thought that treefmt ignores symlinks.

Not does not

Is the issue about files that are contained in folders that are symlinks? If we fully resolve the path or not, we'll potentially apply different rules to the file?

Please see my explanation in above

My preference would be for treefmt to completely ignore symlinks both for walking the filesystem and when deciding to format a file. Anything else involves either wasted double formatting or problematic inconsistent double formatting. I don't know how far that is from our current reality. I know we've had repeated regressions when the project root is itself a symlink to somewhere. There also may be some subtlety around invoking treefmt from inside a symlinked folder.

I think you make some good points. Maybe we should reach out to whomever imolemented the symlink resolution logic in the first place to understand the motivation a bit better? What I care is content behaviour file path and stdin formatting.

[jfly]

Maybe we should reach out to whomever imolemented the symlink resolution logic in the first place to understand the motivation a bit better?

Sure, or if it were me, I'd start by just trying to implement what I described, and see if any tests fail and take it from there.

if we don't use resolvePath here, the second command [cat main.go | treefmt --stdin ./main.go] would format based on the location of the symlink, not the file it is pointing at

I prefer the "format based on the location of the symlink" behavior, which I believe is consistent with my "let's ignore symlinks" proposal.

[jfly]

Also, I do think we can land the non-symlink resolving version of this PR before revisiting treefmt's overall handling of symlinks.

[adrian-gierakowski]

I'd be happy to remove symlink resolving altogether. But hesitant to put any time into this unless an authoritative decision is made that it would be merged. In the meantime I'm using my fork since I really need stdin to work (and without surprises)

[jfly]

Let's get rid of it. If the tests pass, then I see no reason to keep any of it around. @brianmcgee?

[brianmcgee]

main is now green. I'll have some brain space to weigh in on this later in the week.