In a git colocated repo, commits are not rebased off of a deleted/moved branch

[pranaysashank]

Description

After jj git fetch, once a local branch is deleted because the commits / branch is deleted on the remote, commits on top of the deleted branch should be rebased off of it. However this doesn't happen in jj repos colocated with .git

Steps to Reproduce the Problem

1. Create a branch on remote and fetch from it
2. Create some commit on the same branch locally (rp in the snippet below)
3. Check jj log, you should have a few commits

o 1867eb460672 [email protected] 2022-12-05 22:14:08.000 +05:30 11a736a7f55e
| C
@ 2816cc5946db [email protected] 2022-12-05 22:13:05.000 +05:30 rp 5e65d7236afe
| B
o e9909dd4bace [email protected] 2022-12-01 21:56:29.000 +05:30 master HEAD@git 1e70d30028cc
| A
o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000
  (no description set)

3. Delete the remote branch
4. Do a jj git fetch
5. Check the output in jj log, the branch should have been deleted

o 1867eb460672 [email protected] 2022-12-05 22:14:08.000 +05:30 11a736a7f55e
| C
@ 2816cc5946db [email protected] 2022-12-05 22:13:05.000 +05:30 5e65d7236afe
| B
o e9909dd4bace [email protected] 2022-12-01 21:56:29.000 +05:30 master HEAD@git 1e70d30028cc
| A
o 000000000000  1970-01-01 00:00:00.000 +00:00 000000000000
  (no description set)

Expected Behavior

C'
| 
@ (no description set)
A
| 
000000000000

Actual Behavior

C
|
@ B
|
A
|
000000000000

@martinvonz checked and confirmed that this problem doesn't happen in non co-located jj repos with git backend

Specifications

• Platform: nixos 22.05
• Version: 5.1.0

[samueltardieu]

I'm not sure commits should be rebased. Deleting a branch should only delete the corresponding head, but the commits still exist, in particular if other commits are based on them (this is similar to having a nameless branch containing them).

[martinvonz]

Here's my thought process:

1. If you abandon a commit in your local repo, we rebase descendants on top of the abandoned commit's parent(s)
2. If commits were abandoned on a remote (maybe a hypothetical native jj remote with support for anonymous heads), we should similarly rebase off of the abandoned commits
3. Git's concept of visible commits is entirely determined by refs, so making a commit unreachable is equivalent to abandoning it

I'm not sure I'm right, but that's my reasoning anyway.

[ilyagr]

The reasoning Martin gave makes sense, but I'm still unsure whether this is what the user wants in every situation. In any case, however, it's a bug that colocated repos have a different behavior from non-colocated ones.

I will probably try to understand why that happens; let me know if somebody already has an idea.

[ilyagr]

I haven't gotten to the bottom of this, but I have managed to reproduce this bug by adding a jj git export and jj git import into a key place for non-colocated repos. Here's what changed in the view:

79466c6#diff-74189cdc6ab62f4f1f94ca016ba2c1fcc783cbc93ba19e075b2b949c81c63dfbR369-R385

Presumably, the jj git export also deleted the branch in the local git repo.

Somehow, this results in jj git fetch resurrecting the commit.

[yuja]

iirc, in colocated repo, a local ref exists and it prevents the local commits from abandoned. Perhaps, jj git export would create such ref.

[ilyagr]

Yes, that seems about right.

[ilyagr]

I wonder if #779 might be related.

[ilyagr]

I made some partial progress, but I'm quite confused. Here's a brief status report: (I can elaborate if people are interested)

I tried to reproduce this bug in a non-colocated repository. The following test seemed to be it:

A test that tries to reproduce this bug in a non-colocated repo.

Note especially the line marked // NOTE: Removing this export fixes the test.

#[test]
fn test_git_colocated_fetch_deleted_branch() {
    let test_env = TestEnvironment::default();
    let origin_path = test_env.env_root().join("origin");
    git2::Repository::init(&origin_path).unwrap();
    test_env.jj_cmd_success(&origin_path, &["init", "--git-repo=."]);
    test_env.jj_cmd_success(&origin_path, &["describe", "-m=A"]);
    test_env.jj_cmd_success(&origin_path, &["branch", "create", "A"]);
    test_env.jj_cmd_success(&origin_path, &["new", "-m=B"]);
    test_env.jj_cmd_success(&origin_path, &["branch", "create", "B"]);
    test_env.jj_cmd_success(&origin_path, &["new", "-m=C"]);

    let clone_path = test_env.env_root().join("clone");
    std::fs::create_dir(&clone_path).unwrap();
    // git2::Repository::clone(origin_path.to_str().unwrap(), &clone_path).unwrap();
    test_env.jj_cmd_success(
        &clone_path,
        &[
            "git",
            "clone",
            origin_path.to_str().unwrap(),
            &clone_path.to_str().unwrap(),
        ],
    );
    insta::assert_snapshot!(get_log_output(&test_env, &clone_path), @r###"
    @  bc7d08e8de9b7bc248b9358a05e96f1671bbd4d9
    ◉  e1f4268fabd2c84e880c5eb5bd87e076180fc8e3 B
    ◉  a86754f975f953fa25da4265764adc0c62e9ce6b A master
    ◉  0000000000000000000000000000000000000000
    "###);

    test_env.jj_cmd_success(&origin_path, &["branch", "delete", "B"]);

    // NOTE: Removing this export fixes the test
    let stdout = test_env.jj_cmd_success(&clone_path, &["git", "export"]);
    insta::assert_snapshot!(stdout, @"");
    let stdout = test_env.jj_cmd_success(&clone_path, &["git", "import"]);
    insta::assert_snapshot!(stdout, @r###"
    Nothing changed.
    "###);

    let stdout = test_env.jj_cmd_success(&clone_path, &["git", "fetch"]);
    insta::assert_snapshot!(stdout, @"");

    // TODO: e1f4 should have been abandoned (#864)
    insta::assert_snapshot!(get_log_output(&test_env, &clone_path), @r###"
    @  bc7d08e8de9b7bc248b9358a05e96f1671bbd4d9
    ◉  e1f4268fabd2c84e880c5eb5bd87e076180fc8e3
    ◉  a86754f975f953fa25da4265764adc0c62e9ce6b A master
    ◉  0000000000000000000000000000000000000000
    "###);
}

See ilyagr@fetchdel-partial-fix for a fix to the above test. I may clean it up and turn it into a PR soon.

However, to my great surprise, that commit does NOT fix the bug in colocated repos. In other words, the test_git_colocated_fetch_deleted_branch() as it currently exists still has the same behavior.

[yuja]

However, to my great surprise, that commit does NOT fix the bug in colocated repos.

In a colocated repo, we have HEAD ref in addition to the exported local branches. That might be the case. I'm playing with a hacked jj that doesn't do git export/import automatically, and I noticed jj git fetch didn't abandon old commits if they had been checked out.

[ilyagr]

Good point, this is a likely explanation. I'll check whether #1592 fixed my problem, and what's going on with HEAD in general. Thank you!

[yuja]

Good point, I'll check whether #1592 fixed my problem. That seems pretty likely.

Nope, it tries to not abandon more, so wouldn't change the behavior. Maybe you can remove new_git_heads.insert(head_commit_id.clone()) to see if it is the source of the problem.

[ilyagr]

Yeah, I changed my mind about it being pretty likely and edited the comment too late. Still, I expect making the test show HEAD@git will clear things up significantly.

[ilyagr]

Indeed, the problem was the HEAD. Adding jj co A to test_git_colocated_fetch_deleted_branch() fixes the problem. 🎉

[ilyagr]

While #1600 solves this exact bug, I think, I'm not sure it solves every related problem completely. At the moment, I don't have a minimal where there's a clear bug.

For a not-so-minimal example where it's unclear whether there's a bug, here is a repo state:
jj_should_more_branches_be_abandoned.tar.gz

At operation 46e17, the dbg branch is at commit 446ae. After a fetch (operation 4710d), the dbg branch moves elsewhere (as opposed to being deleted), but the commit 46e17 is not abandoned, even with #1600. Naively, I'd expect it to be abandoned (I certainly don't enjoy cleaning it up), but I'm not 100% sure what the correct behavior is.

Here are outputs of jj log (with color, use less -R to view) and jj debug operation at the two relevant operations:
jj_should_more_branches-debuginfo.tar.gz

[ilyagr]

Indeed, it seems that what I described above is the same bug, just with moving the ref as opposed to deleting it.