Make `jj squash --from` accept multiple commits #3276

martinvonz · 2024-03-12T05:53:09Z

Closes #2678

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

cli/src/description_util.rs

cli/src/commands/squash.rs

cli/src/description_util.rs

cli/tests/test_squash_command.rs

I'm going to teach the function to support combining more than two descriptions.

yuja

lgtm, thanks.

cli/src/commands/squash.rs

cli/src/description_util.rs

… sources I plan to teach `jj squash --from` to accept a revset as input.

Now you can do e.g. `jj squash --from 'foo+::' --into foo` to squash a whole series into one commit. It doesn't need to be linear; you can squash a bunch of siblings into another siblings, for example.

joyously · 2024-03-13T14:13:34Z

cli/src/commands/unsquash.rs

@@ -105,8 +105,7 @@ aborted.
    // case).
    if new_parent_tree_id == parent_base_tree.id() {
        tx.mut_repo().record_abandoned_commit(parent.id().clone());
-        let description =
-            combine_messages(tx.base_repo(), parent, &commit, command.settings(), true)?;
+        let description = combine_messages(tx.base_repo(), &[parent], &commit, command.settings())?;


Is unsquash simply split?

No, jj squash -r X moves from X into its parent (by default), while jj unsquash -r X moves from the parent into X.

How does that relate to split?
Or are you saying the squash action is on the graph and not the content?
(It's really difficult to determine what affects just the graph and what affects the content.)

split takes a single commit and splits it in two, while squash and unsquash move changes between existing commits (and it removes commits that became empty).

emesterhazy · 2024-04-02T19:18:28Z

Did you consider using --source and --destination for consistency with jj rebase? For what it's worth, the first time I tied to use jj squash --from I did it wrong because I assumed it would squash everything between the --from revision and @ or --into. I feel like --source would make it a little clearer that I need to write a revset expression if I want to squash multiple commits.

Perhaps we could also improve the --help page. The --from argument is allowed to resolve to multiple revisions, but the --into argument is not. Despite this their documentation is nearly identical.

Zambito1 · 2024-04-02T19:28:17Z

Related to @emesterhazy's comment, I did not find it immediately obvious that --from accepts a revset from its description on jj squash -h:

--from <FROM>          Revision to squash from (default: @)

Maybe describing this as Revset to squash from would be better

Zambito1 · 2024-04-02T19:35:31Z

jj diff also uses --from to essentially mark the start of a revset, reinforcing that expectation for that behavior in squash. --source and --destination would also make more sense to me.

martinvonz · 2024-04-02T22:29:28Z

Did you consider using --source and --destination for consistency with jj rebase?

They mean something different for jj rebase, though. --source includes descendants, and --destination indicates a commit to add new commits on top of. I was worried the difference would make it confusing but maybe it's not.

yuja · 2024-04-03T01:41:11Z

Did you consider using --source and --destination for consistency with jj rebase?

They mean something different for jj rebase, though. --source includes descendants,

I agree about this. squash --from is closer to rebase -r, but squash -r would be more confusing.

and --destination indicates a commit to add new commits on top of.

I'm not too worried about this point, maybe because "squash" implies the destination is mutated.

emesterhazy · 2024-04-03T02:01:33Z

They mean something different for jj rebase

Hmm, I didn't think of that. Personally I think the semantics for squash are clear enough that -s and -d wouldn't be confusing, but I see your point. It does seem a little odd though that --from will happily accept a union of two change IDs but it can't be specified multiple times the way -s can for rebase. This seems like an artificial limitation but maybe there's a case I'm overlooking.

If we keep the current names I think we should clarify the description of --from though, and maybe it should be repeatable.

martinvonz · 2024-04-03T04:20:25Z

Did you consider using --source and --destination for consistency with jj rebase?

I forgot to say that jj move had --from and --to for consistency with jj diff/interdiff. I changed it to --into when copying the functionality to jj squash because I felt like it was clearer about the direction. I think we should also keep this consistency in mind if we decide to rename to -s/-d (i.e., should it be jj diff/interdiff -s/-d?).

Zambito1 · 2024-04-03T13:18:50Z

FWIW

GitHub diff uses: base and compare

GitLab diff uses: source and target

It may be worth considering the terms these and other forges (perhaps even non git ones) use for this. I am particularly fond of the GitLab terms here.

emesterhazy · 2024-04-20T13:00:12Z

I think part of my opposition to --from is that it doesn't behave the way I'd expect. For diff, --from x appears to include the changes from @..x. It's a range and can be read as "all of the commits from @ to x".

But for squash, --from doesn't mean this and only a single revision is squashed.

I would propose the following:

For squash, rename --from to --source/-s and --into to --destination/-d.
For interdiff, rename --from to --source/-s and --into to --destination/-d. Since interdiff is described as temporarily rebasing --from onto --to, it makes sense to use the same flags as rebase.
For diff, keep --from and --to since they do something that seems pretty different than what they do in the other commands (i.e. act on a range of commits) between the arguments.

I think this will be more logically consistent and ergonomic since we'll have short flags -s and -d for squash. Currently I find myself using prev --edit and edit xyx to position @ before squashing instead of typing out --from and --into multiple times.

martinvonz · 2024-04-23T23:39:57Z

I think part of my opposition to --from is that it doesn't behave the way I'd expect. For diff, --from x appears to include the changes from @..x. It's a range and can be read as "all of the commits from @ to x".

We have had similar discussions before. I'm not sure what to search for to find those discussions, unfortunately.

Btw, another example is that diff -r should perhaps be renamed to diff -s too then since it works with the diff compared to the parent. Or maybe not since there's no destination argument?

martinvonz · 2024-04-23T23:41:22Z

Oh, I also meant to say that I don't feel very strongly either way, so don't think that I'm ignoring you because I disagree; I'm just hoping that others will share their thoughts if they feel strongly.

emesterhazy · 2024-04-24T02:27:24Z

Yeah, this is just a proposal and not urgent. There are a few people subscribed to comments on this PR, but if nobody chimes in I can open a PR with these changes to collect comments.

I think that in my opinion -r is fine when there isn't a corresponding destination flag.

yuja · 2024-04-24T03:00:00Z

another example is that diff -r should perhaps be renamed to diff -s too then since it works with the diff compared to the parent.

I think -r is okay here because it specifies a commit to be diffed (just like rebase -r specifies commits to be rebased.) We could rename it to --change (as in hg), but the word "change" is also overloaded.

To me, both --source and --from specify one end of a range or subtree.

martinvonz force-pushed the push-knpoklsnrqtx branch from 6f51621 to 3160cb2 Compare March 12, 2024 06:03

yuja reviewed Mar 12, 2024

View reviewed changes

description_util: make combine_messages() not handle abandoned commit

63b630d

I'm going to teach the function to support combining more than two descriptions.

martinvonz force-pushed the push-knpoklsnrqtx branch 2 times, most recently from 29f8620 to fa5eaa0 Compare March 13, 2024 05:11

yuja approved these changes Mar 13, 2024

View reviewed changes

cli/src/commands/squash.rs Outdated Show resolved Hide resolved

cli/src/description_util.rs Outdated Show resolved Hide resolved

martinvonz added 2 commits March 13, 2024 05:08

description_utils: teach combine_messages() to handle more than two…

86d79b9

… sources I plan to teach `jj squash --from` to accept a revset as input.

squash: accept multiple --from revisions

adb5a9c

Now you can do e.g. `jj squash --from 'foo+::' --into foo` to squash a whole series into one commit. It doesn't need to be linear; you can squash a bunch of siblings into another siblings, for example.

martinvonz force-pushed the push-knpoklsnrqtx branch from fa5eaa0 to adb5a9c Compare March 13, 2024 12:11

martinvonz enabled auto-merge (rebase) March 13, 2024 12:12

martinvonz merged commit 800f0f0 into main Mar 13, 2024
16 checks passed

martinvonz deleted the push-knpoklsnrqtx branch March 13, 2024 12:21

joyously reviewed Mar 13, 2024

View reviewed changes

emesterhazy mentioned this pull request Apr 2, 2024

Add a --use-destination-message option to jj squash #3427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `jj squash --from` accept multiple commits #3276

Make `jj squash --from` accept multiple commits #3276

martinvonz commented Mar 12, 2024 •

edited

Loading

yuja left a comment

joyously Mar 13, 2024

martinvonz Mar 13, 2024

joyously Mar 13, 2024

martinvonz Mar 13, 2024

emesterhazy commented Apr 2, 2024

Zambito1 commented Apr 2, 2024

Zambito1 commented Apr 2, 2024

martinvonz commented Apr 2, 2024

yuja commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

martinvonz commented Apr 3, 2024

Zambito1 commented Apr 3, 2024

emesterhazy commented Apr 20, 2024 •

edited

Loading

martinvonz commented Apr 23, 2024

martinvonz commented Apr 23, 2024

emesterhazy commented Apr 24, 2024

yuja commented Apr 24, 2024

Make jj squash --from accept multiple commits #3276

Make jj squash --from accept multiple commits #3276

Conversation

martinvonz commented Mar 12, 2024 • edited Loading

Checklist

yuja left a comment

Choose a reason for hiding this comment

joyously Mar 13, 2024

Choose a reason for hiding this comment

martinvonz Mar 13, 2024

Choose a reason for hiding this comment

joyously Mar 13, 2024

Choose a reason for hiding this comment

martinvonz Mar 13, 2024

Choose a reason for hiding this comment

emesterhazy commented Apr 2, 2024

Zambito1 commented Apr 2, 2024

Zambito1 commented Apr 2, 2024

martinvonz commented Apr 2, 2024

yuja commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

martinvonz commented Apr 3, 2024

Zambito1 commented Apr 3, 2024

emesterhazy commented Apr 20, 2024 • edited Loading

martinvonz commented Apr 23, 2024

martinvonz commented Apr 23, 2024

emesterhazy commented Apr 24, 2024

yuja commented Apr 24, 2024

Make `jj squash --from` accept multiple commits #3276

Make `jj squash --from` accept multiple commits #3276

martinvonz commented Mar 12, 2024 •

edited

Loading

emesterhazy commented Apr 20, 2024 •

edited

Loading