diff --git a/CHANGELOG.md b/CHANGELOG.md index a76d69b21..717dcb42f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 See [automatic local branch creation](docs/config.md#automatic-local-branch-creation) for details. +* Pushing deleted/moved branches no longer abandons the local commits referenced + by the remote branches. + ### New features * `jj workspace add` now takes a `--revision` argument. diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index b229411ca..d74fbc0cf 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -977,7 +977,7 @@ fn cmd_git_push( force_pushed_branches, }; with_remote_callbacks(ui, |cb| { - git::push_branches(&git_repo, &remote, &targets, cb) + git::push_branches(tx.mut_repo(), &git_repo, &remote, &targets, cb) }) .map_err(|err| match err { GitPushError::InternalGitError(err) => map_git_error(err), @@ -988,9 +988,6 @@ fn cmd_git_push( ), _ => user_error(err.to_string()), })?; - // TODO: mark pushed remote branches as tracking - let stats = git::import_refs(tx.mut_repo(), &git_repo, &command.settings().git_settings())?; - print_git_import_stats(ui, &stats)?; tx.finish(ui)?; Ok(()) } diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 23a84c544..1617a0806 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -300,13 +300,22 @@ fn test_git_push_multiple() { Delete branch branch1 from 45a3aa29e907 Force branch branch2 from 8476341eb395 to 15dcdaa4f12f Add branch my-branch to 15dcdaa4f12f - Abandoned 2 commits that are no longer reachable. "###); let stdout = test_env.jj_cmd_success(&workspace_root, &["branch", "list"]); insta::assert_snapshot!(stdout, @r###" branch2: yqosqzyt 15dcdaa4 (empty) foo my-branch: yqosqzyt 15dcdaa4 (empty) foo "###); + let stdout = test_env.jj_cmd_success(&workspace_root, &["log", "-rall()"]); + insta::assert_snapshot!(stdout, @r###" + @ yqosqzyt test.user@example.com 2001-02-03 04:05:17.000 +07:00 branch2 my-branch 15dcdaa4 + │ (empty) foo + │ ◉ rlzusymt test.user@example.com 2001-02-03 04:05:10.000 +07:00 8476341e + ├─╯ (empty) description 2 + │ ◉ lzmmnrxq test.user@example.com 2001-02-03 04:05:08.000 +07:00 45a3aa29 + ├─╯ (empty) description 1 + ◉ zzzzzzzz root() 00000000 + "###); } #[test] @@ -588,7 +597,16 @@ fn test_git_push_deleted() { insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: Delete branch branch1 from 45a3aa29e907 - Abandoned 1 commits that are no longer reachable. + "###); + let stdout = test_env.jj_cmd_success(&workspace_root, &["log", "-rall()"]); + insta::assert_snapshot!(stdout, @r###" + ◉ rlzusymt test.user@example.com 2001-02-03 04:05:10.000 +07:00 branch2 8476341e + │ (empty) description 2 + │ ◉ lzmmnrxq test.user@example.com 2001-02-03 04:05:08.000 +07:00 45a3aa29 + ├─╯ (empty) description 1 + │ @ yqosqzyt test.user@example.com 2001-02-03 04:05:13.000 +07:00 5b36783c + ├─╯ (empty) (no description set) + ◉ zzzzzzzz root() 00000000 "###); let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--deleted"]); insta::assert_snapshot!(stdout, @""); diff --git a/docs/design/tracking-branches.md b/docs/design/tracking-branches.md index 82bc6706b..296efd52d 100644 --- a/docs/design/tracking-branches.md +++ b/docs/design/tracking-branches.md @@ -197,8 +197,7 @@ In particular, a merge of local and remote targets is * ~`tags`~ (not implemented, but should be the same as `branches`) 2. Pushes diff to the remote Git repo (as well as remote tracking branches in the backing Git repo.) - 3. Sets `remotes[remote].branches[name].state = tracking` - 4. Import changes only for `remotes[remote].branches[glob]` + 3. Updates `remotes[remote]` and `git_refs` reflecting the push. * `jj git export` 1. Copies local `branches`/`tags` back to `remotes["git"]`. @@ -288,25 +287,22 @@ Note: desired behavior of `jj branch forget` is to * Pushing new branch, remote doesn't exist 1. Pushes `[local, absent] - [absent]` -> `local` - 2. Sets `remotes[remote].branches[name].state = tracking` - 3. `import_refs()` merges `[local, local] - [absent]` -> `local` (noop) + 2. Sets `remotes[remote].branches[name].target = local`, `.state = tracking` * Pushing new branch, untracked remote exists 1. Pushes `[local, remote] - [absent]` * Fails if `local` moved backwards or sideways - 2. Sets `remotes[remote].branches[name].state = tracking` - 3. `import_refs()` merges `[local, local] - [remote]` -> `local` (noop) + 2. Sets `remotes[remote].branches[name].target = local`, `.state = tracking` * Pushing existing branch to tracking remote 1. Pushes `[local, remote] - [remote]` -> `local` * Fails if `local` moved backwards or sideways, and if `remote` is out of sync - 2. `import_refs()` merges `[local, local] - [remote]` -> `local` (noop) + 2. Sets `remotes[remote].branches[name].target = local` * Pushing existing branch to untracked remote * Same as "new branch" * Pushing deleted branch to tracking remote 1. Pushes `[absent, remote] - [remote]` -> `absent` * TODO: Fails if `remote` is out of sync? - 2. `import_refs()` merges `[absent, absent] - [remote]` -> `absent` - 3. Removes `remotes[remote].branches[name]` (`target` becomes `absent`) + 2. Removes `remotes[remote].branches[name]` (`target` becomes `absent`) * Pushing deleted branch to untracked remote * Noop since `[absent, remote] - [absent]` -> `remote` * Perhaps, UI will report error diff --git a/lib/src/git.rs b/lib/src/git.rs index c87becc5b..dd996070b 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1094,7 +1094,9 @@ pub struct GitRefUpdate { pub new_target: Option, } +/// Pushes the specified branches and updates the repo view accordingly. pub fn push_branches( + mut_repo: &mut MutableRepo, git_repo: &git2::Repository, remote_name: &str, targets: &GitBranchPushTargets, @@ -1110,9 +1112,24 @@ pub fn push_branches( }) .collect_vec(); push_updates(git_repo, remote_name, &ref_updates, callbacks)?; + + // TODO: add support for partially pushed refs? we could update the view + // excluding rejected refs, but the transaction would be aborted anyway + // if we returned an Err. + for (branch_name, update) in &targets.branch_updates { + let git_ref_name = format!("refs/remotes/{remote_name}/{branch_name}"); + let new_remote_ref = RemoteRef { + target: RefTarget::resolved(update.new_target.clone()), + state: RemoteRefState::Tracking, + }; + mut_repo.set_git_ref_target(&git_ref_name, new_remote_ref.target.clone()); + mut_repo.set_remote_branch(branch_name, remote_name, new_remote_ref); + } + Ok(()) } +/// Pushes the specified Git refs without updating the repo view. pub fn push_updates( git_repo: &git2::Repository, remote_name: &str, diff --git a/lib/src/op_store.rs b/lib/src/op_store.rs index 27c8fd429..8870d98c5 100644 --- a/lib/src/op_store.rs +++ b/lib/src/op_store.rs @@ -83,6 +83,11 @@ impl RefTarget { &TARGET } + /// Creates non-conflicting target that optionally points to a commit. + pub fn resolved(maybe_id: Option) -> Self { + Self::from_merge(Merge::resolved(maybe_id)) + } + /// Creates non-conflicting target pointing to a commit. pub fn normal(id: CommitId) -> Self { Self::from_merge(Merge::normal(id)) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 21b792ab3..cb5a63646 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -2256,6 +2256,20 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet .set_parents(vec![initial_commit.id().clone()]) .write() .unwrap(); + tx.mut_repo().set_git_ref_target( + "refs/remotes/origin/main", + RefTarget::normal(initial_commit.id().clone()), + ); + tx.mut_repo().set_remote_branch( + "main", + "origin", + RemoteRef { + target: RefTarget::normal(initial_commit.id().clone()), + // Caller expects the main branch is tracked. The corresponding local branch will + // be created (or left as deleted) by caller. + state: RemoteRefState::Tracking, + }, + ); let jj_repo = tx.commit(); PushTestSetup { source_repo_dir, @@ -2269,8 +2283,9 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet fn test_push_branches_success() { let settings = testutils::user_settings(); let temp_dir = testutils::new_temp_dir(); - let setup = set_up_push_repos(&settings, &temp_dir); + let mut setup = set_up_push_repos(&settings, &temp_dir); let clone_repo = get_git_repo(&setup.jj_repo); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let targets = GitBranchPushTargets { branch_updates: vec![( @@ -2283,6 +2298,7 @@ fn test_push_branches_success() { force_pushed_branches: hashset! {}, }; let result = git::push_branches( + tx.mut_repo(), &clone_repo, "origin", &targets, @@ -2307,14 +2323,35 @@ fn test_push_branches_success() { .unwrap() .target(); assert_eq!(new_target, Some(new_oid)); + + // Check that the repo view got updated + let view = tx.mut_repo().view(); + assert_eq!( + *view.get_git_ref("refs/remotes/origin/main"), + RefTarget::normal(setup.new_commit.id().clone()), + ); + assert_eq!( + *view.get_remote_branch("main", "origin"), + RemoteRef { + target: RefTarget::normal(setup.new_commit.id().clone()), + state: RemoteRefState::Tracking, + }, + ); + + // Check that the repo view reflects the changes in the Git repo + setup.jj_repo = tx.commit(); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); + git::import_refs(tx.mut_repo(), &clone_repo, &GitSettings::default()).unwrap(); + assert!(!tx.mut_repo().has_changes()); } #[test] fn test_push_branches_deletion() { let settings = testutils::user_settings(); let temp_dir = testutils::new_temp_dir(); - let setup = set_up_push_repos(&settings, &temp_dir); + let mut setup = set_up_push_repos(&settings, &temp_dir); let clone_repo = get_git_repo(&setup.jj_repo); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let source_repo = git2::Repository::open(&setup.source_repo_dir).unwrap(); // Test the setup @@ -2331,6 +2368,7 @@ fn test_push_branches_deletion() { force_pushed_branches: hashset! {}, }; let result = git::push_branches( + tx.mut_repo(), &get_git_repo(&setup.jj_repo), "origin", &targets, @@ -2347,14 +2385,26 @@ fn test_push_branches_deletion() { assert!(clone_repo .find_reference("refs/remotes/origin/main") .is_err()); + + // Check that the repo view got updated + let view = tx.mut_repo().view(); + assert!(view.get_git_ref("refs/remotes/origin/main").is_absent()); + assert!(view.get_remote_branch("main", "origin").is_absent()); + + // Check that the repo view reflects the changes in the Git repo + setup.jj_repo = tx.commit(); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); + git::import_refs(tx.mut_repo(), &clone_repo, &GitSettings::default()).unwrap(); + assert!(!tx.mut_repo().has_changes()); } #[test] fn test_push_branches_mixed_deletion_and_addition() { let settings = testutils::user_settings(); let temp_dir = testutils::new_temp_dir(); - let setup = set_up_push_repos(&settings, &temp_dir); + let mut setup = set_up_push_repos(&settings, &temp_dir); let clone_repo = get_git_repo(&setup.jj_repo); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let targets = GitBranchPushTargets { branch_updates: vec![ @@ -2376,6 +2426,7 @@ fn test_push_branches_mixed_deletion_and_addition() { force_pushed_branches: hashset! {}, }; let result = git::push_branches( + tx.mut_repo(), &clone_repo, "origin", &targets, @@ -2393,6 +2444,28 @@ fn test_push_branches_mixed_deletion_and_addition() { // Check that the main ref got deleted in the source repo assert!(source_repo.find_reference("refs/heads/main").is_err()); + + // Check that the repo view got updated + let view = tx.mut_repo().view(); + assert!(view.get_git_ref("refs/remotes/origin/main").is_absent()); + assert!(view.get_remote_branch("main", "origin").is_absent()); + assert_eq!( + *view.get_git_ref("refs/remotes/origin/topic"), + RefTarget::normal(setup.new_commit.id().clone()), + ); + assert_eq!( + *view.get_remote_branch("topic", "origin"), + RemoteRef { + target: RefTarget::normal(setup.new_commit.id().clone()), + state: RemoteRefState::Tracking, + }, + ); + + // Check that the repo view reflects the changes in the Git repo + setup.jj_repo = tx.commit(); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); + git::import_refs(tx.mut_repo(), &clone_repo, &GitSettings::default()).unwrap(); + assert!(!tx.mut_repo().has_changes()); } #[test] @@ -2403,6 +2476,7 @@ fn test_push_branches_not_fast_forward() { let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let new_commit = write_random_commit(tx.mut_repo(), &settings); setup.jj_repo = tx.commit(); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let targets = GitBranchPushTargets { branch_updates: vec![( @@ -2415,6 +2489,7 @@ fn test_push_branches_not_fast_forward() { force_pushed_branches: hashset! {}, }; let result = git::push_branches( + tx.mut_repo(), &get_git_repo(&setup.jj_repo), "origin", &targets, @@ -2431,6 +2506,7 @@ fn test_push_branches_not_fast_forward_with_force() { let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let new_commit = write_random_commit(tx.mut_repo(), &settings); setup.jj_repo = tx.commit(); + let mut tx = setup.jj_repo.start_transaction(&settings, "test"); let targets = GitBranchPushTargets { branch_updates: vec![( @@ -2445,6 +2521,7 @@ fn test_push_branches_not_fast_forward_with_force() { }, }; let result = git::push_branches( + tx.mut_repo(), &get_git_repo(&setup.jj_repo), "origin", &targets,