commands: Implement next and prev

[PhilipMetzger]

This is a naive implementation, which cannot deal with multiple children or parents stemming from merges.

For now I'll create a follow-up bug, which someone else can solve.

Note: I currently gave each command separate a separate argument struct for extensibility.

Fixes #878

Checklist

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

[PhilipMetzger]

Sorry that I haven't progressed that much. While I have some local changes, I'm also in the progress of moving to neovim as my main $EDITOR for non-work concerned things, which hampers my process. Hopefully after finishing the base of next and prev I have the time for run as described in the Design Doc.

[martinvonz]

Sorry that I haven't progressed that much.

No worries! Take your time.

[PhilipMetzger]

PTAL, @martinvonz if I'm on the correct path.

[martinvonz]

PTAL, @martinvonz if I'm on the correct path.

Yes, looks like it. Do you need help with anything or is it going pretty well?

[PhilipMetzger]

This is almost ready, only the tests are missing now. Here's a screenshot with the Arcanist repo as an example:

I'll need some help to turn the jj next/prev --edit true into just jj next 10 --edit though.

[martinvonz]

I'll need some help to turn the jj next/prev --edit true into just jj next 10 --edit though.

Try replacing Option<bool> by just bool (it still won't mean that the user has to pass the flag).

[PhilipMetzger]

This is now ready for review. A testcase is missing and four are failing but can be fixed during the review.

[yuja]

(Github somehow failed to load the review UI, so commented in old style.)

// Get the N'th child.
for _ in 0..amount {
    // Does the revset optimizer turn this into a `descendants(id)`?
    children_revset = children_revset.children();
}

Perhaps, you can build a single query like this.

    let current = RevsetExpression::commit(current_wc_id.clone());
    let next = current.parents().children().minus(&current); // @-+ & ~@
    Rc::new(RevsetExpression::Descendants {
        roots: next,
        generation: (amount - 1)..amount,
    });

We'll probably need a helper method for the last part. I can't think of a good name, but maybe .descendants_at(n) or .descendants_within(range)?

/// Instead of moving the empty commit from `jj new`, edit the child
/// revision directly. This mirrors the behavior of Mercurial and
/// Sapling.

I think Mercurial's behavior is not --edit. JJ's graph is close to -r 'wdir() + all()' in Mercurial.

FWIW, the query @-+ & ~@ implies that we're not "editing" the @ revision. If we're editing, the next revision would be just @+. I'm not sure if --edit should affect the handling of the current revision.

[PhilipMetzger]

Perhaps, you can build a single query like this.

    let current = RevsetExpression::commit(current_wc_id.clone());
    let next = current.parents().children().minus(&current); // @-+ & ~@
    Rc::new(RevsetExpression::Descendants {
        roots: next,
        generation: (amount - 1)..amount,
    });

We'll probably need a helper method for the last part. I can't think of a good name, but maybe .descendants_at(n) or .descendants_within(range)?

Thanks, this suggestion is simpler than my previous attempts. My initial attempt is still available as commented code, e.g a direct lookup via RevsetExpression::descendants(id), this implementation is based on this comment from martin in Discord.

/// Instead of moving the empty commit from `jj new`, edit the child
/// revision directly. This mirrors the behavior of Mercurial and
/// Sapling.

I think Mercurial's behavior is not --edit. JJ's graph is close to -r 'wdir() + all()' in Mercurial.

FWIW, the query @-+ & ~@ implies that we're not "editing" the @ revision. If we're editing, the next revision would be just @+. I'm not sure if --edit should affect the handling of the current revision.

Thanks for the clarification. What would be a more appropriate description? This is purely based on my assumptions from sl next.

[yuja]

I think Mercurial's behavior is not --edit. JJ's graph is close to -r 'wdir() + all()' in Mercurial.
FWIW, the query @-+ & ~@ implies that we're not "editing" the @ revision. If we're editing, the next revision would be just @+. I'm not sure if --edit should affect the handling of the current revision.

Thanks for the clarification. What would be a more appropriate description? This is purely based on my assumptions from sl next.

My feeling is that jj edit is close to hg update && hg uncommit (without losing commit metadata.)
I don't know if people would want to use --edit while walking stacked commits by jj next/prev, but maybe I'm biased. I rarely use jj edit.

[necauqua]

From my perspective (I did not look too deep into this), for jj next/prev it would not be too useful if it did edits, it would be better if it did "checkouts" aka making/dropping empty working copy children

And something like jj next/prev --edit is not needed bc that's just jj edit @+/@-

[PhilipMetzger]

I don't know if people would want to use --edit while walking stacked commits by jj next/prev, but maybe I'm biased. I rarely use jj edit.

I'm one of the people which generally use edit instead of the new + squash workflow. And @talpr also has jj next aliased to edit @+. Which means that we generally should support both workflows.

And something like jj next/prev --edit is not needed bc that's just jj edit @+/@-

I respectfully disagree, see the initial conversation with Martin on Discord here.

[martinvonz]

FYI, 2164b7e somehow fails to load for me, so I can't review this.

I agree that this comment is misleading:

/// Instead of moving the empty commit from `jj new`, edit the child
/// revision directly. This mirrors the behavior of Mercurial and
/// Sapling.

Maybe something like this?

/// Instead of creating a new working-copy commit on top of the
/// target commit (like `jj new`), edit the target commit directly
/// (like `jj edit`).

[martinvonz]

Is this PR ready to be merged?

[PhilipMetzger]

Sorry for no activity here, as I had some personal obligations. I'm fine to commit as is and add the next creating a commit part later.

[martinvonz]

Sorry for no activity here, as I had some personal obligations. I'm fine to commit as is and add the next creating a commit part later.

Sounds good! Do you have time to rebase it so it can be merged?

[PhilipMetzger]

Sounds good! Do you have time to rebase it so it can be merged?

Yeah, I'll do it after creating a issue with the missing feature and splitting my local changes into a later commit.

[PhilipMetzger]

Should be good now.

[martinvonz]

Thanks! Looking forward to using this.