`jj rebase` and `jj new`: Allow revsets with multiple commits with new `--allow-large-revsets` argument #1189

ilyagr · 2023-02-04T08:48:08Z

Eventually, we should be able to rely on jj op restore and the ~~--multi~~ (Update: --allow-large-revsets)
argument should likely be removed. This is a temporary measure until we figure
out #922 and the like.

Fixes #571

Checklist

If applicable:

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

src/cli_util.rs

avamsi · 2023-02-04T10:48:22Z

Thoughts on naming this --merge instead? Personally, I think it conveys the intent better and --multi could also be confused for other arguments (-r and -s) (it wasn't clear to me from the PR title, for example).

ilyagr · 2023-02-04T16:42:16Z

The confusion is somewhat intentional in this case. The idea is that --multi should also affect -s/-r/-b once #1158 is fixed. For those, --merge wouldn't work.

I agree this is not ideal, but I couldn't think of anything better. If you prefer to rely on jj undo for mistakes, we could add an option to assume --mutli or you could have an alias.

avamsi · 2023-02-04T17:13:06Z

I guess I'm not sure that revsets to -r/-s/-b should require an explicit confirmation in the form of --multi since rebasing multiple revisions (onto origin/main) is probably one of the most common operations for me (till there's a jj sync anyway) vs merge commits, which I don't do very often.

(That said, you've obviously thought more about this than my pass-by comments and I'd appreciate #1158 being fixed more than not having to pass --multi, so feel free to ignore my anecdote.)

ilyagr · 2023-02-04T19:21:23Z

Yeah, I think that the name --multi is too confusing. I'm thinking of renaming it --allow-large-revsets (with short -L option).

@avamsi You'd still be able to rebase multiple revsets without this option with jj rebase -s A -s B -d C. It just tries to protect you from accidentally doing something like jj rebase -s A: -d C, where a typo or a misunderstanding causes a revset unexpectedly expands to many revisions (the command as written means rebase every single descendant of A to be a direct child of C). So, you'd need to use this option if you did jj rebase -s A+ -d C --allow-large-revsets.

src/cli_util.rs

tests/test_rebase_command.rs

src/cli_util.rs

The revset is already printed in the error message, but it's easy to miss since the error message is in different color.

… general We'll soon need that functionality without the checking for rewriteable part.

This will make deduplication easier in the next commit. The error message becomes slightly less informative, unfortunately.

…`--allow-large-revsets` Eventually, we should be able to rely on `jj op restore` and the `--allow-large-revsets`. argument should likely be removed. This is a temporary measure until we figure out #922 and the like. Fixes #571

…ction This extracts the more general `resolve_mutliple_nonempty_revsets_flag_guarded` out of `resolve_base_revs`. This function should be useful for `rebase -s`, etc. `resolve_base_revs` is renamed to `resolve_destination_revs`; that's simply a better name for it. It is also quite specific to the `new` and `rebase -d` commands. It will be moved out of general utilities in the next commit

It's not very general.

src/commands/mod.rs

src/cli_util.rs

…tiple revisions

ilyagr · 2023-02-06T05:11:19Z

Thank you, Martin, for the review!

ilyagr commented Feb 4, 2023

View reviewed changes

src/cli_util.rs Outdated Show resolved Hide resolved

ilyagr force-pushed the ig/multi branch from e58ad87 to 6e1d58a Compare February 4, 2023 08:56

ilyagr changed the title ~~Ig/multi~~ jj rebase: Allow revsets with multiple commits with new --multi Feb 4, 2023

ilyagr marked this pull request as ready for review February 4, 2023 08:57

ilyagr changed the title ~~jj rebase: Allow revsets with multiple commits with new --multi~~ jj rebase: Allow revsets with multiple commits with new --multi argument Feb 4, 2023

ilyagr force-pushed the ig/multi branch 2 times, most recently from 30da17c to c767368 Compare February 4, 2023 09:02

ilyagr changed the title ~~jj rebase: Allow revsets with multiple commits with new --multi argument~~ jj rebase and jj new: Allow revsets with multiple commits with new --multi argument Feb 4, 2023

ilyagr force-pushed the ig/multi branch from c767368 to 33a8bd6 Compare February 4, 2023 09:05

martinvonz reviewed Feb 4, 2023

View reviewed changes

ilyagr force-pushed the ig/multi branch from 33a8bd6 to 34e8dd8 Compare February 4, 2023 20:08

ilyagr changed the title ~~jj rebase and jj new: Allow revsets with multiple commits with new --multi argument~~ jj rebase and jj new: Allow revsets with multiple commits with new --allow-large-revsets argument Feb 4, 2023

ilyagr force-pushed the ig/multi branch 11 times, most recently from 412724f to 6b7a000 Compare February 5, 2023 00:47

ilyagr added 3 commits February 5, 2023 20:38

cli: Include revset in hint for resolve_single_rev

a94762e

The revset is already printed in the error message, but it's easy to miss since the error message is in different color.

Add test for error on jj rebase -d large_revset + test of repeats

81c01e0

Move resolve_multiple_rewriteable_revsets to cli_utils.rs

439ca0a

ilyagr added 5 commits February 5, 2023 20:38

Refactor and rename rewrite_multiple_rewriteable_revsets to be more…

77811f6

… general We'll soon need that functionality without the checking for rewriteable part.

cli_util: Make resolve_base_revs use IndexSet

07cc850

This will make deduplication easier in the next commit. The error message becomes slightly less informative, unfortunately.

Move resolve_destination_revs into mod.rs

91e9490

It's not very general.

ilyagr force-pushed the ig/multi branch from 6b7a000 to 0cf5279 Compare February 6, 2023 04:39

This was referenced Feb 6, 2023

jj log: option to specify preferred id length #1182

Merged

Add --insert-after and --insert-before to jj new #1172

Merged

martinvonz approved these changes Feb 6, 2023

View reviewed changes

src/commands/mod.rs Show resolved Hide resolved

src/cli_util.rs Outdated Show resolved Hide resolved

Mention --allow-large-revsets in hint when one revset resolves to mul…

89ffa81

…tiple revisions

ilyagr force-pushed the ig/multi branch from 0cf5279 to 89ffa81 Compare February 6, 2023 05:10

ilyagr enabled auto-merge (rebase) February 6, 2023 05:11

ilyagr merged commit 9cc536e into main Feb 6, 2023

ilyagr deleted the ig/multi branch February 6, 2023 05:22

martinvonz mentioned this pull request Sep 10, 2023

cli: hint for conflicted branches and change ids #2244

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`jj rebase` and `jj new`: Allow revsets with multiple commits with new `--allow-large-revsets` argument #1189

`jj rebase` and `jj new`: Allow revsets with multiple commits with new `--allow-large-revsets` argument #1189

ilyagr commented Feb 4, 2023 •

edited

Loading

avamsi commented Feb 4, 2023

ilyagr commented Feb 4, 2023

avamsi commented Feb 4, 2023 •

edited

Loading

ilyagr commented Feb 4, 2023

ilyagr commented Feb 6, 2023

jj rebase and jj new: Allow revsets with multiple commits with new --allow-large-revsets argument #1189

jj rebase and jj new: Allow revsets with multiple commits with new --allow-large-revsets argument #1189

Conversation

ilyagr commented Feb 4, 2023 • edited Loading

Checklist

avamsi commented Feb 4, 2023

ilyagr commented Feb 4, 2023

avamsi commented Feb 4, 2023 • edited Loading

ilyagr commented Feb 4, 2023

ilyagr commented Feb 6, 2023

`jj rebase` and `jj new`: Allow revsets with multiple commits with new `--allow-large-revsets` argument #1189

`jj rebase` and `jj new`: Allow revsets with multiple commits with new `--allow-large-revsets` argument #1189

ilyagr commented Feb 4, 2023 •

edited

Loading

avamsi commented Feb 4, 2023 •

edited

Loading