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

jj fix uses incorrect paths when executed from a non-root directory. #4866

Open
matts1 opened this issue Nov 15, 2024 · 10 comments · May be fixed by #4867
Open

jj fix uses incorrect paths when executed from a non-root directory. #4866

matts1 opened this issue Nov 15, 2024 · 10 comments · May be fixed by #4867
Assignees

Comments

@matts1
Copy link
Collaborator

matts1 commented Nov 15, 2024

Description

Steps to Reproduce the Problem

Add a tool to your config

[fix.tools.gofmt]
command = ["gofmt", "$path"]
patterns = ["glob:'**/*.go'", "glob:'**/go.mod'"]
$ cd <workspace root>/subdirectory
subdirectory$ jj fix

Expected Behavior

It should be able to fix it

Actual Behavior

stat subdirectory/foo.go: no such file or directory

Specifications

  • Platform:
  • Version: 0.23
@matts1 matts1 self-assigned this Nov 15, 2024
@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

Proposed solution: Set the working directory to the repo root in

vars.insert("path", tool_input.repo_path.as_internal_file_string());

@martinvonz
Copy link
Owner

I think this is working as intended. The formatter is supposed to take the input on stdin and produce the output on stdout. The $path is just metadata that can be used by the formatter.

@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

Even if it is just metadata, the metadata is incorrect. If I'm in /path/to/repo/foo and I see foo/bar.go, that actually means /path/to/repo/foo/foo/bar.go, while it was supposed to be /path/to/repo/foo/bar.go.

@martinvonz
Copy link
Owner

We use the full path at Google. Our formatter is a multiplexer. It picks the formatter to delegate to depending on $path.

What do you want to use the relative path for?

@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

Ah, I get what you're saying now. My concern is that it's very easy to do what I accidentally did and just write the tool as ["gofmt", "$path"], when the correct tool should be ["gofmt"]. This introduces issues with jj fix -r <other revision>, when the file content doesn't match what needs to be formatted.

I haven't tried this, but I imagine that if I had main -> A -> B (@), which all modified foo.go, then I ran jj fix, then it would attempt to format B's foo.go and write the formatted version into A, resulting in what is essentially an unsquash?

How would you feel about explicitly creating an empty temporary directory to run the fixes in so that the relative paths don't work, and any attempt to do so would fail gracefully, rather than spectacularly blowing up your commits?

@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

I haven't tried this, but I imagine that if I had main -> A -> B (@), which all modified foo.go, then I ran jj fix, then it would attempt to format B's foo.go and write the formatted version into A, resulting in what is essentially an unsquash?

I just experimented and confirmed that this is exactly what happens

@joyously
Copy link

How would you feel about explicitly creating an empty temporary directory to run the fixes in

Don't you need this anyway, because of concurrency?

@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

How would you feel about explicitly creating an empty temporary directory to run the fixes in

Don't you need this anyway, because of concurrency?

You shouldn't need it for a well-behaved fix command, since a well-behaved one only reads from stdin and writes to stdout, which leaves jj to deal with any concurrency issues itself (which it does).

matts1 added a commit to matts1/jj that referenced this issue Nov 15, 2024
@matts1 matts1 linked a pull request Nov 15, 2024 that will close this issue
4 tasks
matts1 added a commit to matts1/jj that referenced this issue Nov 15, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 15, 2024
@matts1
Copy link
Collaborator Author

matts1 commented Nov 15, 2024

@yuja raised a very good point on my PR, pulling it into the discussion here.

Doesn't it break formatter configuration? Ideally config files should be loaded from the project root.

This is a very good point which I hadn't considered before, but it also means that it's currently broken under a few specific circumstances.

  • Since it runs in your current directory, jj --repository /path/to/repo fix from outside the repo won't load the configuration.
  • If you have config in /path/to/repo/.clang-format and some overrides in /path/to/repo/subdir/.clang-format, the overrides will get loaded based on whether you're currently in the subdirectory, not based on whether you're formatting a file in said subdirectory.

@martinvonz
Copy link
Owner

Good points. Sounds like we should set the current directory to the workspace root when running the formatters then.

matts1 added a commit to matts1/jj that referenced this issue Nov 19, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 19, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 19, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 20, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 20, 2024
matts1 added a commit to matts1/jj that referenced this issue Nov 21, 2024
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 a pull request may close this issue.

3 participants