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

[matts1]

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]

Proposed solution: Set the working directory to the repo root in https://github.com/martinvonz/jj/blob/68b72ad51f5fc2f14cb649adaa428daed3b198e5/cli/src/commands/fix.rs#L391

[martinvonz]

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]

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]

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]

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]

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]

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]

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]

@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]

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