Skip to content

fix: normalize stdin path to be relative to tree root #682

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.

Sign up for GitHub

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

Open
adrian-gierakowski wants to merge 1 commit into
base: main
Choose a base branch
from
+53 −3
Open

fix: normalize stdin path to be relative to tree root #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions cmd/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2117,6 +2117,42 @@ func TestStdin(t *testing.T) {
}),
)

// verify that absolute paths work
absPath, err := filepath.Abs("test.nix")
as.NoError(err)
os.Stdin = test.TempFile(t, "", "stdin", &contents)

treefmt(t,
withArgs("--stdin", absPath),
withNoError(t),
withStats(t, map[stats.Type]int{
stats.Traversed: 1,
stats.Matched: 1,
stats.Formatted: 1,
stats.Changed: 1,
}),
withStdout(func(out []byte) {
as.Equal(`{ ...}: "hello"
`, string(out))
}),
)

// verify that absolute paths in subdirectories work with glob patterns
absPathInSubdir, err := filepath.Abs(filepath.Join("go", "main.go"))
as.NoError(err)
goContents := "package main\n"
os.Stdin = test.TempFile(t, "", "stdin", &goContents)

treefmt(t,
withArgs("--stdin", absPathInSubdir),
withNoError(t),
withStats(t, map[stats.Type]int{
stats.Traversed: 1,
stats.Matched: 1,
stats.Formatted: 1,
}),
)

// the nix formatters should have reduced the example to the following

// try a file that's outside of the project root
Expand Down
20 changes: 17 additions & 3 deletions walk/walk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,26 @@ func NewCompositeReader(
}

path := paths[0]
resolvedPath, err := resolvePath(path)
if err != nil {
// If the path doesn't exist, we still want to make it relative to the root if possible.
Copy link
Copy Markdown
Collaborator

@jfly jfly Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@adrian-gierakowski adrian-gierakowski Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@adrian-gierakowski adrian-gierakowski Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

@brianmcgee brianmcgee Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@adrian-gierakowski adrian-gierakowski Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@adrian-gierakowski adrian-gierakowski Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@jfly jfly Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@jfly jfly Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@adrian-gierakowski adrian-gierakowski Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Collaborator

@jfly jfly May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// We use the absolute path without resolving symlinks.
resolvedPath, err = filepath.Abs(path)
if err != nil {
return nil, fmt.Errorf("error computing absolute path of %s: %w", path, err)
}
}

relativePath, err := filepath.Rel(root, resolvedPath)
if err != nil {
return nil, fmt.Errorf("error computing relative path from %s to %s: %w", root, resolvedPath, err)
}

if strings.HasPrefix(path, "..") {
return nil, fmt.Errorf("path %s not inside the tree root %s", path, root)
if strings.HasPrefix(relativePath, "..") {
return nil, fmt.Errorf("path %s not inside the tree root %s (relative path: %s)", path, root, relativePath)
}

return NewStdinReader(root, path, statz), nil
return NewStdinReader(root, relativePath, statz), nil
}

// create a reader for each provided path
Expand Down