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 running in the working copy for jj fix #3808

Open
arxanas opened this issue Jun 1, 2024 · 11 comments
Open

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

arxanas opened this issue Jun 1, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@arxanas
Copy link
Contributor

arxanas commented Jun 1, 2024

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:

  • To set up formatters in a much more simple way:
    • Like jj fix --exec 'cargo fmt' without any complicated configuration.
    • This scales to even moderate-size repos without much thought.
  • To support tools that can't be feasibly adjusted to work in-memory.
    • Like my work's linter setup 🙃
    • Like if you wanted to run your existing pre-commit checks.
  • To run whole-repo analyses, such as lints or builds:
    • Like jj fix --exec 'cargo clippy --fix'.
  • To run tools which would operate on the commit history, such as to create new commits, modify existing commits, etc.
    • This is how git-branchless-submit works with Phabricator: it runs arc diff and actually amends the commit message with the effective change ID (the Phabricator URL).
    • Gerrit integration could work the same (to add change IDs to the commit messages).
    • That's an advanced feature with several design considerations which is probably not important to get in the initial implementation of operating on 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

  • Add a --fix flag to jj run instead. That's kind of weird because "fixes" are more or less the same thing to the user, so there's no reason to arbitrarily split them up into two different subcommands.
@hooper
Copy link
Contributor

hooper commented Jun 10, 2024

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:

  • "rebase" to perform merges at the risk of introducing conflicts, with the benefit of not repeating the action (due to resource consumption or other undesired side effects).
  • "fix" to repeat a cheap action so as to avoid conflicts.
  • "reparent" to avoid propagating the changes at all.
  • Maybe some topology-changing stuff like squashing, re-ordering, or parallelizing.

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

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

@arxanas
Copy link
Contributor Author

arxanas commented Jun 15, 2024

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

Yes. Here's my most common workflows:

  • Run a side-effectful command, like submit a commit for code review.
  • Run a "check" command (lint, build, or test) and aggregate the results.
  • Run an analysis command and aggregate the results. For example, run an automatic benchmark on each commit to identify potential regressions or improvements.
  • Run a "fix" command (like a formatter or linter with autofixes) and apply the results.
  • There are also a couple of commit-graph-oriented workflows that are probably out of scope for this discussion 👀
    • Test case minimization (find a minimal subset of commits that satisfies a predicate).
    • Performance optimization (find a roughly minimal subset of commits with a roughly greatest value for some evaluation function).

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

  • Linear search: I probably want to stop submitting code reviews in my commit stack when one of the commits fails to be submitted.
  • Binary search: I want to efficiently find the commit that broke a check and fix it, either manually or automatically.
    • Or maybe I want to find that commit just so I can submit all the known-good commits in the stack.
  • Reverse-linear search: I think that one of my very recent commits broke a check and would rather start the search there.
  • No termination: I want to know exactly which commits pass/fail a certain check (or all checks).

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.

  • As a kind of litmus test: if my organization set up my fix configuration for me, it would appear mostly arbitrary if some of the fixes had to be done via jj fix and others had to be done via jj run --fix.
    • It would also be confusing if my organization ended up optimizing a fix (to run on files instead of working copies) and required me to change my invocation, despite there being no user-visible behavioral change.

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

  • If it's slow, then I'll spend time manually optimizing the flags to reduce the overall runtime.
    • For example, git test fix -x 'cargo clippy --workspace --fix' always works and could be a pre-defined tool/configuration,
    • but I might as well run git test fix -x 'cargo clippy -p <package> --fix' if I know which package I want to operate on and save myself some time (if I'm sensitive to that).

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

  • A per-file fix that's fast even when running per-working-copy: in my repo, running cargo fmt is fast enough that it's not worth the time to manually or automatically configure it to run on only changed files.
  • A per-working-copy fix that's fast regardless: cargo update technically requires looking at all the Cargo.tomls in the workspace, and fundamentally not able to operate per-file, but it's fast enough in my repo that I don't care.

@arxanas
Copy link
Contributor Author

arxanas commented Aug 14, 2024

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

  • Users today want to run and fix formatters and linters that don't directly fit into the current per-file jj fix paradigm.
    • The fixes may not be able to operate on individual files, either for fundamental reasons (like requiring access to other files or the build graph) or practical reasons (not implemented to do so).
  • Users seem to logically think of jj fix as running on entire revisions by default.
    • Then the implementation of the fix tool itself (and how it accepts input) seems to be an implementation detail, rather than part of the fundamental mental model.
    • Alternatively, jj fix is seen as a more ergonomic or efficient way to accomplish per-working-copy tasks that could still be done with jj run; that is,jj run is a different level of abstraction than jj fix.
  • Users value aliases/streamlined workflows for fixing.
  • Users have concretely separate workflows in mind for querying (running unit tests) vs fixing (formatting).
  • Users don't expect jj run to apply fixes without opting in.

Compared to my original mental model:

  • Users expect jj fix to be the same fundamental kind of thing as jj run.
    • Confirmed: It seems like jj fix should be able to materialize working copies in some mode, to reduce user surprise/friction here. I don't see indication that users consider them to be specifically different in terms of "per-file" vs "per-working-copy" operation.
    • "Specialization" seems to mean that it makes jj run use-cases easier, but not necessarily that it's incompatible with jj run use-cases.
    • Since we are still in the "early adoption" phase, users like @jennings will naturally be willing to fit their workflow into whatever jj provides, rather than give up. I would expect later adopters to find this to be more of a stumbling block.
  • Users think of jj fix as being at a higher level of abstraction than jj run.
    • Refuted: It suggests the coexistence of jj fix and jj run --fix, which I was against in my original post here.
    • This is unlike my design in git-branchless, which treats run and fix at the same level of abstraction.

@jennings
Copy link
Contributor

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:

  • It always materializes working copies, since some tools need that.
  • It can optionally amend revisions with any files the tool changes.
  • It always runs in-place in the working copy by checking out each revision. This is because it might be expensive to set up a fresh working copy (think npm install), or some projects might be dependent on the absolute file system path
  • There's probably a way to configure tools: run.tools.prettier.command = ["prettier",...], but you can also run one-off commands with --exec or similar.

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

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

hooper commented Aug 16, 2024

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:

  • Scope of execution:
    • Run a command commit-by-commit in the original working copy/cwd (an escape hatch for things that really need to mess with the repo in a way that's unsupportable in the other options; requires or implies --parallelism=1)
    • Run a command commit-by-commit in the known workspaces of the repo (maybe this is redundant with a cache for the next few modes)
    • Run a command commit-by-commit in working copies that get snapshotted to amend the commits (for multi-file fixes)
    • Run a command commit-by-commit in mutable working copies that don't get snapshotted (for multi-file analysis that can't deal with a readonly copy)
    • Run a command commit-by-commit in readonly working copies (for compiling/testing/linting etc; creating an opportunity for backends to be simpler or more efficient than for a mutable working copy)
    • Run a command on some files one-by-one, in place, optionally rewriting commits (for respecting .clang-format during clang-format -i $path; this might be redundant with the snapshotted working copy modes)
    • Run a command on some files one-by-one, via stdin/stdout, optionally rewriting commits (to supersede jj fix)
  • Execution parallelism:
    • "Auto" to choose an appropriate parallelism based on other options, available RAM and/or CPU, and any need for sequencing.
    • An explicit natural number.
    • Possibly also allow control of the number of working copies that are materialized, separately from execution parallelism.
  • Execution order:
    • Topological (useful for improving build incrementality)
    • Chronological (useful if side effects need to have the same order)
    • Reverse topological
    • Reverse chronological
    • Unspecified (creating an opportunity for backends to be more efficient in unforeseen ways)
    • Bisection
      • All execution orders will need some notion of when to stop or keep going. Under this model, bisection is just the combination of a particular order and a particular stop condition. The order of execution for bisection has a data dependency on execution results, which is potentially messy, but still allows parallelism.
  • Standard input/output handling:
    • TTY passthrough (for stuff like jj run bash; weird/messy for parallel execution)
    • Pipe to files (like Philip's PR)
    • Pipe to an aggregator, which could be a subprocess or a builtin function or something. For example, an expensive tool could be run on commits in parallel, and a separate user-specified subprocess (or builtin function) could consume the tool's output via pipes and print a usable summary to jj's stdout. Google uses something like this with hg to good effect.
  • File filtering:
    • File arguments should be acceptable in file-by-file execution scopes. Maybe in other scopes to narrow the working copies or something.
    • The file arguments act as a filter for one of the following, based on a flag like --all-files or --changed-files:
      • All files in the commit
      • Changed files in the commit
      • Changed files in the commit plus changed files in ancestors that belong to the revset (for jj fix-like behavior). This can be based on either a diff against a set of common ancestors, as in hg fix, or on incremental diffs as in jj fix. Those are not semantically equivalent.
  • Merging/rewriting:
    • Reparent (like jj fix)
    • Rebase (first class conflicts make it reasonable to offer this)
    • Maybe other options? I haven't thought about it enough.
  • Command specification:
    • Execute argv from jj run <options> [--] <argv> or jj run -r foo --exec '<argv>'. We need to decide if positional arguments are used for argv, file patterns, or otherwise.
    • Execute a builtin function call, for example to allow custom jj builds to send RPCs. The builtin could be identified by a string that gets passed to a flag like --exec-internal.
    • Execute argv(s) from a config option, identified by a string passed to a repeatable flag like jj run -r foo --tool tool1 --tool tool2. Maybe this is the same flag as the builtin. The configuration would define which tools/functions/RPCs are applicable to which execution scopes/modes.

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

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

arxanas commented Aug 18, 2024

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

hooper commented Aug 19, 2024

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:

  • fix is a bad name. We kind of knew that already, but it seems confusing enough that any historical reasons for using it should be abandoned. We could consider something like jj format in the future if it's possible to agree on some sensible defaults. Shipping it as a no-op is a bit weird.
  • The current UX trade offs of jj fix are a little too monorepo focused for many users' expectations. Tucking it away as a variation of jj run probably makes sense in the long term, from the user perspective and technically.

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?

@Minion3665
Copy link
Contributor

Minion3665 commented Oct 7, 2024

However, these formatters have to operate in-memory on single files, which limits the utility somewhat

To set up formatters in a much more simple way

I think it may be worth mentioning here that some formatting scenarios, even with formatters that support in-memory use are not, to my knowledge, possible to sensibly set up with jj fix as it currently is. An example is a formatter which has configuration in a file or an ignore list - which might be common if the formatter was added to the project later and you don't want to have a large commit changing all the formatting.

For prettier, say, you can get around the configuration by adding a repo-specific setting where you give arguments to prettier (and change them if the in-repo config is ever changed), and get around the ignore file by specifying the paths manually, but it's non-ideal and requires you to remember whether a file should be formatted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants