jj rebase output inconsistent in no-op cases

[tim-janik]

Description

The jj rebase behavior in jj-0.23 in the following no-op cases is inconsistent.
Suppose @ has some children, then:

$ jj rebase -b @ -d @-
Skipped rebase of 8 commits that were already in place
Nothing changed.
$ jj rebase -b @ -d @+
$

In the latter invocation, it should also print "Nothing changed."
Things are less obvious for the observer when @+ is given as a change_id, in that case "Nothing changed." is quite helpful, I am not sure if "Skipped rebase of commits that were already in place" would applicable in that case as well.

Steps to Reproduce the Problem

• Issue jj rebase -b @ -d @+ so that destination is any @:: descendant.
• Or issue jj rebase -b @ -d @

Expected Behavior

Output: Nothing changed.

Actual Behavior

No output.

Specifications

• Platform: Linux
• Version: 0.23.0

[martinvonz]

The difference between the two cases is that jj rebase -d @- has at least one commit in the set of commits to rebase, but jj rebase -d @+ has no commits since there are no commits in the branch from ::@+ to @ (@+..@ is empty).

[martinvonz]

Related to this, what do people think about making -r, -s, -b accept multiple revisions without requiring the all: prefix? Currently -s and -b require all:. Because they expect exactly one commit, they also produce an error if the input set is empty (that's how the question is related to this bug).

[martinvonz]

See main...push-rqrmtmwnrxyy for what it will look like

[yuja]

what do people think about making -r, -s, -b accept multiple revisions without requiring the all: prefix? Currently -s and -b require all:.

Ilyagr pointed out -s base can be a bit confusing if base != roots(base). I don't have a strong feeling about that, but that's a good point.
#4492 (comment)

[martinvonz]

Good point. We could make it print a warning if base != roots(base).

[pelme]

Related to this, what do people think about making -r, -s, -b accept multiple revisions without requiring the all: prefix? Currently -s and -b require all:. Because they expect exactly one commit, they also produce an error if the input set is empty (that's how the question is related to this bug).

I would prefer not having to add all:. I use jj rebase -b 'all:mutable() & mine()' -d main@origin --skip-emptied a lot to rebase all my branches on top of main in one go. Would be more straightforward without all:. I am not sure why all: makes it safer? The output says "Rebased 3 commits onto destination". If that was not intended, there is always jj undo 😻. I have not much more context than that and am new to using jj and are happy with having all: too.

[martinvonz]

The all: prefix is mostly for things like jj new main and jj rebase -d main where you might be surprised if main resolves to multiple commits (it can if the bookmark itself has a conflict).

Perhaps we could remove all: completely and print warnings in those cases instead.

[thoughtpolice]

You can also disable the requirement for all: by setting ui.allow-large-revsets = true.

[pelme]

To me, it was not super intuitive that all: is not really part of the revset language but a way to avoid the warning. The docs are clear about it but it was another thing that I had to grok when trying to grasp revsets for the first time. Getting rid of all: and printing a warning sounds great to me. If git rebase theoretically could rebase multiple branches in one go, you would be extremely brave to try it because of the conflicts and not being able to easily undo. Again, we are blessed with jj undo which is so amazing and makes it possible to just go back if something is not working as you expected... so not much harm done.

Did not know about ui.allow-large-revsets, thanks for the pointer! Will consider adding it. Am helping my colleagues to learn jj and we share commands all the time (including the command above). Would be nice to avoid having to talk about all: when there are so much other exciting jj stuff to talk about. 😆 (Again, this is not a huge issue to me, this is just my point of view. git user for 15+ years and jj for a couple of weeks 😆 )