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: jj squash should allow you to preserve the commit even if it was made empty #3815

Closed
matts1 opened this issue Jun 3, 2024 · 15 comments · Fixed by #3830
Closed

FR: jj squash should allow you to preserve the commit even if it was made empty #3815

matts1 opened this issue Jun 3, 2024 · 15 comments · Fixed by #3830

Comments

@matts1
Copy link
Contributor

matts1 commented Jun 3, 2024

Is your feature request related to a problem? Please describe.
Squash is one command designed for two different semantic use cases. To be honest, this makes me very concerned, as it feels very much like the mistake git made with checkout where they needed to, later on, split it into git switch and git restore.

  • Move some files (technically, file diffs) from one commit into another
  • Squash two commits into one, merging both files and commit metadata.

This presents issues when the files that I want to merge just happen to be all the files currently in the commit. For example, consider the following use case:

merge (empty)
|\
A B
  1. I write code in the merge commit
  2. I run jj squash --from merge --to A

I would want, in this scenario, to preserve everything about the merge commit:

  • Commit description
  • Change ID
  • branch names
  • topic names (once implemented)

However, in reality, my merge commit is abandoned, and it attempts to add the description "merge" to the parent commit (I can avoid this second part with -u). It then creates a new merge commit with an empty string as the description.

Describe the solution you'd like
I think that it's really nice to have a very clear distinction of "this command modifies commits. This command modifies files within commits". My personal preference would be to split jj squash into two commands.

  • jj squash would work on whole commits. It would no longer support --interactive, or paths (if you wanted that, you would use move instead)
  • jj move would work on files. It wouldn't touch anything related to commits.

However, maybe that's a controversial opinion (I'd be interested to see what others think to see whether it is a controversial one). I think a less extreme approach, if people don't like my first suggestion, would be:

  • jj squash would get an option --no-abandon or something to that end, where it wouldn't abandon empty commits.
  • jj move would be added to the default config as an alias to ["squash", --no-abandon"].

Describe alternatives you've considered
I've described multiple potential approaches above.

Additional context
Add any other context or screenshots about the feature request here.

@joyously
Copy link

joyously commented Jun 3, 2024

I can relate to having "this command modifies commits. This command modifies files within commits", but I don't see how your example illustrates that.
Why would you merge, write code, squash, and want to keep the meta data?

I've always thought that move should be solely for manipulating the file tree, as it is in so many other VCSs and OSs.

@ilyagr
Copy link
Contributor

ilyagr commented Jun 3, 2024

My personal preference would be to split jj squash into two commands...

This is an interesting idea. I think I like it, though I may have missed something and I'm also curious of other people's opinions. However, I don't like the name move for the command that moves changes to files around (it doesn't move files! it moves diffs from commit to commit. You mentioned this distinction off-hand, but I think it is important to emphasize). The first names that come to my mind are move-diff or move-patch, but I hope we can come up with something better.

I think a less extreme approach, if people don't like my first suggestion, would be:

  • jj squash would get an option --no-abandon or something to that end, where it wouldn't abandon empty commits.

If we go this way (which seems useful and uncontroversial to me as well), I think something like --keep-empty would be a better name. We already had a discussion about this for jj rebase, again --keep-emptied seems like another appropriate name, but I think we went with the other one. I also wouldn't create the jj move alias; the user can easily create it, and I'm again not very happy about the name.

Update: For rebase, we went with --skip-empty, so we can't quite reuse it. So, perhaps, --keep-emptied is better for squash, I'm unsure.

Update 2: To elaborate on the move name, AFAIC, one of the reasons to deprecate it as an alias of squash (as we did recently) is that the move name is too generic for this somewhat non-obvious operation.

@ilyagr
Copy link
Contributor

ilyagr commented Jun 3, 2024

Also, I should mention that... for your merge example, jj restore --from merge --to A seems to do exactly what you asked for.

(Update: Nope, they are not the same because of the merge.)

... in the most common case of moving from a child to a parent (Update: in a linear history), jj squash --keep-emptied --from Y+ --to Y is identical jj restore --from Y+ --to Y; they are however different in other cases.

@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

I can relate to having "this command modifies commits. This command modifies files within commits", but I don't see how your example illustrates that.
Why would you merge, write code, squash, and want to keep the meta data?

This was meant to illustrate an example where you might want to keep the metadata associated with a commit. It happened to be what I was doing when I found this problem, but isn't the only time you might want to do so. I can think of ~5-10 different reasons you might want to do so off the top of my head - most are relatively niche, but they do add up.

My use case is that I'm working on multiple features at once, and most of the time I want to compile all those features at the same time. To do this I just have a local working copy which is a merge of all the features, work on that the same way you would a squash-based workflow, and then amend the feature I was working on in the merge commit by running jj squash --from merge --to <feature>.

  • I need to keep the merge commit description because if I ever leave that merge commit while it has an empty description, the merge commit is deleted and I need to re-specify which sets of commits I want to depend on.
  • I want to keep the change ID because I want my change IDs to be stable so I can remember them. This will become more relevant with topics.

This is an interesting idea. I think I like it, but I don't like the name move for the command that moves changes to files around (it doesn't move files! it moves diffs from commit to commit). The first names that come to my mind are move-diff or move-patch, but I hope we can come up with something better.

That seems fair. I mainly used move because I felt like it was a strict improvement to the current behaviour of move. move-diff and move-patch seem technically correct, but I'm worried about potential confusion for new users (is moving patches a difficult concept? I feel like we'd have to ask git users unfamiliar with jj), so I'm not particularly happy with the name either.

If we go this way (which seems useful and uncontroversial to me as well), I think something like --keep-empty would be a better name.

SGTM, I just said the first thing that popped into my head, wasn't fussed on the name.

Also, I should mention that for your merge example, jj restore --from merge --to A seems to do exactly what you asked for.

Ah, I hadn't even considered that! That'd work in my use case (I was working on files that happened to be only modified in one feature). Though the fact that I hadn't even considered that is kind of part of the problem. When I move files to a parent commit, I use squash - I shouldn't have to think about the side effects of such an action, and if there are alternative ways of achieving the same effect without those side effects, and whether those other options have side effects.

@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

I picked my partner's brains, who's unfamiliar with jj, and she said "isn't that just an amend?" So her suggestion was that our move-diff command would be called amend, so you could write jj amend --from foo --to bar.

I think it actually makes a lot of sense for most use cases. If I have the commit chain @ -> B -> C, then I could write jj amend to move the files to the parent, or jj amend --to C to move the files into C. Similarly, if @ was a merge of B and C, I think jj amend --to C makes a lot of sense.

Where it might start to get confusing, though, is:

  • Amending your child a'la unsquash (jj amend --to @+)
  • moving patches to arbitrary commits
  • Amending from arbitrary commits (eg. jj amend --from @- --to @)

I'd be interested to see whether other people feel confused by that, or whether it sounds like a good idea?

@ilyagr
Copy link
Contributor

ilyagr commented Jun 3, 2024

Just in case you don't know this already, jj amend is a pre-defined alias for jj squash in https://github.com/martinvonz/jj/blob/a9694cba274d1dd3f36eb8b669c98bbd8e4daf5c/cli/src/config/misc.toml. See also #3252.

I am not sure what I think about the idea about making them distinct in whether they work on whole commits. At first thought, I don't like it, but maybe I'd get used to it.

@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

Just in case you don't know this already, jj amend is a pre-defined alias for jj squash in https://github.com/martinvonz/jj/blob/a9694cba274d1dd3f36eb8b669c98bbd8e4daf5c/cli/src/config/misc.toml. See also #3252.

Yeah, I'm aware of that. I'm struggling to think of a use case where you actually want amend to modify the commit metadata of either child or parent though (and I can think of a few minor circumstances where it'd be bad to do so). Do you have any?

One argument for it is that if we were to go with my original suggestion of "squash only works on commits, and move-patch only works on files", then we would have to choose whether amend would operate on files or commits. If that were the case, then it should definitely be files (otherwise, we couldn't do amend --interactive, which seems like a dealbreaker).

I am not sure what I think about the idea about making them distinct in whether they work on whole commits.

I feel like you're thinking about this differently to me. Rather than thinking of it as a "whole commit", I think of it as "some subset of files (possibly all)" vs "all files + commit metadata". My point here is that jj amend would still work on the "whole commit" in the sense of it works on all files by default. The only thing that it wouldn't touch is the commit metadata.

@ilyagr
Copy link
Contributor

ilyagr commented Jun 3, 2024

I'm struggling to think of a use case where you actually want amend to modify the commit metadata of either child or parent though (and I can think of a few minor circumstances where it'd be bad to do so). Do you have any?

I am not sure whether you have some distinction between "amend" and "squash" in mind; I currently don't, so I'll answer this for "squash".

I quite often create temporary commits that are meant to be squashed into other commits later (say, after I make sure that they work the way that I expect them to). I quite like them disappearing on jj squash --into destination.

Another related use-case that comes to mind is when I want to squash multiple consecutive commits into one. I don't feel like jj currently provides an excellent way to do that, but one of the best ones I know and often use is to repeat jj squash -r Y+ several times in a row.

@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

I am not sure whether you have some distinction between "amend" and "squash" in mind; I currently don't, so I'll answer this for "squash".

I do have a distinction in mind - to me, the intention of jj squash is "these two commits should become one", while the intention of amend is "make some changes to a commit".

After thinking about it further though, I think my question may be a bit silly. To me, squash and amend are two different commands, and thus those examples you provided to me are squashes, and I can't think of a use case because to me because any use case where you would want to do that is a squash, not an amend. To you, it appears that you don't distinguish, and thus you can see a use case for this.

Maybe this is because when I first switched to jj, my translation was hg fold -> jj squash, and hg amend -> jj amend. My suspicion is that users who've been using jj for longer will be more likely to be tripped up by such a change, but to a new user it's probably quite normal. My opinion is that that's probably a sign that jj users have learnt some bad habits that need to be unlearned because we designed squash this way, rather than that it being a bad idea to separate them though.

Interesting to hear other people's perspectives on this though. To me, the thought of "I need to squish these two commits into one" is quite different from "Update this commit with changes from this other commit", and I think that's probably the right way to think about it, but I'd be open to discussions on this matter if other people think differently.

@joyously
Copy link

joyously commented Jun 3, 2024

To me, the thought of "I need to squish these two commits into one" is quite different from "Update this commit with changes from this other commit", and I think that's probably the right way to think about it, but I'd be open to discussions on this matter if other people think differently.

Both of those sound to me like "merge", and both sound like hacking the DAG to meet an external criteria. Although that is often the case for developers, maybe it shouldn't be the focus of commands. Isn't the goal of jj to manage "changes" versus commits?

@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

Based on discussions both here and in the discord, it sounds like my first option is controversial, and that we should just add an option to the existing squash command to not abandon empty commits.

Both of those sound to me like "merge", and both sound like hacking the DAG to meet an external criteria.

Could you explain this more? This comment confused me. It seems like a very natural thing to want to move files without commit metadata. And it's definitely not a merge (at least in the jj merge command sense, which is why I avoided using the term merge).

Isn't the goal of jj to manage "changes" versus commits?

I agree, which is why I find it very annoying that jj destroys your change metadata and creates a new change when I don't want it to.

@joyously
Copy link

joyously commented Jun 4, 2024

Could you explain this more? This comment confused me. It seems like a very natural thing to want to move files without commit metadata. And it's definitely not a merge (at least in the jj merge command sense, which is why I avoided using the term merge).

Ha ha! Your comment of "squish two into one is different from update one from another" confused me. What I meant was that both are merging or combining changes. I wasn't referring to jj merge because I didn't realize it was a command (I thought it was an alias for jj new). Or maybe you were asking what I meant about the DAG? When you say it's natural to want to move files without commit metadata, that does not compute. Almost all changes are file contents changes, so it seems unnatural to separate the files from the commit metadata. Since you explained your workflow as wanting to keep the placeholder change because it's easier to remember just one instead of many, I can see that one lazy case, but otherwise it seems like micromanagement of the tool instead of letting it do the work for you.
Can you come up with an alternate way for your workflow to be better/easier?

@matts1
Copy link
Contributor Author

matts1 commented Jun 4, 2024

I guess my perspective might be different from yours. I think @mlcui-corp described my thoughts very well in a comment on the discord

I personally use the "move files" part of squash 98% more than the "combine two changes, including description" part of squash - in fact, I can't remember whether I've actually done that.

It personally doesn't feel like a hack to me that I want to avoid moving metadata. The hack to me feels like the fact that we made "combine two changes, including description" the default, when as @mlcui-corp said, 99% of the time I just want to combine the file changes.

I think I can come up with a better example that might help you understand the problem:

  • Suppose I have a commit with description "foo" and also the branch name "foo".
  • Now I run jj amend foo.py
    • If foo.py was one of many files modified in that commit, it preserves the description and the branch name
    • If foo.py was the only file in that commit, it attempts to merge the descriptions, and the branch is moved to the parent commit.

Consider that this is something that I'm likely to do without thinking too much about it. I simply make a change to foo.py, realise I want it to be in the parent, run jj amend foo.py to ensure that if I happened to have modified another file, those changes don't get sent in. What I dislike about this is that the user clearly came with the intent to just move a single file into the parent (they literally said amend foo.py, so they're clearly thinking in terms of files, not commits), but now they may get unintended side-effects depending on how many files the commit contained.

My thoughts is simply that jj needs to be able to understand the intent behind a command, as that will reveal what the correct thing for jj to do is. In the current form:

  • jj amend foo.py or jj amend --interactive clearly signals they want to work on files
    • But depending on the state of the current change, it may work on changes
  • jj squash is a relatively strong signal that the user just wants to squish two commits into one, including description
  • jj amend is a little more vague. It could possibly be argued either way.

My proposal would involve jj being able to distinguish these intents:

  • jj movediff foo.py and jj movediff would both be a clear intent to move file diffs and not commits
  • jj squash would be a clear intent to join two commits into one, including description

@mlcui-corp
Copy link
Contributor

mlcui-corp commented Jun 4, 2024

+1 for @matts1's comments. I think we should get more thoughts on this from the community in general. My thoughts:

Differentiating intent: Agreed that we should differentiate. Using squash with the intent of moving diffs results in confusing scenarios as mentioned above, so we should understand the intent of the user if possible. However, this only applies if the "from" commit has useful commit metadata, or the "from" commit is a merge, which may be rare (but seems like it's common enough to be an issue).

How we differentiate - two commands vs. a new flag: I think two commands would be best. I agree that the current "overloading" of squash isn't great, but unlike git checkout, both "overloads" are very similar. As discussed, the only difference would be the "auto-abandon + move metadata" behaviour, which could be moved into a flag. However, if we have two commands, the CLI interface will be simpler for both:

  • jj squash:
    • Flags: --interactive (and by extension --tool) can be removed.
    • Arguments: [PATHS]... can be removed. As a result, the -r flag be removed too and change to an argument. (edit: nevermind, I assume there is a good reason why jj diffedit does not do this)
    • Help message: all explanations about abandoning (except for the final "if WC is abandoned" paragraph) can be removed.
  • jj movediff (assuming that the name is not final):
    • Flags: --message and --use-destination-message can be removed.
    • Help message: all explanations about description merging abandoning can be removed.

In both, there would be no need to explain "empty commit abandon" logic, as it wouldn't exist. The interface simplicity would also make the commands much more accessible to new jj users IMO.

I think the main issue for splitting the two commands would be breaking the habits of existing jj users. I think that while this is indeed an issue, the interface simplicity - and the ability to provide intent - is worth the habit breakage.

@yuja
Copy link
Contributor

yuja commented Jun 4, 2024

I agree that moving diffs and squashing (or folding) commits are conceptually different, but I'm not sure if adding two separate commands is the right approach. I think the boundary is a bit fuzzy in jj because it's easy to stack up volatile commits (with no description.) I would have to use "squash" to combine them, but I'm only interested in file diffs.

Another possible use case is moving some file diffs and commit message without making a commit empty. If the interactive tool supports selecting a commit description in addition to file diffs, this kind of operation might be useful.

matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 5, 2024
If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 11, 2024
If the user wrote, for example, `jj squash foo.rs`, it'd be a little wierd if we then moved a branch pointer to the parent commit.

Fixes jj-vcs#3815
matts1 added a commit to matts1/jj that referenced this issue Jun 14, 2024
matts1 added a commit to matts1/jj that referenced this issue Jun 14, 2024
matts1 added a commit to matts1/jj that referenced this issue Jul 3, 2024
@matts1 matts1 closed this as completed in ca4eb60 Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants