martinvonz / jj

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

FR: support running in the working copy for `jj fix` #3808

Open arxanas opened 3 months ago

arxanas commented 3 months ago

Is your feature request related to a problem? Please describe. jj fix(/hg fix) are currently specialized towards formatters. However, these formatters have to operate in-memory on single files, which limits the utility somewhat.

For context, git-branchless-test fix is implemented to operate on working copies, which I find quite useful in practice.

There are several cases where you would prefer the full flexibility of running in the working copy:

Describe the solution you'd like jj fix can operate in the working copy, similar to existing designs for jj run. It would ideally be able to run in the current workspace, or in parallel across multiple workspaces.

Describe alternatives you've considered

hooper commented 3 months ago

I'm partial to the run/fix distinction being equivalent to the per-working-copy/per-file distinction, but mostly due to familiarity with such an implementation for hg. I wonder if there are other granularities we should consider, and how that impacts our UI choices?

The --exec flag feels like an undesirable Git-ism, but I think we agree that a terse way of specifying a command is important in both the per-working-copy model and the per-file model. hg fix is lacking this, although command aliases can bridge some of the gap. It's more natural in the per-working-copy model because less configuration is needed (or maybe some of the info is instead communicated by the subprocess running jj commands).

The --fix flag feels like a bool where we need an enum. If jj run has such an enum, --exec probably doesn't belong in jj fix. Along with a way to determine how much (none, all, a file set, ...) of the specified commit gets updated as a result of a command, the ways of handling its descendants include:

I should read more about git-branchless-test fix. When I find the time, I'll write up my preferred mapping of use cases to CLI syntax.

joyously commented 3 months ago

Do you see either fix or run as related to some sort of bisect? How about blame?

arxanas commented 3 months ago

Do you see either fix or run as related to some sort of bisect? How about blame?

Yes. Here's my most common workflows:

In practice, many (all?) of these benefit from configuring the search behavior (git-branchless-test supports the following behaviors):

The majority of my usage is on my local commit stacks, not historical commits, so I don't have much to say about the relation to blame, except that I am indeed trying to attribute behavior to a certain change.

I'm partial to the run/fix distinction being equivalent to the per-working-copy/per-file distinction, but mostly due to familiarity with such an implementation for hg. I wonder if there are other granularities we should consider, and how that impacts our UI choices?

I don't see why a user would consider fixing via per-file or per-working copy to be relevant for the UI.

The main distinction for me as a user in practice is whether a fix is "slow" or "fast".

Per-file vs per-working-copy fixes are roughly correlated to be "fast" vs "slow", but not always.

arxanas commented 1 month ago

User survey

I surveyed a couple of users in Discord about their mental models.

NOTE: The transcripts below are not verbatim; I removed and reordered messages to try to follow the individual threads.

Question 1 (Discord link):

@arxanas: When you consider a command called jj fix, what is your first impression in terms of what the command would be able to do/what the scope would be?

@tingerrr responses: > @tingerrr: My intuition would be that it fixes something and rebase all descendants > Which is what it does right now > Idk what assumptions I can work off of > @arxanas: When you think of a "fix", is it primarily stuff like formatting? Did you have certain use-cases in mind for things you would like or expect to fix? > @tingerrr: Formatting, maybe some automatic lint fixes, and perhaps some project specific scripts > If I came across jj fix first time > I would think it can materialize a working copy > Which it doesn't > So I think that's the main limitation I would be surprised by > I'm also unsure if it turns for unchanged files at all > Or only for changes files > +- the filesets > @martinvonz: for example, would you expect cargo clippy --fix to work? > @tingerrr: yeah, if i came across it without any prior knowledge > if i saw both jj fix and jj run i would assume that jj fix is just a persistent version of jj run, whereas jj run doesn't rebase descendants or something
@jennings responses: > @arxanas: When you think of a "fix", is it primarily stuff like formatting? Did you have certain use-cases in mind for things you would like or expect to fix? > @martinvonz: for example, would you expect cargo clippy --fix to work? > @jennings: My intuition is tainted because, until last night, I didn't understand why jj fix was created ahead of jj run. So, I've just been thinking of it as "a simpler version of jj run that exists now", and the examples I saw were running formatters. > I'm disappointed that it's either difficult or impossible to make dotnet format work with jj fix, but i don't think of that as a shortcoming of jj fix. I just think of it as an unfortunate design choice for dotnet format. > But, since jj run presumably will be able to handle tools like dotnet format (and cargo clippy --fix), now I'm just waiting patiently for that feature to be released.

Question 2 (Discord link):

@arxanas: Do you expect that modifications dumped into the working copy with jj run will be automatically reflected in the caller (like it'll do the same rewrites as jj fix), or would you have to do something else to indicate that you want the changes to be reflected outside the jj run working copy? (Or, opposite, do you expect that there are cases that jj run changes to the working copy should be discarded/ignored?) Like do you imagine yourself using jj run with commands that don't modify the working copy?

@tingerrr responses: > @tingerrr: i do > if i was bisecting i'd imagine all output would land in ignored directories > Search according to some strategy to find a change in a predicate, either manually by marking commits or automatic using a script, the latter running in a separate materialized working copy > The manual one would be akin to running jj new manually on the bisection commits and abandoning any changes we do there
@jennings responses: > @jennings: I was expecting that jj run would have a flag like --amend-changed-files (or --ignore-changed-files) that would choose whether any changes the tool makes to the working copy are amended into the revision it's running against > But, I didn't have a strong desire for that, it's just what I sort of assumed. I haven't dug deep into the design or discussions of jj run, I only have a general notion of what it will do > I've been assuming that jj run is meant to run commands against a revset, which involves materializing working copies. One use case might be "format all the files", another might be, "execute unit tests against my whole branch to make sure I didn't break anything" > I think of jj fix as a simpler, easier-to-configure specialization of that. > Just like, I think, bisect is planned to be a specialization of jj run > If jj fix didn't exist, I think you could replace it with jj run, it would just be slower. > @arxanas: Is it that you thought it would be easier to configure, or you still think that? Or you think that jj fix should be easier to configure than jj run? > @jennings: I guess by "easier to configure", I mean the normal use of jj fix is: > > ``` > jj fix > ``` > > Because the tools are configured ahead of time and don't really change often. > But I assume the normal use of jj run will be something like: > > ``` > jj run -r main..@ --amend-changes 'cargo clippy --fix' > ```

Question 3 (Discord link):

@arxanas: By default, would you expect jj fix to operate on all files in the working copy, or only files changed in the commit?

@tingerrr responses:

@tingerrr: I would expect it to be all of them

Takeaways

Compared to my original mental model:

jennings commented 1 month ago

Thanks @arxanas for summarizing all that. I obviously haven't thought about this as deeply as y'all have, but if I were building these features, I think this is how I would go about it:

Phase 1: Implement the most general version of jj run for all use cases

I think of jj run as being the Swiss army knife that can do anything, so it would be implemented first with a suite of options:

I think this supports most use cases, but is probably slow because it runs sequentially and materializes everything.

Example:

# Run an ad hoc command
$ jj run --amend-changes --exec 'dotnet format'

# Run a pre-configured command
$ echo '
[run.tools.prettier]
command = ["prettier"]
amend-changes = true
' > ~/.jjconfig.toml
$ jj run --tool prettier

# Run unit tests, display failures somehow on non-zero exit code
$ jj run --exec 'npm test'

Phase 2: Optimize by allowing parallel execution

Since some projects/tools don't care about their absolute path, add an option jj run --parallel that creates temporary working copies and executes the tool there. There could be options for how many to run in parallel.

$ jj run --tool prettier --parallel

# configuration: `run.tools.prettier.parallel = true`

Phase 3: Optimize by not materializing working copies

Some tools won't need a working copy, so it would be nice to avoid that to make everything faster. Since this is so different from the other execution mode, I am indifferent between:

  1. Add an option jj run --mode file-filter (and run.tools.NAME.mode = "file-filter") to execute the tool like a Unix filter, or
  2. Have a separate command jj fix which does this

If jj went with --mode file-filter, I would probably just create an alias named "fix" that does that.

joyously commented 1 month ago

My first impression of jj fix is that it seems weird and the only thing to fix is some sort of meta data integration with a bug ticket system, marking a commit as a bug fix.

I don't expect that jj run would make any modifications to the working copy since it would be "running" on a revset, not the working copy. I suppose the actual command that is run could make modifications though. Perhaps a flag should be used to indicate what to snapshot. I assume the working copy is untouched because I expect that I could run simultaneous jj run commands in different windows.

I don't think jj fix makes any sense, but I would expect that it would take parameters for what to work on. (It seems that running formatting tools or whatever you are fixing is the responsibility of other tools, not version control.)

hooper commented 1 month ago

To be clear, the main reason jj fix exists right now is because it is immediately valuable to Google, even in its nascent form. That jj run isn't fleshed out yet is partly because we're discussing it, rather than letting Google (me) throw more specialized code over the fence, which I think is a good thing. We should not consider any of the work done to date on jj fix to be a constraint; Google will be fine as long as our users can run stdin/stdout formatters on changed files (and, preferably, changed line/byte ranges) "quickly" by typing jj fix, regardless of the implementation. I think we can reach a similar outcome for Google's use cases for its internal hg run (which is interesting in its own right, but mostly for the proprietary integrations, and not so much for its tremendous insight into the problems/opportunities we're facing with jj run).

Fortunately, I think we have a lot of convergent thinking on many aspects. I keep seeing hypothetical statements that sound like what I've been thinking.

I've attempted to arrange the many desired behaviors into a small-ish set of orthogonal-ish dimensions, in the spirit of making jj run the "swiss army knife" of this feature cluster. Hopefully each dimension serves to map directly between a UI widget (like a single flag or GUI element) and a corresponding aspect of the implementation. This should allow for Google's jj fix to be expressed as an alias for a customized jj run invocation, making the layers of abstraction clearer, and keeping the unfortunate string fix as far away from out-of-the-box JJ as possible.

I also hope that this provides a framework for phased implementation, where we implement the default behavior of each flag first, and fill in combinations over time.

Dimensions with some hypothetical enumerations:

I'm not sure what the flags for each dimension should look like exactly. We can either make exclusivity obvious with --thing=foo|bar, or make it sugary with --foo-thing|--bar-thing. I'm not sure if we have a strong precedent for this in the CLI. In fact, that may even be a reason to not design it this way.

I have some thoughts on how this would map to implementation, but I wanted to post this before I spend too much time on that. I think it could make good use of a planning phase that considers the flags and generically feeds instructions into a queue that is consumed by a worker pool that doesn't concern itself with flags. Instructions would include some arrangement of things like materializing working copies, executing subprocesses in working copies, passing file contents through subprocesses, rewriting commits with a FileId->FileId map, etc. with some mechanism for data dependencies between them.

As an aside, this is a complicated feature. I think it is worthwhile in its entirety, but we should honestly consider if it's not. If we scope it down, we cede more territory to third party extensions, and allow the functionality to develop in a more fragmented way (for better or worse). Maybe the incoming governance structure can weigh in on that.

joyously commented 1 month ago

If existing tools have run or fix, are their designs robust enough to use for jj, or do you need to slog through an entire design process for this? Can you use the best of what already works?

arxanas commented 4 weeks ago

@joyously posted responses to the questions here.

@emilazy posted responses to the questions in the Discord, reproduced below:

Question 1

@arxanas: When you consider a command called jj fix, what is your first impression in terms of what the command would be able to do/what the scope would be?

@emilazy responses: > @emilazy: honestly, not what it actually does, although I don't know what name I'd expect for what it actually does. it puts me in mind of things like cargo fix, which is certainly in the same ballpark although of course ironically doesn't work with jj fix. the idea that it would do formatting doesn't really cross my mind if I try to imagine not already knowing what it's for. > > I almost expect it to be something VCS-internal – like something to get out of conflicted states or something. > >> @arxanas: When you think of a "fix", is it primarily stuff like formatting? Did you have certain use-cases in mind for things you would like or expect to fix? > > @emilazy: if I interpret this as "when I think of 'the kind of thing I might use a command like jj fix for', knowing what it does", formatting and things that might as well be formatting (e.g. cargo fix type things like automated removal of unused variables). I also can imagine wanting to do things like run a quick sed script over the commit contents to do a quick and dirty replacement of something. > >> @arxanas: (I'm trying not to lead with my preferred answers but this is regarding that I don't think jj fix's current limitations are intuitive, and would like to hear what real users think) > > @emilazy: I completely agree. I think that if jj fix didn't exist and I knew that jj run let you do things like run test suites over commits and bisect, I would reach for something like jj run --edit to do reformatting and the other tasks I listed. (I don't know if jj run is my favourite name for what it is either, but I don't really have a better suggestion; it seems basically fine.) > > I also wouldn't expect a command intended for running formatters to completely ignore the formatter configuration you have in the repository! > > I understand why you'd want jj fix's limitations but they feel firmly in implementation detail territory to me, something that you might pass some arcane --no-materialize flag to or something. and also, like, I dunno, do we want that kind of restriction to be a first-class part of the interface or should you just use a VFS if materializing the commits is that much of a bottleneck? > >> @jennings: My intuition is tainted because, until last night, I didn't understand why jj fix was created ahead of jj run. So, I've just been thinking of it as "a simpler version of jj run that exists now", and the examples I saw were running formatters. > > I knew the inside baseball about materializing vs. not, but I also admit that I've developed a mindset of "it's jj run except not useful for half the things I'd want". > >> @jennings: I'm also unsure if it turns for unchanged files at all > > yes, there's also this. for instance jj run --edit cargo fmt would not be the same as jj fix to reformat! and, hm, it's complicated, because sometimes I would want the jj fix format of only touching files I modified, but
 eh. it's confusing. I think that reformatting the entire tree is usually the cleaner model because you want CI to enforce that property anyway so the command is just a "clean this stuff up before it goes upstream" run. (semi-relatedly: I bet I might end up with some kind of jj fix && jj run tests && jj desc --sign-off && jj sign "pre-push" alias at some point.) > (I mentioned this a while back, but Nixpkgs is in the middle of transitioning to auto-formatting and we currently have the rule of "any new files, and any files that were correctly formatted before the PR, must be correctly formatted after the PR". that feels difficult to encode, because it cares about both @ and @-, but food for thought if you want to make something even more flexible and complicated
) > anyway I like jj run --edit. jj run --edit cargo fmt, jj run --edit cargo clippy --fix, jj run --edit fd -e nix -x sed -i '' -e 's/bad/good/g' {}, it just makes sense to me. Nixpkgs isn't exactly fast to materialize but the mental model is simple and I have to deal with checkout performance every time I jj new anyway, so I'd rather address that in a holistic manner. (I wonder if there is any use for something like jj run --bisect --edit.) > > > @jennings: I also had no intuition for what jj fix would do before reading the docs because I don’t think of “fix” as being something a VCS is concerned with. To me, a VCS is pretty indifferent about what kinds of text files it’s tracking, so “fixing” them isn’t something I think a VCS is responsible for. > > jj run makes sense because “run a command across many revisions” is something that a VCS should plausibly help you with. > > jj fix is very useful and wouldn’t want the functionally removed, but maybe that’s why I think if it as a specialization of jj run > > @PhilipMetzger: jj fix takes its name from the hg extension, whereas Git only has rebase --exec. And it hopefully will be implemented as a specialization of run when we get there

Question 2

@arxanas: Do you expect that modifications dumped into the working copy with jj run will be automatically reflected in the caller (like it'll do the same rewrites as jj fix), or would you have to do something else to indicate that you want the changes to be reflected outside the jj run working copy? (Or, opposite, do you expect that there are cases that jj run changes to the working copy should be discarded/ignored?) Like do you imagine yourself using jj run with commands that don't modify the working copy?

@emilazy responses: > @PhilipMetzger: what you and steve call edit/--amend-changes will be the default for run > > @emilazy: hm, I don't know if I love that, but I guess most of the time a command wouldn't touch non-ignored files anyway > I do think that jj fix would be better spelled jj run --no-materialize-one-file-only-stdin or whatever. obvious not a serious proposal for the flag, but I do feel like it's an optimization rather than something that warrants a separate top-level command. (I also think that jj run should be parallel by default fwiw, you can always -j1) > > @PhilipMetzger: out of interest, would you be interrsted in a jj format just for formatters? > > @emilazy: I would expect jj format to use some per-repo configuration or something to automatically jj run a formatter, I guess I'd certainly use it (if it supported rustfmt.toml files
) > > @PhilipMetzger: I meant as a separate command, since people already mix run/fix so much > > @emilazy: that's an implementation detail to me though. interface-wise, jj fix feels like "something you can use instead of jj run that's faster if you can be happy with a laundry list of constraints" > > @PhilipMetzger: yes, thats a fair assesment > but unless you really need the full context it is powerful enough > > @emilazy: I think you'd be surprised how much tooling is unhappy with jj fix's restrictions, like in the Nix world we have https://treefmt.com/formatter-spec which does materialization and there's still headaches with stuff that isn't happy with those restrictions > >> @emilazy: hm, I don't know if I love that, but I guess most of the time a command wouldn't touch non-ignored files anyway > > @arxanas: I think 3/3 surveyed users don't expect jj run's modifications to affect the caller > > @emilazy: I expect to have to say jj run --edit, but OTOH, what cases would you get modifications to non-ignored files and want them discarded? > like my first thought is "well I don't want my build output to modify the commits", but obviously that's an issue in general if you don't ignore those generated files > and jj run sed 
 being a nop might be confusing > > @arxanas: > - I think weird build artifacts would be the main thing (but yeah you need to ignore them). > - You could also imagine doing an ad-hoc script/command that writes a file to the working copy to use later in the pipeline, which you wouldn't want to actually amend the commit with, but also wouldn't want to bother explicitly ignoring. > -If you travel further along the extensions to "run" functionality, then you can imagine a bisect command wanting to modify the working copy to prepare for a test predicate, but you definitely wouldn't want to modify the historical commits as part of the investigation. The commit rewriting behavior would mostly be a matter of consistency (and whether we think it's important for run vs bisect to be consistent in this way, depending on how users consider them to be related). > > Re no-op, git-branchless will always note down the changes to the working copy when you run git test run and report "fixable" if it changed. Since it also caches run results by tree+command by default, it means you can see that and follow up with git test fix to instantly apply the cached fixes without having to rerun the command. > Actually, one real use case is that cargo commands sometimes modify Cargo.lock, and I absolutely don't want to implicitly update Cargo.lock in those cases (if I wasn't specifically running cargo update). It sometimes happens with git-branchless (and I see "fixable" and ignore it). > >It also happens with git-branchless because it supports a mode where it uses the current working copy for git test run (via sequentially switching to each commit), and random IDE integrations can modify the working copy (rust-analyzer with Cargo.lock 😬), even if I'm not making any changes myself in the editor. Running in the current workspace would be a good feature for jj to have (for cases where the repo is too big to casually make extra workspaces), but I don't think it's on the immediate roadmap. > >> @arxanas: you can imagine a bisect command wanting to modify the working copy to prepare for a test predicate, but you definitely wouldn't want to modify the historical commits as part of the investigation > > @emilazy: oh, this is a very good point! > my hope was that there would be no jj bisect, just jj run --bisect (ok, an alias would be fine) > (also maybe --bisect could be replaced by a better name since it's not quite "bisection", but it's common terminology so eh) > I'm going to be thinking in the back of my mind if there's any possible use case for --bisect --edit though :) > >> @arxanas: Actually, one real use case is that cargo commands sometimes modify Cargo.lock, and I absolutely don't want to implicitly update Cargo.lock in those cases (if I wasn't specifically running cargo update). It sometimes happens with git-branchless (and I see "fixable" and ignore it). > > @emilazy: oh, +100. lock files is a really good point. I now firmly and strongly believe that --edit should not be default. also, this is a problem for --edit in general, but if it's the default it becomes more pertinent – do the edits apply in sequence? i.e. can the run for commit A++ see the changes made to commit A? that's incompatible with parallelism, for instance.
hooper commented 4 weeks ago

That last point about "seeing changes made in previous commits" is important. It could either be considered as another dimension, or part of the "scope" dimension from my last comment. jj fix is basically "not seeing previous changes" under the assumption that it doesn't matter for a category of tools including formatters. You could also consider jj fix a hybrid because of the diffing used to target changed files and avoid rebasing. I think we would want to be careful to avoid n^2 auto-rebasing behavior in the "see previous changes" mode (this is one way where jj run can be more useful than the equivalent shell loop, and could be useful to document in jj help run).

A couple of things seem clearer now:

I also generally agree with the idea that --edit by default is too risky to be net useful, even assuming it plays well with jj undo.

I don't think anyone contradicted this, but I want to point out that --edit only implies serializing parents and children; we could still parallelize branches, even if they merge.

Another rabbit hole is that --edit implies "topological execution order", but later we may also want to consider some notion of predicting the runtime for each revision, so we can optimize that order for a shorter critical path. That's complicated, because it interacts with external things like build system caches, but it will come up eventually. I wonder if we can get ahead of that kind of thing by implementing a more general bin packing for resources like RAM, cores, and wall time?