jj-vcs
Warn/require confirmation on bookmarking empty changes?
Discussed in https://github.com/jj-vcs/jj/discussions/5363
Originally posted by sourcefrog January 14, 2025 In getting to know jj, when I am working on a feature branch that I want to push to github, I've fairly often run a flow like
jj commit -m Whatever
jj bookmark move featurebranch
The trap of course is that the bookmark is set to the new empty revision following the thing that I committed. I suppose pushing the empty rev to a feature branch on github is not too harmful but it does seem a bit messy.
The right answer I guess is that I should either
- Move the bookmark before committing, which seems a bit weird coming from other VCS
bookmark move -r@-- Just
jj descthen bookmark it but then don't forget tojj new
I see there is a good bit of existing discussion about various types of auto-moving bookmarks, which would perhaps be a more fundamental answer for this kind of workflow:
- https://github.com/jj-vcs/jj/discussions/3549
- https://github.com/jj-vcs/jj/discussions/2425
- https://github.com/jj-vcs/jj/discussions/5333
- https://github.com/jj-vcs/jj/issues/3402
- https://github.com/jj-vcs/jj/issues/2338
As a more concrete and tactical (perhaps excessively tactical) fix I wonder if the UI should by default prevent moving a bookmark to an empty change, the same way it by default prevents you pushing new bookmarks and moving bookmarks backwards.
That is, jj new; jj bookmark set bbb would fail unless you said --allow-empty.
Alternatively, rather than failing it could say
warning: bookmark bbb is set to an empty revision
hint: to set it to the parent revision use `jj bookmark move bbb -rrstuvwxyz --allow-backwards`
I agree with @yuja's suggestion here, i.e. that we should just make jj bookmark create/set/move require a revision argument instead of defaulting to @.
I agree with @yuja's suggestion here, i.e. that we should just make
jj bookmark create/set/moverequire a revision argument instead of defaulting to@.
That sounds good to me. There are two practical things to consider:
- Many tests do
jj bookmark create(and probably alsoset/move) without specifying a revision. Those tests need to change. - Some users may actually prefer the current behavior (an implicit
-r@).
Do we want to introduce a new jj config knob(s), say:
bookmark-create.target_revision_defaults_to_working_copy = falsebookmark-set.target_revision_defaults_to_working_copy = falsebookmark-move.target_revision_defaults_to_working_copy = false
That way users (or tests) can set those knows to true to continue working as they do today. I don't have a strong preference, and don't mind fixing every test if necessary.
Good question. We can ask on Discord how people feel about it. I basically never create bookmarks (outside of tests and demos).
Regardless of what we do with the default, I'm against adding configuration for this. It seems like too much UI complexity for minimal gain.
Also regardless of the default, shouldn't we just warn whenever you try to set a bookmark to an empty, single-parent revision?
- It's not clear to me if the proposal is to warn only for the default case, or in all cases.
- Even if done explicitly, it seems likely to be a mistake (one of those things you have to adjust to when coming from Git).
Philosophically, I think we should remove the default, as it seems to derive from the working-copy-oriented branching style, in order to keep commits reachable/visible. If users prefer to bookmark @ by default can't they add an alias for it, anyways?
shouldn't we just warn whenever you try to set a bookmark to an empty, single-parent revision?
I think it's fine to set a bookmark to such commits, kind of like how it's okay to set a description before you write code. By making the revision argument required, we're just making the user think about the target, without saying that it's a bad thing to target an empty commit. But that's just my opinion. Note that I barely use bookmarks myself.
The discussion on Discord seemed a bit inconclusive. It seemed like some users like the current default. Some users would prefer if it was @-. It seemed like no one felt strongly either way. So I'm not sure where that leaves us. Maybe we just leave it as is for now?
I think it's a bit of a trap for new users onboarding to the jj model, especially because putting the bookmark back on @- takes a somewhat long command. Requiring an explicit destination seems coherent with jj requiring explicit instructions for some other cases. But that's just my impression as a new user.
I agree that making target revision explicit is a good change. Specifically, jj bookmark create will return an error if target revision is not explicitly given on the command-line (I will later deal with bookmark set and bookmark move).
I agree with martinvonz that we should allow setting a boomark on an empty commit, but we can revisit that decision. I view that as separate from this open issue.
I think it's a bit of a trap for new users onboarding to the jj model, especially because putting the bookmark back on @- takes a somewhat long command. Requiring an explicit destination seems coherent with jj requiring explicit instructions for some other cases. But that's just my impression as a new user.
+1 for being consistent, i.e. having the commands require explicitly specifying a revision.
As for new users, the warning about moving bookmarks backwards does not make the experience any better, so avoiding that seems like a good idea.
And I think users that are accustomed to the current behaviour are able to work around that with e.g. an alias as recommended above.
As I said above, the conclusion from Discord was unclear. But the recent messages here all seem to be for making the revision argument required :) @jennings, I think you were one of the users on Discord who said you kind of like the current behavior. I know you said you don't feel very strongly at all, so I'm not calling you here to defend your position (but feel free to) :) I Just wanted to make sure it won't be a surprise, given the Discord discussion, if we decide to make the revision argument required.
I'm fine with making the revision argument required.
+1 for being consistent, i.e. having the commands require explicitly specifying a revision.
I'm for being consistent, but I'm not convinced that requiring an explicit revision argument is consistent. Lots of commands currently default to @ (abandon, absorb, describe, diff, diffedit, new, ...).
As for new users, the warning about moving bookmarks backwards does not make the experience any better, so avoiding that seems like a good idea.
I think that's a separate issue. I would like to hear from more users before we make a decision about that. Feel free to open a separate issue for removing that flag (I think you're talking about --allow-backwards).
As for new users, the warning about moving bookmarks backwards does not make the experience any better, so avoiding that seems like a good idea.
I think that's a separate issue. I would like to hear from more users before we make a decision about that. Feel free to open a separate issue for removing that flag (I think you're talking about
--allow-backwards).
You misunderstood me, sorry. I was merely adding to @sourcefrog 's point that the command to move a bookmark backwards is long. Most newcomers will try to do that and then get that "scary" warning about having to use the --allow-backwards option.
As for the discussion on whether or not bookmark commands would need an explicit revision or not:
Maybe the better way out for now is to address the topic of setting bookmarks to empty changes as described in the OP. Maybe introducing a configuration option to have jj allow or not allow setting a bookmark to an empty change (or have jj warn or not warn) would be an easy way out?
the command to move a bookmark backwards is long
In case people don't know, --allow-backwards has a short alias, -B. Currently, the only way to find out about that is to run jj bookmark move --help. Perhaps we should include the short version in the message as well.
The reason it's not there is that it's a bit difficult to insert both options without making the message more confusing, but hopefully we'll think of something. (I think we have to include the long version since it explains what the option does)
Perhaps "Use --allow-backwards (alias -B) to allow it."
I don't think there should be a limit on which revision can be bookmarked.
The problem seems to be that bookmarks are not branches, yet that's what they are used for, so perhaps bridging the gap in the jj git push command is the answer.
Would it make sense to have an option on push to do what a Git user expects (move the bookmark and push what changed)?
Or maybe a revset for changes in the range of bookmark to tip (probably already doable, but I don't know and novices don't know)?
I think you can't get rid of the Git expectations since there is a Git interface, but there should be less friction when using it.
I usually edit commits directly instead of new+squash, and I am usually editing the last commit in a stack anyway when I'm ready to push. So, that's why I use jj b set with no -r flag.
Even though it's ever so slightly inconvenient for my use, it sounds like requiring -r is the right call because:
- It's a possible foot-gun for newcomers.
- Someone using the squash workflow would never want the default, and it seems like we encourage the squash workflow by default.
- Adding
-r@is not a huge burden. - Making it optional again in the future is a non-breaking change.
If users prefer to bookmark @ by default can't they add an alias for it, anyways?
Yes, but I think aliases for sub-commands are a little inconvenient because I have to backtrack my thoughts. This is my thought process step-by-step:
# Time to set a bookmark!
$ jj _
# Bookmark commands are nested under the "bookmark" namespace
$ jj bookmark _
# Oh crap, I have an alias for this
^H^H^H^H^H^H^H^H
$ jj bset my-branch <ENTER>
If I could create an alias nested under the "bookmark" command, then I'd have no issue with it :)
How would this interact with jj b m --from @- ? I have a dual-branch flow (one branch that deploys to a target environment, and the other branch that contains the work being done). Would it still work after this change?
Now you will have to do this:
jj b m --from @- --to@
In short, you will have to specify the target revision in:
jj bookmark create ...jj bookmark move ...jj bookmark set ...
From a feature point of view the only change here is making target revision required, otherwise things work exactly as before.
I don't love "cryptic" single capital letter options, and -B is already used as an alias for --insert-before in many commands so I am not sure advertising it is a good idea... Instead, maybe we could try to avoid the problem in the first place, and make it easier to fix it when it happens:
- We could add an
--allow-emptyflag that must be used to create or move a bookmark into an empty head. - We remove the need to use --allow-backwards when moving a bookmark from an empty head to its parent. I'm usually not a big fan of adding special cases, but I think that this scenario hardly counts as moving backwards....
https://github.com/jj-vcs/jj/pull/5527 is the first step: bookmark create | set | move will display a warning if the user does not specify the target revision explicitly. After a few releases, in jj 0.32+, a follow change will make those fail instead.
Requiring an explicit revision for move/set makes sense to me since those commands can be more ambiguous. However, requiring a revision for bookmark create adds a bit of friction which is imo unnecessary, for the same reason that a revision is not required for things like jj desc or jj squash. When creating a bookmark, defaulting to the current revision makes sense and is how I use this command 99% of the time. Having to append -r@ to each invocation of bookmark create isn't a huge deal, but it is mildly annoying.
Just wanted to throw in my feedback, for whatever it's worth.
I agree. I think requiring to pass a revision to jj bookmark create add unnecessary friction.
+1
I really don't want to have to add a revision. I think bookmarking the current revision is the intuitive and correct default.
Yeah as the person who originally filed the bug leading to this description: I do think it can be a little confusing at first but once you acclimatize to jj, allowing jj b m with no args even if it marks the empty revision is fine.
Here's a discovery I made: If @ is an empty revision that has a bookmark pointing to it, you can run jj squash to move it to its parent. That makes sense and seems like intentional behavior, so it seems fine to rely on it. So, I propose we go back to the idea to print a warning if a bookmark is pointed to and empty @, and the warning suggests to run jj squash.
Here's a discovery I made: If
@is an empty revision that has a bookmark pointing to it, you can runjj squashto move it to its parent. That makes sense and seems like intentional behavior, so it seems fine to rely on it. So, I propose we go back to the idea to print a warning if a bookmark is pointed to and empty@, and the warning suggests to runjj squash.
That sounds like a great idea: doesn't block people who know what they're doing, but less confusing to new users.
Actually that suggestion was dropped from the PR that closed this issue. Turns out jj squash doesn't always work: specifically when moving the bookmark to @ makes @ immutable. In that case, @ will automatically become a new commit on top of the previous @. So, for now, there will just be a warning "Target revision is empty"