-
Notifications
You must be signed in to change notification settings - Fork 784
cli: Split jj squash into jj squash and jj amend
#7664
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
base: main
Are you sure you want to change the base?
Conversation
a32c9f5 to
23aec32
Compare
|
I know this is bikeshedding but can the command be named |
|
According to #7465, you probably want to remove more options from Also I'm not sure why you haven't kept |
I beg to differ about this. Maybe from a perspective of knowing implementation details, it makes more sense. But for me (who doesn't know the code), "moving a diff" makes no sense at all. |
dd8e02e to
fe82e9a
Compare
Done
My rationale was that
Moving a diff is simply the first step of squashing. It "squashes" the changes, but not any metadata.
Roughly speaking, squash is a four stage process:
|
Again, it sounds more like implementation details. I think it's better to hide all that from the user, and just do what needs to be done. The user shouldn't have to run multiple commands to accomplish the squash. |
They aren't implementation details though, because it affects how squash behaves in a meaningful way to a user.
It's not an implementation detail because, for example, I frequently don't want jj to abandon a commit that I was squashing from, or to combine the descriptions.
No-one is suggesting having to run multiple commands to accomplish the squash. For now, this splits squash into two commands:
There has been discussions about removing the partial squash functionality from squash. This is still under debate, so I'm not implemmenting this. However, even if this were implemented, this would also not require you to use multiple commands to accomplish the squash. You would simply choose between |
I seems like implementation detail to me, since I think
Again, this doesn't actually make sense to me since I don't know what is in a revision (and I shouldn't have to know to use the tool). |
By that definition, every part of squash is an implementation detail. The definition of an implementation detail is a piece of information that only someone writing the feature needs to know, and not someone using it. Whether all workflows use it is completely irrelevant (except if you can achieve it with other commands, which you cannot). The only thing that matters is whether it makes a meaningful difference to the user, which in this case, it does. To some users it might be an implementation detail, but discussions have clearly shown that it isn't an implementation detail to all users, and some people care a great deal about this (see #7465).
You don't need to know what's in a revision to use this tool - that's what |
PhilipMetzger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits
It is very relevant, when you refer to choosing which command to use by knowing what is in the revision! How can you say it's not relevant that your statement made no sense? I don't know how to choose between the commands because I don't know what's in a revision. When you "explain" the command in terms that don't make sense, how is the user supposed to learn the tool? I know it's difficult, but you have to design as if you don't know what you know. I don't look at the details, so I can tell you that this makes no sense to a novice. |
fe82e9a to
48e8f62
Compare
jj squash into jj squash and jj movediffjj squash into jj squash and jj diffmove
There are two questions at play here:
Your previous commands seemed to be mostly about whether the command should exist (perhaps because you don't think it's easy to understand for a user?), while your latest seems to be complaining that it's not clear what it does. I personally think the name explains what it does, but I will admit that I (and probably most other people commenting on jj PRs) understand that a revision is just a set of diffs + some metadata, and that to "squash" is to move those diffs from one revision to another. Do you have a better name for it, or a way to better explain it? |
and
These both have the insider knowledge built-in, and you can't even see it. If there are two commands, you have to figure out which to use, and the basis to decide is what you explain is implementation details of "revision is just a diff with metadata". When I use version control, it's "changes to files". I don't think in terms of diff or metadata. I might have to update something based on a review, but it's still all about how a file's content is changed (and it is confusing when you use the word "content" differently). |
I don't think "revision is just a diff with metadata" is fair to call an implementation detail - I think that's basic knowledge that everyone should have. The literal meaning of a diff is "changes to files", and while metadata can be understood to a varying degree, at the bare minimum users should understand that a revision can have a description, and most should understand that bookmarks, for example, are metadata associated with a revision (semantically - as an implementation detail I have no clue where they're stored). So
Fair, but note that the term "content" doesn't exist in the documentation, I only used it in this discussion
That's a good point, and a very philosophical question. To phrase it in the terminology you're more familiar with then,
I think that the pros and cons are:
I called it Personally though, I do agree that I personally feel that Also, this way |
...according to you... The trick is that my concept of diff is that it's a transient thing, and what the VCS is storing is my files. So talking about moving a diff makes no sense when all I have is files. That's why it's an implementation detail. |
b566ead to
baab557
Compare
As has been discussed in several places, `jj squash` is a very
complicated command that can do many things.
`jj movediff` is much simpler to explain ("moves diffs from one commit
to another").
It is implemented in terms of squash because the difference between the
two is more semantic than programatic.
In the future, we *may* consider removing the ability for squash to
operate on individual files, but that is still hotly debated.
|
It's very clear to me now from talking to coworkers that while I've renamed it to |
baab557 to
efdb809
Compare
jj squash into jj squash and jj diffmovejj squash into jj squash and jj amend
Maybe you can tackle |
Definitely outside the scope of this PR. That'd need to be a whole seperate discussion. |
|
Just to be sure, when you talked to your coworkers, did you explain that the command moves changes from one commit to another? I want to make sure that people don't think of it just as the more limited |
|
I'm still not in favor of doing such a thing even without considering the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in the commit message you still talk about movediff/diffmove when the actual command name now is amend. I still am unconvinced on it though.
That's a good point. My personal opinion is that amend means "fix something up with something else". Thus, I think that My survey of my teammates (different teammates from before, so they wouldn't be primed):
My conclusion:
|
As has been discussed in several places,
jj squashis a very complicated command that can do many things.jj movediffis much simpler to explain ("moves diffs from one commit to another").It is implemented in terms of squash because the difference between the two is more semantic than programatic.
In the future, we may consider removing the ability for squash to operate on individual files, but that is still hotly debated.
This works towards #7465
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)