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: Implement JJ run #1869

Open
2 of 5 tasks
PhilipMetzger opened this issue Jul 14, 2023 · 11 comments
Open
2 of 5 tasks

FR: Implement JJ run #1869

PhilipMetzger opened this issue Jul 14, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent

Comments

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Jul 14, 2023

This is the tracking issue for run, mostly for myself.

The design doc is available here (Google Docs) and here (docs/design).

Related to #405

  • Add a barebones run command, which is hidden.
  • Move the design doc from Google Docs to docs/design.
  • Add the VFS integration point, for EdenFS and CitC.
  • Implement the worker interface for the Git backend.
  • Add a beautiful TUI with superconsole, based on @arxanas suggestion in On `jj run` #1486.

this should establish the base features of run.

And to add, here's a simple wishlist based on Discord conversations:

  • Add run --bisect, to support a git bisect -x like workflow based on git-branchless's git test.1
  • Supporting bayesian based bisection 2
  • Partially exposing the underlying repo, for the default backend with something like --files [PATHS]

Footnotes

  1. My suggestion in Discord

  2. A suggestion from @thoughtpolice in Discord

@PhilipMetzger PhilipMetzger added enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent labels Jul 14, 2023
@PhilipMetzger PhilipMetzger self-assigned this Jul 14, 2023
@arxanas
Copy link
Contributor

arxanas commented Jul 14, 2023

The scm-bisect crate contains bisection logic already. For git run --bisect, it would be nice if we could centralize logic into scm-bisect so that other version control systems could potentially use it.

PhilipMetzger added a commit that referenced this issue Aug 3, 2023
This adds the `run` command as described in the doc.
The command is hidden for now, to allow in-tree development. 

The next steps are: 

1. Move it to `run.rs`
2. Find a good backend trait
3. Implement the workers

Initial progress on #1869 and #405
@PhilipMetzger PhilipMetzger mentioned this issue Aug 3, 2023
4 tasks
PhilipMetzger added a commit that referenced this issue Aug 3, 2023
This adds the `run` command as described in the doc.
The command is hidden for now, to allow in-tree development. 

The next steps are: 

1. Move it to `run.rs`
2. Find a good backend trait
3. Implement the workers

Initial progress on #1869 and #405
PhilipMetzger added a commit to PhilipMetzger/jj that referenced this issue Aug 4, 2023
This adds the `run` command as described in the doc.
The command is hidden for now, to allow in-tree development. 

The next steps are: 

1. Move it to `run.rs`
2. Find a good backend trait
3. Implement the workers

Initial progress on jj-vcs#1869 and jj-vcs#405
PhilipMetzger added a commit that referenced this issue Aug 4, 2023
This adds the `run` command as described in the doc.
The command is hidden for now, to allow in-tree development. 

The next steps are: 

1. Move it to `run.rs`
2. Find a good backend trait
3. Implement the workers

Initial progress on #1869 and #405
PhilipMetzger added a commit that referenced this issue Sep 7, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 7, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 7, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 8, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 11, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 11, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 13, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
PhilipMetzger added a commit that referenced this issue Sep 14, 2023
This ticks another box in #1869.

Co-Authored-By: arxanas <[email protected]>
Co-Authored-By: hooper <[email protected]>
Co-Authored-By: martinvonz <[email protected]>
@thoughtpolice
Copy link
Member

thoughtpolice commented Oct 31, 2023

As an update to the entry for bayesian bisection support, this just came out of Google pretty recently: Flake Aware Culprit Finding. https://research.google/pubs/pub52048/ This is certainly a good overview of what such a tool would try to achieve and look like, and what assumptions it would make.

Algorithms 2 and 3 in Section 3 effectively summarize the whole algorithm, operationally.

PhilipMetzger added a commit that referenced this issue Dec 9, 2023
In principle this trait is a layer above an actual VFS, but should be enough to 
start working with them. It's responsible for caching and managing working-copies which 
are stored locally or on a remote. 

The plan is that `jj run` will query it to request working copies, which it then uses. 

This checks the third checkmark in #1869. 

Progress on #1869 and #405

cc @martinvonz, @hooper, @kevincliao, @arxanas
PhilipMetzger added a commit that referenced this issue Mar 7, 2024
In principle this trait is a layer above an actual VFS, but should be enough to 
start working with them. It's responsible for caching and managing working-copies which 
are stored locally or on a remote. 

The plan is that `jj run` will query it to request working copies, which it then uses. 

The client is coming with the workqueue which is step 4 in #1869. 

This checks the third checkmark in #1869. 

Progress on #1869 and #405

cc @martinvonz, @hooper, @kevincliao, @arxanas
@PhilipMetzger
Copy link
Contributor Author

@matts1 Moving this conversation here, as this is the correct place for it.

Comments on jj run
@PhilipMetzger, a few suggestions on your design doc for jj run (is there a good way to give feedback on design docs on > github - comments on a google doc work pretty well, but there doesn't seem to be anything like that for github):

If I understand correctly, exactly one of --dry-run, --rebase, --reparent, and --readonly must be chosen
Probably worth calling out on there. It's very unclear, especially because --clean is stuck between them.

Just --reparent and --rebase are exclusive, as they operate on the repos history. --readonly only exists for specific workflows, while --clean is there for manual garbage collection until there's a better way. And the section is only there to provide an overview of all requested features.

Based on the fact that you say that you can write by default, and that it's parallel by default, I assume that the default is -- >reparent?

No, the default is to rebase the "new"/"modified" commits, as its jj's default. While you could argue that run should be minimally intrusive into your existing tree, it just isn't. I mean should act as if you've run <command> on all ancestors manually and it just is the obvious behavior.

You write that by default, jj run works on @.
I'd personally like jj run to work by default on mutable()::@ (If I wanted to run jj run -r @ 'command', I'd instead just run > command directly).
I think it'd be valuable to instead do the same thing we do for log, where we add revsets.edit (which could default to @)

Yeah, this totally makes sense, but as the Design Doc was written when neither trunk() nor immutable_heads() existed, it was the correct choice to just use @.

No mention was made about the working directory, so I'm curious how relative paths would work
IMO, we should set the $JJ_REPO environment variable so that a relative path to the root of the repo can be $JJ_REPO/foo
Maintaining relative paths has value
A unix mount namespace could solve this by literally just overlaying our new virtual file system on top of the real one
But unclear how this'd work on non-unix systems

This is another great point, which never was brought up. I agree that the $JJ_REPO env var should be included from the beginning.

@matts1
Copy link
Contributor

matts1 commented May 31, 2024

Could you clarify what --readonly does in more detail. The documentation says " ignore changes across multiple run invocations", and that seems like it could mean what I understand --readonly to mean, but it feels like a wierd way to phrase it. My understanding of --readonly would either be:

  • If the run command attempts to modify the tree, throw an error and don't update the commit with those changes
  • If the run command attempts to modify the tree, don't update the commit with those changes

If my understanding is correct, then surely readonly / reparent / rebase should be mutually exclusive - it doesn't make much sense to rebase something if it's readonly

--readonly only exists for specific workflows

This doesn't make much sense to me given my understanding of how --readonly would work. Could you clarify what you mean by this

No, the default is to rebase the "new"/"modified" commits, as its jj's default. While you could argue that run should be minimally intrusive into your existing tree, it just isn't. I mean should act as if you've run <command> on all ancestors manually and it just is the obvious behavior.

The reason I said that I assumed reparent would be the default is that IIRC (might be misremembering this) you said somewhere that by default it would run in parallel, and I thought that would be incompatible with rebase. I figured that that reparent and readonly can run edit in parallel on all specified revisions at once, because they have no dependency on the parent's post-run tree. On the other hand, rebase may have changed the tree by the time you got to a revision, so you can't do it in parallel, and it can create conflicts.

Maybe I'm misunderstanding how you're implementing rebase (maybe you run commands then do rebasing or something like that - if so, probably worth clarifying in the docs), but here was my understanding of the difference between rebase and reparent.

Suppose I have two changes A and B stacked on top of each other, with commit IDs a1 and b1 respectively.

Now I run jj run --rebase <command>:

  1. <command> runs on a1, changing A to CommitID a2
  2. B is rebased onto a2, changing B to CommitId b2
  3. <command> runs on b2, changing B to CommitId b3

You cannot run steps 1 and 3 at the same time because step 3 is dependent on the tree b2, which is dependent on the rebase and thus on the tree a2, which requires running step 1 to determine.

On the other hand, jj run --reparent <command> would:

  • <command> runs on a1, changing A to CommitId a2
  • <command> runs on b1, changing B to CommitId b2

Note that this would also mean that if conflicts were generated with --rebase, jj run would either have to back out, or attempt to run on a conflict.

@arxanas
Copy link
Contributor

arxanas commented Jun 1, 2024

@PhilipMetzger do you or someone else have immediate plans to implement jj run, and will the first implementation be exactly according to the scope set out in the design document?


It doesn't make sense for jj run to be making changes to the working copy/commit graph as part of its default mode of operation —

  • if it's a good change to make, then it's a "fix" and belongs in jj fix;
  • if it's a bad change to make, then it shouldn't be processed anyways.

An initial implementation of jj run should probably be restricted to running a command on each of a set of commits and collecting the output/exit codes and presenting that to the user somehow. Then jj fix, etc., can be modified to make use of the jj run infrastructure.

I think the relevant issue for rebasing/reparenting behavior is now #3805.


No mention was made about the working directory, so I'm curious how relative paths would work
IMO, we should set the $JJ_REPO environment variable so that a relative path to the root of the repo can be $JJ_REPO/foo
Maintaining relative paths has value
This is another great point, which never was brought up. I agree that the $JJ_REPO env var should be included from the beginning.

I don't disagree with the idea of preserving relative paths, but I don't think it needs to go into an initial implementation?


The reason I said that I assumed reparent would be the default is that IIRC (might be misremembering this) you said somewhere that by default it would run in parallel, and I thought that would be incompatible with rebase. I figured that that reparent and readonly can run edit in parallel on all specified revisions at once, because they have no dependency on the parent's post-run tree. On the other hand, rebase may have changed the tree by the time you got to a revision, so you can't do it in parallel, and it can create conflicts.

I don't know what specifically was being referred to, but there is a useful rebase approach that does have the potential to induce conflicts, similar to git rebase -x. It's most useful for commands that operate on the diff of the working copy with its parent, rather than the whole repository at once (for which reparenting might be more appropriate if the operation is "coherent"" in a mathematical sense).

@PhilipMetzger
Copy link
Contributor Author

PhilipMetzger commented Jun 1, 2024

I still plan to implement run to answer that question, but it will be clearly best effort not immediately matching the design doc, even with all the amendments required. The goal is to get incrementally there and probably adding some required features which get requested along the way. And since fix now exists, this hopefully will be easier.

@PhilipMetzger
Copy link
Contributor Author

The VFS can arrive at a later point, as I definitely have no experience writing such a thing, as Googlers will have a their requirements and experience from already building one.

@matts1
Copy link
Contributor

matts1 commented Jun 3, 2024

if it's a good change to make, then it's a "fix" and belongs in jj fix

This isn't necessarily true. IIUC, jj fix is optimized to run a single tool on many different files. On the other hand, jj run could be used for something like REGENERATE_GOLDENS=1 <run tests> to regenerate golden testdata. Whether that's enough of a justification to make the default mode not readonly, that's another question.

@arxanas
Copy link
Contributor

arxanas commented Jun 15, 2024

This isn't necessarily true. IIUC, jj fix is optimized to run a single tool on many different files. On the other hand, jj run could be used for something like REGENERATE_GOLDENS=1 to regenerate golden testdata. Whether that's enough of a justification to make the default mode not readonly, that's another question.

Ah, to be clear, I meant to describe the ideal UI: things which are logically "fixes", which includes both formatting changes and updating snapshot tests, should go into jj fix. I commented at #3808 (comment) to expand on why jj fix shouldn't be limited to only files.

I still plan to implement run to answer that question, but it will be clearly best effort not immediately matching the design doc, even with all the amendments required. The goal is to get incrementally there and probably adding some required features which get requested along the way. And since fix now exists, this hopefully will be easier.

What I'm suggesting is cutting scope rather than adding to it (i.e. don't make jj run able to modify commits to begin with), so it should make your job easier 😉.

PhilipMetzger added a commit to PhilipMetzger/jj that referenced this issue Jun 20, 2024
In principle this trait is a layer above an actual VFS, but should be enough to 
start working with them. It's responsible for caching and managing working-copies which 
are stored locally or on a remote. 

The plan is that `jj run` will query it to request working copies, which it then uses. 

The client is coming with the workqueue which is step 4 in jj-vcs#1869. 

This checks the third checkmark in jj-vcs#1869. 

Progress on jj-vcs#1869 and jj-vcs#405

cc @martinvonz, @hooper, @kevincliao, @arxanas
PhilipMetzger added a commit to PhilipMetzger/jj that referenced this issue Jun 22, 2024
In principle this trait is a layer above an actual VFS, but should be enough to 
start working with them. It's responsible for caching and managing working-copies which 
are stored locally or on a remote. 

The plan is that `jj run` will query it to request working copies, which it then uses. 

The client is coming with the workqueue which is step 4 in jj-vcs#1869. 

This checks the third checkmark in jj-vcs#1869. 

Progress on jj-vcs#1869 and jj-vcs#405

cc @martinvonz, @hooper, @kevincliao, @arxanas
@dbarnett
Copy link
Contributor

Heya @PhilipMetzger @arxanas, think there's anything here or in related issues that could use help? Is #4021 still a good place to look for the latest?

I got some coding time on my hands and am still itching to get jj to support precommit hooks/checks.

@PhilipMetzger
Copy link
Contributor Author

Yes. it is the latest. See also the discussions in these issues #3575 and #3577 for some more infos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent
Projects
None yet
Development

No branches or pull requests

5 participants