Rebasing (and some other operations) is confusing for hidden commits

[ilyagr]

It is confusing what should happen when jj rebase-ing a hidden commit. (Optional TODO: Investigate what exactly does happen currently. My testing so far is sufficient only to say "weird things happen". In fact, this might be a good way to stress-test jj's rebasing infra and unearth some bugs).

With the current behavior as I understand it, jj rebase -r hidden_commit should create a visible commit. This will create divergence if there is already a visible commit with the same change id.

jj rebase -s hidden_commit is even more confusing. Currently, jj rebase -s visible_commit only rebases visible descendants of visible_commit. hidden_commit will never have visible descendants. So, either jj rebase -s hidden_commit will be pretty useless or it will be fundamentally different from jj rebase -s visible_commit.

jj rebase -b is at least as confusing as jj rebase -s.

Proposal

Here is one proposal on how this could be resolved (alternative proposals are welcome):

• Forbid rebasing hidden commits.
• Suggest using jj duplicate instead of jj rebase -r hidden_commits
• Implement FR: jj duplicate -d #3518 as well as jj duplicate --before/--after from FR: Add --insert-before and --insert-after to split, new, duplicate, etc. #3772, and most other rebase options.
• Do not implement jj duplicate -s or jj duplicate -b for hidden commits unless we figure out a reasonable behavior and a need for it. jj duplicate -s visible_commit may or may not be worth doing, that's outside the scope of this issue.

Open questions for this proposal:

• What other commands does this affect? I think the situation with jj simplify-parents from simplify-parents: add a command to remove redundant parents #4492 is similar.
• Should check_rewriteable return false for hidden commits? That would be one possible answer to the previous question, though there may need to be exceptions.

[ilyagr]

Actually, my confusion about jj rebase -s hidden_commit is related to my confusion about what hidden_commit:: should even mean. (As before, the inconsistency is that visible_commit:: does not include hidden descendants, I'm pretty sure, while hidden_commit::another_hidden_commit should ideally work). Should we have two operations, one that includes hidden descendants of a commit and one that doesn't?

Conversely, if we are OK with this kind of inconsistency in behavior, there is no problem with having jj duplicate -s that behaves the same way.

[yuja]

Forbid rebasing hidden commits.

Sounds good to me. I'm not pretty sure if rebase -r <hidden> should be banned, but we can at least issue a warning. rebase -s/-b <hidden> wouldn't probably make sense.

the inconsistency is that visible_commit:: does not include hidden descendants, I'm pretty sure, while hidden_commit::another_hidden_commit should ideally work). Should we have two operations, one that includes hidden descendants of a commit and one that doesn't?

I don't think <root>::<all_historical_visible_heads> is useful. The user would probably want <root>::<visible_heads_at_a_certain_operation_where_root_was_visible>, which can't be easily deduced.

[martinvonz]

#555 about reporting new divergent changes is also relevant. Such a warning might be sufficient in many cases.

[ilyagr]

I don't think <root>::<all_historical_visible_heads> is useful. The user would probably want <root>::<visible_heads_at_a_certain_operation_where_root_was_visible>, which can't be easily deduced.

IIUC, you are saying that A::<all_historical_visible_heads> is not super-useful as an interpretation of A::. I agree. However, it's the only interpretation that's consistent with A::B working for hidden A and/or B.

Your second point, IIUC, is that A::<visible_heads_at_a_certain_operation_where_root_was_visible> would be more useful, but is not well defined since its meaning depends on which operation we pick.

Additionally, this might be obvious, but I'll say explicitly that A::B has a clear meaning regardless of whether A or B are hidden. It is not consistent with our current meaning of A::, though.

I'm not sure what, if anything, to do about this. It could be argued that the inconsistency doesn't practically confuse many people.

[ilyagr]

#555 about reporting new divergent changes is also relevant. Such a warning might be sufficient in many cases.

It would certainly help.

There is an additional issue with rebase -r A that I forgot to mention before: normally, it also rebases the (visible) children of A. This doesn't have a good analogue if A is hidden. This makes me more convinced that rebase -r hidden should be banned. I'm still unclear about rebase -s.

Update: Speaking more positively, duplicate -r A -d B would be more useful for a hidden A than jj duplicate -r A, especially if A's parent is also hidden. So, I think adding features to jj duplicate is the first step.

[arxanas]

However, it's the only interpretation that's consistent with A::B working for hidden A and/or B.

[not recommending this] One consistent solution is that rebase should always rebase all commits, but commits that were hidden before the operation should remain hidden after the operation. Generalizing, A:: always expands to all descendants (both visible and hidden), but operations preserve visibility by default. (Aside: the meaning of all: might (?) change somehow.)

Then jj rebase -r A or jj rebase -s A with hidden A is meaningful, with the commits being rebased successfully but the new commits remaining hidden. It's nice in a theoretical sense: you could in principle commute the operations of rebasing and unhiding A, which you can't do now (?).

I thought I'd mention it for discussion purposes since it's theoretically nice, but in practice I don't think it's workable —

• There's obvious UX problems when people start unintentionally operating on hidden commits.
  • Can partially address with warnings regardless of the ultimate approach.
• It might be generally inefficient to always query all hidden commits vs keeping only live commits for easy access
• I think it would be easy to somehow get into a quadratic situation where you're repeatedly operating on and growing the same set of hidden commits, causing performance problems.

---

Does it work to just restrict revsets to only ever evaluating to visible commits (and warn for certain cases, like where the entire revset value to an operation is hidden commits, or when A::B includes hidden commits), then toggle the behavior with a hidden: modifier?

• In this case, jj rebase and jj duplicate would function somewhat similarly when the original commits are hidden, since the main logical difference is that rebase hides the original commits, but duplicate preserves them, but it might be fine.
• Note that in both cases, the resulting commits are visible, unlike the above suggestion where visibility is preserved as part of the operation.

Does Mercurial do anything instructive when it comes to addressing hidden/obsolete commits? git-branchless has what's effectively a hidden modifier (via a --hidden flag); I thought that might have come from Mercurial, but I don't recall.

[yuja]

IIUC, you are saying that A::<all_historical_visible_heads> is not super-useful as an interpretation of A::. I agree. However, it's the only interpretation that's consistent with A::B working for hidden A and/or B.

It's at least defined consistently, A:: is a short for A::visible_heads(). If A:: contained hidden descendants, the result would be a mess. If A:: changed behavior depending on whether A is hidden, it would be super confusing (and useless.)

One thing we could add (and is potentially useful I think) is at_op(<op-id-or-range>, A::) to evaluate A:: in historical view?

Does it work to just restrict revsets to only ever evaluating to visible commits

Mercurial works that way by default (by design.) However, it was inconvenient and they later added experimental.directaccess=True to resolve hidden ids without specifying --hidden flag.

[ilyagr]

It's at least defined consistently, A:: is a short for A::visible_heads().

I see, this makes a lot of sense, and I didn't realize that before (well, forgot more likely). Thanks for pointing it out! I think things are clearer for me now.

Interestingly (and somewhat counterintuitively, but I think correctly) this means that jj log -r hidden_commit:: returns no results. This seems consistent with jj rebase -s hidden_commit being a no-op, so in my mind is another point for forbidding that. I'm now even more doubtful that jj rebase -b hidden_commit could make any sense.

---

It might make sense to implement A::heads_at_op(op) and/or A::all_heads(). This might be a bit simpler than implemented a full at_op(...) operation (which I agree we still want for all kinds of reasons, #1283), and would be nice for jj duplicate. If we jump straight to at_op(...), these would just be aliases.

I see your point that A::all_heads() is not always super-useful (and might not be deserving of a special syntax). I think it should actually ideally be spelled as A::heads_at_op(::), with heads_at_op taking an opset. An alias like all_heads() could also exist, or not. It could be useful if you can't easily find the op id you want. Conceptually, I also miss it when I read the definition of A:: in https://martinvonz.github.io/jj/prerelease/revsets/; I wouldn't mind having it just for completeness and to point out that A::all_heads() is different from A::.

he entire revset value to an operation is hidden commits, or when A::B includes hidden commits), then toggle the behavior with a hidden: modifier?

For @arxanas 's point, if we did want to have two or more different behaviors for ::, hidden: would make sense, but it seems that we already have something better implemented. Following Yuya's point, at_op(...): might often be more useful than hidden:. Also, hidden: could maybe be written as at_op(::). In any case, I think we should think of at_op(...): and hidden: as closely related features.

at_op(op):revset seems like it'd be easier to implement than a function at_op(op, revset), but the latter is more powerful. It'd also be nice to have a better syntax for them.

[arxanas]

at_op(op):revset seems like it'd be easier to implement than a function at_op(op, revset), but the latter is more powerful. It'd also be nice to have a better syntax for them.

Why is the latter more powerful?

[yuja]

at_op(op):revset seems like it'd be easier to implement than a function at_op(op, revset), but the latter is more powerful. It'd also be nice to have a better syntax for them.

Why is the latter more powerful?

I think it's syntactic issue. <modifier>: <expr..> applies globally, and <kind>:<pattern> can't take rhs expression.

BTW, at_op(single_op, revset) is relatively easy to implement. Symbols, refs, and visible_heads() are resolved by the revset frontend, and the repo view would only matter there.

[ilyagr]

at_op(op):revset seems like it'd be easier to implement than a function at_op(op, revset), but the latter is more powerful. It'd also be nice to have a better syntax for them.

Why is the latter more powerful?

I meant that you could combine both expressions for the current operation and expressions for an old operation in the same revset if it was a function.