-
Notifications
You must be signed in to change notification settings - Fork 751
prev/next: add --rebase option #7687
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
1eec0ac
to
ea04ae3
Compare
cli/src/commands/next.rs
Outdated
/// Keep the changes when moving to the next revision | ||
#[arg(long, short, conflicts_with = "edit")] | ||
keep: bool, |
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: maybe better to say that the working-copy changes are rebased? "keep" can also mean the opposite thing.
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.
I tried to avoid --rebase
, because the short option, -r
, would look too much like -r
for --revision(s)
in the other command for my taste. --rebase
is a better name though.
Maybe -R
for the short option? Or do I worry for nothing for -r
?
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.
@PhilipMetzger do you have an opinion on what would be the best option name? --keep
? --rebase
? Something else?
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.
I called it keep because Martin called it like that.
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.
I'm not attached to --keep
if we have a better name. I like --rebase
better but I agree that -r
is not ideal. Maybe we don't need a short form, at least to start with? Maybe it's not so bad with --r<TAB>
.
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.
Something like
--carry
/bring
/drag
?
They're just synonyms for --keep
which was the initially proposed name. So I agree that they don't make more sense than --rebase
though.
Even if we go with
--keep
, I think the help text should mention that the working-copy revision is rebased, not kept in the original position.
👍
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.
I've been thinking a bit about that, and I don't think --revision
would make sense for these commands, and -r
here is a flag—we don't pass a value—so there is much less chance to get confused.
I think I would go with --rebase/-r
and see how users react to that.
Maybe mark the flag as experimental so we can rename it easily in case of negative feedback?
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.
Can we try that without the short -r
flag? It's unlikely we'll add -r
/--revision
to these commands, but the existence of -r
can provide false impression that next
/prev
would bring you to the nearest revision within the range, e.g. jj prev -rmutable()
.
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.
I think a short option is quite valuable here, but if we can't find anything better, sure, we can do that
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.
My vote is to keep the short option just -k
so we don't spread more confusion, if you want it so much.
54acbd8
to
119681e
Compare
…s children We shouldn't be in that case, and it can lead to confusing situations with next completely switching branch. fix: jj-vcs#7046
This allows to rebase unfinished working copies when moving through history. fix: jj-vcs#3665
119681e
to
c2ad24a
Compare
fix: #3665
based on #7690
Checklist
If applicable:
CHANGELOG.md
README.md
,docs/
,demos/
)cli/src/config-schema.json
)