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

refactor: remove 'jj checkout' command #1713

Closed
wants to merge 1 commit into from

Conversation

thoughtpolice
Copy link
Collaborator

Some discussion on Discord basically lead to Martin deciding this would be OK; users should generally use jj new instead of checkout, which is not only more general (it can take more than one revset, in order to perform a merge) -- it's also not trying to be something it isn't.

The idea was, people will try to do checkout and might feel strange if it doesn't work. First impressions matter. But on the other hand, git checkout is a particular colloquialism that even Git itself seems to be (slowly) moving away from with git branch and the experimental git switch. Just trying to have a familiar name isn't enough if the mechanics aren't the same, and may lead to more confusion due to behavioral changes.

This can of course always be revisited later if people really do clamour for that wonderful checkout command, but in such a case, it's probably better as an alias of new anyway, rather than having similar-but-more-restricted semantics.

Change-Id: Iwmotxnmmopxptrvzqzpsytvkyvwlrrll

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have added tests to cover my changes

Some discussion on Discord basically lead to Martin deciding this would be OK;
users should generally use `jj new` instead of checkout, which is not only more
general (it can take more than one revset, in order to perform a merge) -- it's
also not trying to be something it isn't.

The idea was, people will try to do `checkout` and might feel strange if it
doesn't work. First impressions matter. But on the other hand, `git checkout` is
a particular colloquialism that even Git itself seems to be (slowly) moving away
from with `git branch` and the experimental `git switch`. Just trying to have
a familiar name isn't enough if the mechanics aren't the same, and may lead to
*more* confusion due to behavioral changes.

This can of course always be revisited later if people really *do* clamour for
that wonderful `checkout` command, but in such a case, it's *probably* better
as an alias of `new` anyway, rather than having similar-but-more-restricted
semantics.

Signed-off-by: Austin Seipp <[email protected]>
Change-Id: Iwmotxnmmopxptrvzqzpsytvkyvwlrrll
@thoughtpolice
Copy link
Collaborator Author

I also got the tests and examples changed, but might have missed something here or there.

@GaryBoone
Copy link

GaryBoone commented Jun 19, 2023

Instead of removing the command entirely, could we leave a redirect that says, ~"Use jj new instead"? It would gently ease people trained in git co into the new command and reduce confusion.

Ideally, for those transitioning from git, retaining as much git motor memory is a huge ergonomic advantage.

@PhilipMetzger
Copy link
Collaborator

This looks fine. But it'd be better if we left jj checkout/co as an alias in for atleast the next release, before actually removing it. The last time we had such a breakage #1570 popped up. And the same issue appeared when jj log -T show was renamed.

@thoughtpolice
Copy link
Collaborator Author

thoughtpolice commented Jun 19, 2023

How about:

  • We make it an alias for jj new for the next release, and also say it's deprecated and will later become a hard error. This would be 0.8 I suppose
  • After that, we can remove it, but add an error saying "You should use jj new instead." This would be 0.9
  • After that, delete all vestiges and remove the hard error too in 0.10, etc

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 19, 2023

jj co is also shorter than jj new. I suppose we could alias n to new. We can also leave it for people to define a custom alias if they want, but I like using jj co in tests. I'm not sure to what degree 1 letter matters, but it seems to be the reason I usually type jj co in my workflow.

I'd also be OK with keeping jj checkout & jj co as aliases long-term.

@thoughtpolice
Copy link
Collaborator Author

FWIW, on the note of shorthands, I'd be a big fan of more one-letter shorthands by default, but I know people have wildly varying opinions on that. At the very least I have d (diff), l (enhanced log), and will probably add n as well.

@necauqua
Copy link
Collaborator

necauqua commented Jun 19, 2023

When I was playing with the idea of making an intellij plugin for jj I found deeper meaning in the new command, which was possibly previously slightly obscured by the existence of checkout.
So I 100% support the idea of pushing towards always using new as the semantic is different - we never "check out" a commit in jj, we create a new working commit on top of it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, please limit the reformatting to the paragraphs you actually changed. If you'd like to reformat the whole file, that can be a separate commit.

In this case, I'm not sure the reformatting is for the better, but I only glanced at it; feel free to have a reformatting commit if you feel it is for the better.

If this is difficult, we can leave it be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, I thought I avoided this but apparently not. It was an accident in vscode and I guess my changes got slurped up. I'll back this out and fix it.

@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re the commit's and PR title: I wouldn't call this a "refactor". I'd put "cli" instead (not sure if other people have better thoughts).

@martinvonz
Copy link
Member

How about:

  • We make it an alias for jj new for the next release, and also say it's deprecated and will later become a hard error. This would be 0.8 I suppose
  • After that, we can remove it, but add an error saying "You should use jj new instead." This would be 0.9
  • After that, delete all vestiges and remove the hard error too in 0.10, etc

That plan sounds good to me.

@GaryBoone
Copy link

We make it an alias for jj new for the next release, and also say it's deprecated and will later become a hard error. This would be 0.8 I suppose

To be explicit, this first version will also include the message to prefer jj new, yes?

@necauqua
Copy link
Collaborator

necauqua commented Jun 21, 2023

delete all vestiges and remove the hard error

The final version could be more welcoming to git users by giving a hint if they type jj checkout:
"You probably wanted to run jj new [arg] to create a new child commit and edit it, which is more or less equivalent to checkout in git (todo better wording)"

And this never goes away (unless git completely gets replaced by jj everywhere, then sure)


I found deeper meaning in the new command

Should we do the same thing with jj merge?

I feel like in git there is this strong connection between "merging" and "fixing merge conflicts", and git merge is a whole process, but jj merge just creates a child of multiple commits and that's it, period - you then edit it additionally to fix conflicts if any, or even leave the conflicted merge commit there forever and fix the conflicts in it's child(ren)

So jj merge is just another alias of jj new with some very slight difference I don't even remember off the top of my head

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 21, 2023

Should we do the same thing with jj merge?

I'm mildly opposed to removing jj merge. I think people will be looking for it and its effortlessness is a good jj advertisement.

I wouldn't mind making it look like an actual alias to jj new, though that also brings some second thoughts. Currently, it checks that it's given at least two changes and throws an error otherwise. In my ideal world, it would look like an alias to new in the help, and would only print a warning when it sees one revision. Though, given the limitations of clap, that ideal world could be hard to achieve.

@martinvonz
Copy link
Member

I'm mildly opposed to removing jj merge. I think people will be looking for it and its effortlessness is a good jj advertisement.

Do you feel the same about removing jj checkout? I do.

I think my preference is to make both jj checkout and jj merge print a warning and then delegate to jj new. I don't care much if they keep their current validation of 1/more-than-1 arguments if we have that warning. Then we can make them no longer delegate and just print warning/error in a later version. I think we can keep the commands around for a long time before we remove them.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 21, 2023

Do you feel the same about removing jj checkout? I do.

I think the case for removing checkout being didactic to users is a bit stronger than with merge.

Personally, I'm not sure how objective this feeling is (I'm not opposed to any outcome here, and my feelings may change), but I currently don't really want to remove them in a later version (especially if they are aliases) or print a warning without a good reason (e.g. if merge is called with one argument). I feel a bit stronger about merge.

I might be a bit biased by using jj co often and its symmetry with jj ci. But it's not a big deal; I can always create whatever interface I want with my own aliases.

@thoughtpolice
Copy link
Collaborator Author

thoughtpolice commented Jun 21, 2023

We make it an alias for jj new for the next release, and also say it's deprecated and will later become a hard error. This would be 0.8 I suppose

To be explicit, this first version will also include the message to prefer jj new, yes?

Yep, that's what I was imagining, more or less.

"You probably wanted to run jj new [arg] to create a new child commit and edit it, which is more or less equivalent to checkout in git (todo better wording)"

And this never goes away (unless git completely gets replaced by jj everywhere, then sure)

Yeah, and I think that's actually a good idea. Generally speaking, I think jj is early on enough in its life (and in the 0.x series) that removing some stuff like this is good, healthy, and should be done early, so you don't have to maintain semi-similar commands with minor differences forever. It seems small now but a lot can happen in the long run. Forwarding commands and stuff is mostly fine, probably, but minor semantic differences can get hairy in the long run. But on the other hand, just printing a command that says "Please use this other command" is like, 5 lines of code and probably not a big deal? You can keep that forever, yeah.

So really the motivation is just "Do it now, before you can never, ever do it," which in my (FOSS, mostly) experience is a point that comes along and hits you much like a truck, at high speeds and quite unexpectedly.

It probably would have been better for me to lead with that as more of a motivation, admittedly.

I think my preference is to make both jj checkout and jj merge print a warning and then delegate to jj new. I don't care much if they keep their current validation of 1/more-than-1 arguments if we have that warning. Then we can make them no longer delegate and just print warning/error in a later version. I think we can keep the commands around for a long time before we remove them.

Yeah, I hadn't thought about merge actually, but it's a good point... There's a tension between basically having too many "verbs" (Git has an astronomical amount of them), but also not making people uncomfortable by lacking the verbs they expect...

In light of this, I'm going to close this PR for now. It's clear we don't want to delete this code just yet, but at the same time there seems to be some consensus that maybe "thinning the verbs" is worth thinking about. I think starting a discussion in the discussion forums with a little wider context might be more useful; we can start with jj checkout and jj merge as a jumping off point.

@necauqua
Copy link
Collaborator

necauqua commented Jun 23, 2023

(sorry for giving my last msg here I guess)

IMO the best outcome would be for the mature version of jj to not have functional checkout or merge verbs, but when they are called it gives those learning errors: "in jj the merge is just creating a child of multiple parents, so use jj new" and "in jj checkout is just creating a child and editing it, so use jj new"

And there could be a known set of aliases you could easily add to your config to make jj way more git-like if you prefer, and it's mentioned in learning resources etc

Multiple verbs that do essentially the same thing must be one actual command and learning-warnings/aliases/not-being-there-at-all

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 23, 2023

Well said, put that way it sounds pretty compelling. If jj config set worked as described below (update: when I originally wrote this, I thought we don't have jj config set, but I'm quite behind the times: it was implemented in #1312; I'm not sure I got the syntax right), the error could say something like

jj uses `jj new` instead of `git checkout`.
If you would like to be able to use `jj checkout` or `jj ci`,
you can create such aliases by running:
    jj config set --global 'alias.checkout' new
    jj config set --global 'alias.ci' new

It would be pretty easy to have a hidden clap subcommand that prints such messages.

I am not sure this is the one true way to go, but it seems like a sensible option.

(Update: I didn't know jj config set existed when I first wrote the following paragraph, but I think the problem with aliases being string lists does exist)

Also, this fantasy jj config set is not completely obviously the right one, as in the present day, configs like alias.checkout usually take TOML lists. We could make it take a space-separated string as well (or maybe it does already), but that has its own problems: there is either no way to escape a space or we'd have to set up some escaping rules which will interfere with the shell's escaping rules.

Actually, jj config set alias.ci --list new might work well.

@thoughtpolice thoughtpolice deleted the push-wmotxnmmopxp branch July 15, 2024 23:20
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 this pull request may close these issues.

6 participants