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

FR: Support filename arguments for jj fix #3800

Open
hooper opened this issue Jun 1, 2024 · 4 comments
Open

FR: Support filename arguments for jj fix #3800

hooper opened this issue Jun 1, 2024 · 4 comments

Comments

@hooper
Copy link
Contributor

hooper commented Jun 1, 2024

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:

  • Reluctance to do the same by modifying the jj fix configuration, which might be more work than it's worth for infrequent issues.
  • Working around bad code formatter behavior that affects a specific version of a file in a way that wouldn't justify modifying the jj fix configuration to always avoid the file.
  • Incremental testing of fixes introduced across multiple files.
  • Avoidance of slow tools in situations where the immediate goal is to run other faster tools.

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
Copy link
Contributor

arxanas commented Jun 1, 2024

I meant to open this a while ago: #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
Copy link
Contributor Author

hooper commented Jun 10, 2024

#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:

  • All patterns always match unchanged files:
    jj fix <pattern1> <pattern2> ...
  • All patterns either do or don't match unchanged files:
    jj fix <pattern1> <pattern2> ... --unchanged-files
  • A subset of patterns match unchanged files:
    jj fix <pattern1> --unchanged-files <pattern2> ...
    (maybe the pattern syntax itself could specify this?)

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
Copy link
Member

  • 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
Copy link
Contributor

arxanas commented Jun 15, 2024

I commented in #3808 (comment) 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.)

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

No branches or pull requests

3 participants