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: Allow jj squash to span multiple commits. #2678

Closed
PhilipMetzger opened this issue Dec 7, 2023 · 12 comments · Fixed by #3276
Closed

FR: Allow jj squash to span multiple commits. #2678

PhilipMetzger opened this issue Dec 7, 2023 · 12 comments · Fixed by #3276
Labels
enhancement New feature or request

Comments

@PhilipMetzger
Copy link
Collaborator

PhilipMetzger commented Dec 7, 2023

We've also discussed this already in Discord. Particularly useful if you need only a single patch for codereview.
Ideally this is can be implemented, when/if we make squash an alias for move.

Edit: The insightful Discussion around making squash an alias of move starts here. It also proposes the feature for jj move some messages below.

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Dec 17, 2023
@ilyagr
Copy link
Collaborator

ilyagr commented Dec 29, 2023

If we don't make squash and alias for move (I haven't thought much about that), I think jj move -r A::B --to C would make sense as an interface. I don't think we'd have to implement any version of this in squash, it feels confusing at first glance.

@martinvonz
Copy link
Owner

I suspect we shouldn't make squash an alias.

I think jj move -r A::B --to C would make sense as an interface

There's currently jj move --from A --to C. I would suggest extending --from to accept more than a single revision. I'm not sure if we should require the all: prefix.

@ilyagr
Copy link
Collaborator

ilyagr commented Dec 29, 2023

Absolutely, I meant to say --from.

I would probably require all:, that's also a good point. In principle, we could make an exception for renders of the form A::B, but that seems a bit too complicated for the benefit.

So, the above turns into something like

jj move --from all:A::B --to C

@joyously
Copy link

I don't think there should be a command called move that deals with commits. Other VCSs use the move concept for files. See https://en.wikipedia.org/wiki/Comparison_of_version-control_software#Basic_commands

@PhilipMetzger
Copy link
Collaborator Author

I don't think there should be a command called move that deals with commits. Other VCSs use the move concept for files. See https://en.wikipedia.org/wiki/Comparison_of_version-control_software#Basic_commands

Yeah, that's totally fair. As both #2678 (comment) and #2678 (comment) hinted at, this shouldn't be the way forward. But recently there has been a discussion (#2882) to move jj move into jj squash which everyone probably prefers.

@amiryal
Copy link
Collaborator

amiryal commented Feb 20, 2024

Let’s leave jj squash to deal with a single revision. For dealing with a revset, I suggest a new command jj collapse -r REVISIONS.

The way it would work is up for debate. My idea is to identify the roots and the heads within the revset, associate each root with the set of heads that are descended from it, virtually merge each set of heads, and finally “collapse” the merged sets of heads down to their respective roots. In the final result only the roots of the revset remain, and their contents are of the virtual merges of their descendant heads; the descendants of the former heads of the revset are rebased onto the roots of it.

In the simple case of a single linear descent, the result is intuitive. In other cases, maybe an all: prefix or some other form of acknowledgement should be required. The other cases may get as ugly as overlapping sets of descendant heads, which themselves may have descendants that are merges… 🤕

@joyously
Copy link

Let’s leave jj squash to deal with a single revision. For dealing with a revset, I suggest a new command jj collapse -r REVISIONS.

This sounds a lot like what the proposed absorb would be.

@PhilipMetzger
Copy link
Collaborator Author

Let’s leave jj squash to deal with a single revision.

Why though? I think its in our complexity/novelty budget to give it new powers.

For dealing with a revset, I suggest a new command jj collapse -r REVISIONS.

That sounds like sl fold which I find a better synonym, so I'd rather have jj fold if we even go through with it.

The way it would work is up for debate. My idea is to identify the roots and the heads within the revset, associate each root with the set of heads that are descended from it, virtually merge each set of heads, and finally “collapse” the merged sets of heads down to their respective roots. In the final result only the roots of the revset remain, and their contents are of the virtual merges of their descendant heads; the descendants of the former heads of the revset are rebased onto the roots of it.

In the simple case of a single linear descent, the result is intuitive. In other cases, maybe an all: prefix or some other form of acknowledgement should be required. The other cases may get as ugly as overlapping sets of descendant heads, which themselves may have descendants that are merges… 🤕

This really sounds like #170.

@amiryal
Copy link
Collaborator

amiryal commented Feb 20, 2024

That sounds like sl fold

Yes, but if I understand sl fold correctly, it refuses to work on a non-linear chain, and keeps only the head, not the root (like jj unsquash, but unlike jj squash or my suggested jj collapse). Thus, using the same name might be confusing.

This sounds a lot like what the proposed absorb would be.

No; let me try to explain.

What jj squash currently does to a single commit C: move its changes to its parent commit, and if C is left empty, delete it.

What this FR suggests to do to a linear chain of commits of length N: move the cumulated changes from the top N - 1 commits to the first commit and delete the top N - 1 commits.

What my understanding of absorb would do to a single commit C: divide the changes from C, between some or all of its unpublished ancestor commits, by using an algorithm that figures out which chunk of changes should go into which ancestor commit.

The way I suggested that jj collapse would work is a generalisation, from “a linear chain of commits of length N” as described above, to a revset with potentially multiple roots, heads and merges in between, not necessarily interconnected.

@julienvincent
Copy link
Collaborator

julienvincent commented Feb 21, 2024

Probably not constructive to this conversation, but I was wanting to do this and I concocted this monstrocity:

jj log -r master+..tzyrpmnw --template 'change_id ++ "\n"' --no-graph | xargs -I {} jj squash -r {} -m "Final commit message"

Which will one-by-one squash all commits in a given range. Just dumping this here in case it is useful to anyone who stumbles across this issue.

@martinvonz
Copy link
Owner

What this FR suggests to do to a linear chain of commits of length N: move the cumulated changes from the top N - 1 commits to the first commit and delete the top N - 1 commits.

What I had in mind was actually not to limit it to a linear chain. I think it makes sense to move/squash any number of changes into a given destination. All the source revisions would become empty and abandoned. You could combine that with paths too, which means some of the source revisions might remain. Combining it with -i would be possible but would be annoying to use if it opens up the diff editor once per source revision.

@PhilipMetzger
Copy link
Collaborator Author

That sounds like sl fold

Yes, but if I understand sl fold correctly, it refuses to work on a non-linear chain, and keeps only the head, not the root (like jj unsquash, but unlike jj squash or my suggested jj collapse). Thus, using the same name might be confusing.

I don't think this will matter as there are way less Sapling users than Git, so we could adopt the name. It will only be confusing initially.

What jj squash currently does to a single commit C: move its changes to its parent commit, and if C is left empty, delete it.

What this FR suggests to do to a linear chain of commits of length N: move the cumulated changes from the top N - 1 commits to the first commit and delete the top N - 1 commits.
[...]
The way I suggested that jj collapse would work is a generalisation, from “a linear chain of commits of length N” as described above, to a revset with potentially multiple roots, heads and merges in between, not necessarily interconnected.

Yes, that seems to be a agreeable solution, although having the option to squash everything would be better.

What my understanding of absorb would do to a single commit C: divide the changes from C, between some or all of its unpublished ancestor commits, by using an algorithm that figures out which chunk of changes should go into which ancestor commit.

Yes, thats also my understanding.

What this FR suggests to do to a linear chain of commits of length N: move the cumulated changes from the top N - 1 commits to the first commit and delete the top N - 1 commits.

What I had in mind was actually not to limit it to a linear chain. I think it makes sense to move/squash any number of changes into a given destination. All the source revisions would become empty and abandoned. You could combine that with paths too, which means some of the source revisions might remain. Combining it with -i would be possible but would be annoying to use if it opens up the diff editor once per source revision.

That's my favorite solution so far. It'd also make sense if we follow-through with #2882 .

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

Successfully merging a pull request may close this issue.

6 participants