Implement transform_descendants(), rewrite jj parallelize
#3521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emesterhazy
reviewed
Apr 17, 2024
martinvonz
force-pushed
the
push-msstusvklrml
branch
3 times, most recently
from
b549ada to
bebd5fe
Compare
martinvonz
commented
Apr 18, 2024
yuja
reviewed
Apr 18, 2024
martinvonz
force-pushed
the
push-msstusvklrml
branch
from
bebd5fe to
b29608e
Compare
martinvonz
force-pushed
the
push-rlknuwzmnzks
branch
from
9c2bb3c to
3e7e65f
Compare
martinvonz
force-pushed
the
push-msstusvklrml
branch
2 times, most recently
from
a072d6c to
c50227a
Compare
…nts()` There are several existing commands that would benefit from an API that makes it easier to rewrite a whole graph of commits while transforming them in some way. `jj squash` is one example. When squashing into an ancestor, that command currently rewrites the ancestor, then rebases descendants, and then rewrites the rewritten source commit. It would be better to rewrite the source commit (and any descendants) only once. Another example is the future `jj fix`. That command will want to rewrite a graph while updating the trees. There's currently no good API for that; you have to manually iterate over descendants and rewrite them. This patch adds a new `MutableRepo::transform_descendants()` method that takes a callback which gets a `CommitRewriter` passed to it. The callback can then decide to change the parents, the tree, etc. The callback is also free to leave the commit in place or to abandon it. I updated the regular `rebase_descendants()` to use the new function in order to exercise it. I hope we can replace all of the `rebase_descendant_*()` flavors later. I added a `replace_parent()` method that was a bit useful for the test case. It could easily be hard-coded in the test case instead, but I think the method will be useful for `jj git sync` and similar in the future.
We always call `.to_vec()` on the slice, so let's just have the caller pass in an owned vector instead.
`jj parallelize` was a good example of a command that can be simplified by the new API, so I decided to rewrite it as an example. The rewritten version is more flexible and doesn't actually need the restrictions from the old version (such as checking that the commits are connected). I still left the check for now to keep this patch somewhat small. A subsequent commit will remove the restrictions.
The new API makes it easy to leave commits in place if their parents didn't change, so let's do that.
I'm going to make some `jj parallelize` cases that currently error out instead be successful. Some of the will result in ancestor merges with the root commit. This patch updates those tests to avoid that.
Should make it easier to understand the graph shape when there are lots of crossing lines.
The checks are not needed by the new implementation, so just drop them.
The rewritten code is already a no-op when there's a single input. I don't think the case is common enough to warrant having a special case for performance reasons either. Also, by not having the special case, `jj parallelize <immutable commit>` fails consistently with the non-singleton case.
martinvonz
force-pushed
the
push-msstusvklrml
branch
from
c50227a to
fa6b01d
Compare
martinvonz
changed the title
transform_descendants(), rewrite jj parallelizetransform_descendants(), rewrite jj parallelize
|
@yuja: This PR still requires an approval. I don't mean to rush you, just making sure you didn't forget it (I sometimes think that I had approved a PR that I had not approved). Take your time if you need. |
yuja
approved these changes
Apr 19, 2024
Makes sense. Thanks for reviewing! @bnjmnt4n, @emesterhazy, and/or @ilyagr might want to take a look. I'll merge it based on Yuya's review and to unblock e.g. Benjamin's |
emesterhazy
added a commit
that referenced
this pull request
Apr 20, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 20, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 20, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 21, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 21, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 21, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
emesterhazy
added a commit
that referenced
this pull request
Apr 21, 2024
This is following on the rewrite for `parallelize`. - #3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@bnjmnt4n: This PR includes the
transform_descendants()function.@emesterhazy: FYI, this is how I'm planning to rewrite
jj parallelize.Checklist
If applicable:
CHANGELOG.md