Fix rebase -r of a parent of a merge commit

[ilyagr]

Comments

(UPDATE: I deleted some no-longer relevant info from this comment)

This is a fix to a bug in rebase -r described below. It also changes the behavior of another rebase test in a way that seems desired to me.

I'm not friends with Rust yet, so I'm sure the style is not great.

Feel free to just use the test I wrote in here if you prefer a different fix.

PR Description

Previously, using rebase -r on the parent of a merge commit
turned it into a non-merge commit. In other words, starting
with

    o   d
    |\
    o | c
    o | b
    | o a
    |/  
    o 

and doing rebase -r c -d a resulted in

    o d
    o b
    | o c
    | o a
    |/  
    o 

where d is no longer a merge commit.

For reference, here's a complete test that passed before this commit (but should NOT pass; see the diff for a test that should pass):

#[test]
fn test_rebase_single_revision_merge_parent() {
    let test_env = TestEnvironment::default();
    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
    let repo_path = test_env.env_root().join("repo");

    create_commit(&test_env, &repo_path, "a", &[]);
    create_commit(&test_env, &repo_path, "b", &[]);
    create_commit(&test_env, &repo_path, "c", &["b"]);
    create_commit(&test_env, &repo_path, "d", &["a", "c"]);
    // Test the setup
    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
    @
    o   d
    |\
    o | c
    o | b
    | o a
    |/
    o
    "###);

    let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "c", "-d", "a"]);
    insta::assert_snapshot!(stdout, @r###"
    Also rebased 2 descendant commits onto parent of rebased commit
    Working copy now at: 3e176b54d680 (no description set)
    Added 0 files, modified 0 files, removed 2 files
    "###);
    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
    @
    o d
    | o c
    o | b
    | o a
    |/
    o
    "###);
}

[ilyagr]

I think I figured out my original problem, but it uncovered another problem (see below). I don't have the energy to figure out the new problem today, though.

---- test_rebase_single_revision stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: rebase_single_revision-3
Source: tests/test_rebase_command.rs:199
───────────────────────────────────────────────────────────────────────
Expression: get_log_output(&test_env, &repo_path)
───────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────
    0     0 │ @
    1     1 │ o d
    2     2 │ o c
    3     3 │ | o b
    4       │-| o a
    5     4 │ |/
    6       │-o
          5 │+o a
          6 │+o
────────────┴──────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'test_rebase_single_revision' panicked at 'snapshot assertion for 'rebase_single_revision-3' failed in line 199', /home/ilyagr/.local/app/cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.19.1/src/runtime.rs:461:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:142:14
   2: insta::runtime::finalize_assertion
             at /home/ilyagr/.local/app/cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.19.1/src/runtime.rs:461:9
   3: insta::runtime::assert_snapshot
             at /home/ilyagr/.local/app/cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.19.1/src/runtime.rs:518:9
   4: test_rebase_command::test_rebase_single_revision
             at ./tests/test_rebase_command.rs:199:5
   5: test_rebase_command::test_rebase_single_revision::{{closure}}
             at ./tests/test_rebase_command.rs:168:1
   6: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[ilyagr]

Actually, I could argue that the last change to the test is desired behavior. WDYT?

I changed the test, so the tests now pass.

[martinvonz]

Actually, I could argue that the last change to the test is desired behavior. WDYT?

Yep, that looks much better. Maybe I was too confident that it worked correctly before so I didn't look carefully enough before accepting the current output.

Thanks for the fix! I'll take a look at the code later. I just wanted to share that I think the change to the test makes sense.

[martinvonz]

Looks great! Thanks again.

[ilyagr]

Thank you for the quick review!

[ilyagr]

I realized that the logic I added here should be identical to a large piece of jj abandon. Perhaps that's what you meant in your code comment.

I think that the operation "rebase all the children of X onto parents of X, without losing the other parents of the children" could be factored out into a function. rebase -r does that and then rebase_commit. abandon does that and then forgets the commit.

squash also does something conceptually very similar, but there's less rebasing involved.

[martinvonz]

I realized that the logic I added here should be identical to a large piece of jj abandon. Perhaps that's what you meant in your code comment.

I think that the operation "rebase all the children of X onto parents of X, without losing the other parents of the children" could be factored out into a function. rebase -r does that and then rebase_commit. abandon does that and then forgets the commit.

squash also does something conceptually very similar, but there's less rebasing involved.

Yes, that's what I meant (in https://github.com/martinvonz/jj/pull/538/files#r967986097). The common code lives in https://github.com/martinvonz/jj/blob/main/lib/src/rewrite.rs. That code knows how to move commits on top of rewritten commits and to updates branches accordingly. It also knows how to rebase children onto parents of abandoned commits, and to updated branches to the parents of abandoned commits. However, jj rebase -r does something that's a mix of those two - it updates branches to the rewritten commit but it rebases descendants onto the parents of the old version. We could fix that by extracting a function for updating branches, so we could call that to move the branches (and working copies) to the rewritten commit and then we could use the existing code for moving descendants to parents of abandoned commits.