martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
8.25k stars 277 forks source link

FR: Support filename arguments for `jj fix` #3800

Open hooper opened 3 months ago

hooper commented 3 months ago

Problem:

Users of jj fix are likely to sometimes want to only fix subsets of changed files, or to fix specific unchanged files.

Solution:

Use positional arguments to create a matcher for use when diffing commits to choose which paths to fix: jj fix -s <rev> <file...>

Alternatives:

Tweaking the jj fix configuration on a per-command basis seems too cumbersome, although this depends on how frequently a particular project would want to format a subset of files, and on the complexity of its jj fix configuration.

Context:

Possible motivations for using this feature include:

One debatable aspect is whether users should be able to fix unchanged files by listing their names in the command. hg fix does this by default. We may want to require an additional flag to enable this behavior: jj fix -s <rev> <file> --fix-unchanged-files. An example use case is generating a commit that updates the formatting of some files to agree with the current version of a formatter, so that descendant commits won't become cluttered with those formatting-only changes.

An open question is how exactly to use copy/move information. Is it reasonable to assume that the user wants to format the file in revisions where it has moved to a new path? Is it reasonable to format multiple copies of a file when the user only specified one path?

The specific command hg fix . has proven to be a mild foot gun at Google, because it looks a lot like hg fix -r .. Fortunately, jj fix shouldn't have that issue, due to the revset syntax using @ instead of .. So, we can probably allow jj fix . to fix everything in the directory, as long as it has some output or a progress bar to make it obvious when the user has accidentally asked it to format an impractically large number of files (which is less of a problem for smaller repos).

arxanas commented 3 months ago

I meant to open this a while ago: https://github.com/martinvonz/jj/issues/3809, which would also address the hg fix . footgun. Even if . doesn't mean the current commit in jj, by accepting positional path arguments, it's still possible that users might pass positional revset arguments instead, which would confuse them when it does nothing.

hooper commented 3 months ago

https://github.com/martinvonz/jj/pull/3857 addressed the issue of fixing a subset of changed files. There's still some room to determine exactly how fixing unchanged files should work. Some possibilities:

I think we probably want this to compose with commands like jj new --insert-before foo rather than adding anything like jj fix --insert-formatting-commit.

martinvonz commented 3 months ago
  • jj fix <pattern1> <pattern2> ... --unchanged-files

I think that's what I'd vote for. It seems rare to need the more generic version, and it seems it should be possible to achieve by two calls instead (jj fix <pattern1> && jj fix <pattern2> --unchanged-files).

arxanas commented 3 months ago

I commented in https://github.com/martinvonz/jj/issues/3808#issuecomment-2170747160 to 1) propose that separating per-file and per-working-copy fixes for jj fix doesn't make sense from a UI perspective and 2) give some examples of useful working-copy fixes that I run regularly.

I'm not necessarily opposed to configuring the passing of filenames to jj fix, but I would like to see it not overly limited or tailored to the case of per-file fixes. Working on this FR would probably require agreement on the scope of jj fix to decide how to design the interface.

One debatable aspect is whether users should be able to fix unchanged files by listing their names in the command. hg fix does this by default. We may want to require an additional flag to enable this behavior: jj fix -s --fix-unchanged-files. An example use case is generating a commit that updates the formatting of some files to agree with the current version of a formatter, so that descendant commits won't become cluttered with those formatting-only changes.

In this case, it seems appropriate to not rely on making jj fix responsible for this? It's just a general situation where you want to make changes to the files in the working copy and commit them. The problem seems common in large organizations where other people have configured the tools for you, and now you don't know how to invoke them manually even if you wanted to, but I'm not convinced that making jj fix a general-purpose interface to "tools that modify source code" is the best solution, precisely because the interface may become too broad.

In my experience doing codebase-wide formats and refactorings, I often want to check the result in a non-automated way afterwards, to make sure that the scope wasn't unexpectedly broad or narrow, or there weren't any obvious egregious issues. In those cases, it doesn't benefit me to put it in a subcommand that's meant for automated fixes, except in that it was possibly easier to find a good command invocation. (It might not be anyways; it's probably much easier for me to run my-format-tool <dir> than figure out exactly what flags I need to pass to jj fix to force it to format those files even though it wouldn't normally.)