Skip to content

Commit

Permalink
git: on deserialize/import/export, copy refs/heads/* to remote named …
Browse files Browse the repository at this point in the history
…"git"

I've added a boolean flag to the store to ensure that the migration never runs
more than once after the view gets "op restore"-d. I'll probably reorganize the
branches structure to support non-tracking branches later, but updating the
storage format in a single commit would be too involved.

If jj is downgraded, these "git" remote refs would be exported to the Git repo.
Users might have to remove them manually.
  • Loading branch information
yuja committed Oct 7, 2023
1 parent 9407d4e commit 717d0d3
Show file tree
Hide file tree
Showing 9 changed files with 159 additions and 55 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
should be noticeably faster on large repos. You may need to create a new
clone to see the full speedup.

* The `remote_branches()` revset now includes branches exported to the Git
repository (so called Git-tracking branches.) Use
`remote_branches(remote=exact:"origin")` to query branches of certain remote.

### New features

### Fixed bugs
Expand Down
21 changes: 6 additions & 15 deletions cli/tests/test_branch_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ fn test_branch_forget_export() {
insta::assert_snapshot!(stdout, @"");
// Forgetting a branch does not delete its local-git tracking branch. The
// git-tracking branch is kept.
// TODO: Actually git-tracking branch is forgotten. Update the branch
// resolution code to not shadow the real @git branch.
let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
insta::assert_snapshot!(stdout, @r###"
foo (forgotten)
Expand All @@ -258,25 +260,14 @@ fn test_branch_forget_export() {
(empty) (no description set)
"###);

// The presence of the @git branch means that a `jj git import` is a no-op...
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "import"]);
insta::assert_snapshot!(stdout, @r###"
Nothing changed.
"###);
// ... and a `jj git export` will delete the branch from git and will delete the
// git-tracking branch. In a colocated repo, this will happen automatically
// immediately after a `jj branch forget`. This is demonstrated in
// `test_git_colocated_branch_forget` in test_git_colocated.rs
// `jj git export` will delete the branch from git. In a colocated repo,
// this will happen automatically immediately after a `jj branch forget`.
// This is demonstrated in `test_git_colocated_branch_forget` in
// test_git_colocated.rs
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "export"]);
insta::assert_snapshot!(stdout, @"");
let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
insta::assert_snapshot!(stdout, @"");

// Note that if `jj branch forget` *did* delete foo@git, a subsequent `jj
// git export` would be a no-op and a `jj git import` would resurrect
// the branch. In a normal repo, that might be OK. In a colocated repo,
// this would automatically happen before the next command, making `jj
// branch forget` useless.
}

#[test]
Expand Down
34 changes: 12 additions & 22 deletions cli/tests/test_git_import_export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,30 +39,20 @@ fn test_resolution_of_git_tracking_branches() {
"###);

// Test that we can address both revisions
let stdout = test_env.jj_cmd_success(
&repo_path,
&[
"log",
"-r=main",
"-T",
r#"commit_id ++ " " ++ description"#,
"--no-graph",
],
);
insta::assert_snapshot!(stdout, @r###"
let query = |expr| {
let template = r#"commit_id ++ " " ++ description"#;
test_env.jj_cmd_success(
&repo_path,
&["log", "-r", expr, "-T", template, "--no-graph"],
)
};
insta::assert_snapshot!(query("main"), @r###"
3af370264cdcbba791762f8ef6bc79b456dcbf3b new_message
"###);
let stdout = test_env.jj_cmd_success(
&repo_path,
&[
"log",
"-r=main@git",
"-T",
r#"commit_id ++ " " ++ description"#,
"--no-graph",
],
);
insta::assert_snapshot!(stdout, @r###"
insta::assert_snapshot!(query("main@git"), @r###"
16d541ca40f42baf2dea41aa61a0b5f1cbf1f91b old_message
"###);
insta::assert_snapshot!(query(r#"remote_branches(exact:"main", exact:"git")"#), @r###"
16d541ca40f42baf2dea41aa61a0b5f1cbf1f91b old_message
"###);
}
Expand Down
35 changes: 35 additions & 0 deletions cli/tests/test_git_remotes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::fs;

use crate::common::TestEnvironment;

pub mod common;
Expand Down Expand Up @@ -133,6 +135,7 @@ fn test_git_remote_named_git() {
.remote("git", "http://example.com/repo/repo")
.unwrap();
test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]);
test_env.jj_cmd_success(&repo_path, &["branch", "set", "main"]);

// The remote can be renamed.
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "rename", "git", "bar"]);
Expand All @@ -141,4 +144,36 @@ fn test_git_remote_named_git() {
insta::assert_snapshot!(stdout, @r###"
bar http://example.com/repo/repo
"###);
// @git branch shouldn't be renamed.
let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rmain@git", "-Tbranches"]);
insta::assert_snapshot!(stdout, @r###"
@ main
~
"###);

// The remote cannot be renamed back by jj.
let stderr = test_env.jj_cmd_failure(&repo_path, &["git", "remote", "rename", "bar", "git"]);
insta::assert_snapshot!(stderr, @r###"
Error: Git remote named 'git' is reserved for local Git repository
"###);

// Reinitialize the repo with remote named 'git'.
fs::remove_dir_all(repo_path.join(".jj")).unwrap();
git_repo.remote_rename("bar", "git").unwrap();
test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]);

// The remote can also be removed.
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "remove", "git"]);
insta::assert_snapshot!(stdout, @"");
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
insta::assert_snapshot!(stdout, @r###"
"###);
// @git branch shouldn't be removed.
let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rmain@git", "-Tbranches"]);
insta::assert_snapshot!(stdout, @r###"
◉ main
~
"###);
}
48 changes: 41 additions & 7 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,14 @@ pub fn import_some_refs(
mut_repo.merge_single_ref(&local_ref_name, old_git_target, new_git_target);
}
} else {
if let RefName::LocalBranch(branch) = ref_name {
// Update Git-tracking branch like the other remote branches.
mut_repo.set_remote_branch_target(
branch,
REMOTE_NAME_FOR_LOCAL_GIT_REPO,
new_git_target.clone(),
);
}
mut_repo.merge_single_ref(ref_name, old_git_target, new_git_target);
}
}
Expand Down Expand Up @@ -493,7 +501,15 @@ pub fn export_some_refs(
reason,
});
} else {
mut_repo.set_git_ref_target(&git_ref_name, RefTarget::absent());
let new_target = RefTarget::absent();
if let RefName::LocalBranch(branch) = &parsed_ref_name {
mut_repo.set_remote_branch_target(
branch,
REMOTE_NAME_FOR_LOCAL_GIT_REPO,
new_target.clone(),
);
}
mut_repo.set_git_ref_target(&git_ref_name, new_target);
}
}
for (parsed_ref_name, (old_oid, new_oid)) in branches_to_update {
Expand All @@ -504,10 +520,15 @@ pub fn export_some_refs(
reason,
});
} else {
mut_repo.set_git_ref_target(
&git_ref_name,
RefTarget::normal(CommitId::from_bytes(new_oid.as_bytes())),
);
let new_target = RefTarget::normal(CommitId::from_bytes(new_oid.as_bytes()));
if let RefName::LocalBranch(branch) = &parsed_ref_name {
mut_repo.set_remote_branch_target(
branch,
REMOTE_NAME_FOR_LOCAL_GIT_REPO,
new_target.clone(),
);
}
mut_repo.set_git_ref_target(&git_ref_name, new_target);
}
}
failed_branches.sort_by_key(|failed| failed.name.clone());
Expand All @@ -533,6 +554,7 @@ fn diff_refs_to_export(
target
.remote_targets
.keys()
.filter(|&remote| remote != REMOTE_NAME_FOR_LOCAL_GIT_REPO)
.map(|remote| RefName::RemoteBranch {
branch: branch.to_string(),
remote: remote.to_string(),
Expand Down Expand Up @@ -777,6 +799,13 @@ pub fn remove_remote(
GitRemoteManagementError::InternalGitError(err)
}
})?;
if remote_name != REMOTE_NAME_FOR_LOCAL_GIT_REPO {
remove_remote_refs(mut_repo, remote_name);
}
Ok(())
}

fn remove_remote_refs(mut_repo: &mut MutableRepo, remote_name: &str) {
let mut branches_to_delete = vec![];
for (branch, target) in mut_repo.view().branches() {
if target.remote_targets.contains_key(remote_name) {
Expand All @@ -797,7 +826,6 @@ pub fn remove_remote(
for git_ref in git_refs_to_delete {
mut_repo.set_git_ref_target(&git_ref, RefTarget::absent());
}
Ok(())
}

pub fn rename_remote(
Expand All @@ -820,6 +848,13 @@ pub fn rename_remote(
GitRemoteManagementError::InternalGitError(err)
}
})?;
if old_remote_name != REMOTE_NAME_FOR_LOCAL_GIT_REPO {
rename_remote_refs(mut_repo, old_remote_name, new_remote_name);
}
Ok(())
}

fn rename_remote_refs(mut_repo: &mut MutableRepo, old_remote_name: &str, new_remote_name: &str) {
mut_repo.rename_remote(old_remote_name, new_remote_name);
let prefix = format!("refs/remotes/{old_remote_name}/");
let git_refs = mut_repo
Expand All @@ -840,7 +875,6 @@ pub fn rename_remote(
mut_repo.set_git_ref_target(&old, RefTarget::absent());
mut_repo.set_git_ref_target(&new, target);
}
Ok(())
}

#[derive(Error, Debug)]
Expand Down
2 changes: 2 additions & 0 deletions lib/src/protos/op_store.proto
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ message View {
// TODO: Delete support for the old format.
bytes git_head_legacy = 7 [deprecated = true];
RefTarget git_head = 9;
// Whether "@git" branches have been migrated to remote_targets.
bool has_git_refs_migrated_to_remote = 10;
}

message Operation {
Expand Down
3 changes: 3 additions & 0 deletions lib/src/protos/op_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ pub struct View {
pub git_head_legacy: ::prost::alloc::vec::Vec<u8>,
#[prost(message, optional, tag = "9")]
pub git_head: ::core::option::Option<RefTarget>,
/// Whether "@git" branches have been migrated to remote_targets.
#[prost(bool, tag = "10")]
pub has_git_refs_migrated_to_remote: bool,
}
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
Expand Down
30 changes: 28 additions & 2 deletions lib/src/simple_op_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,11 @@ fn operation_from_proto(proto: crate::protos::op_store::Operation) -> Operation
}

fn view_to_proto(view: &View) -> crate::protos::op_store::View {
let mut proto = crate::protos::op_store::View::default();
let mut proto = crate::protos::op_store::View {
// New/loaded view should have been migrated to the latest format
has_git_refs_migrated_to_remote: true,
..Default::default()
};
for (workspace_id, commit_id) in &view.wc_commit_ids {
proto
.wc_commit_ids
Expand Down Expand Up @@ -355,7 +359,10 @@ fn view_from_proto(proto: crate::protos::op_store::View) -> View {
view.git_head = RefTarget::normal(CommitId::new(proto.git_head_legacy));
}

migrate_git_refs_to_remote(&mut view);
if !proto.has_git_refs_migrated_to_remote {
migrate_git_refs_to_remote(&mut view);
}

view
}

Expand All @@ -371,6 +378,16 @@ fn migrate_git_refs_to_remote(view: &mut View) {
.remote_targets
.remove(git::REMOTE_NAME_FOR_LOCAL_GIT_REPO);
}
for (full_name, target) in &view.git_refs {
if let Some(name) = full_name.strip_prefix("refs/heads/") {
assert!(!name.is_empty());
let branch_target = view.branches.entry(name.to_owned()).or_default();
branch_target.remote_targets.insert(
git::REMOTE_NAME_FOR_LOCAL_GIT_REPO.to_owned(),
target.clone(),
);
}
}

// jj < 0.9 might have imported refs from remote named "git"
let reserved_git_ref_prefix = format!("refs/remotes/{}/", git::REMOTE_NAME_FOR_LOCAL_GIT_REPO);
Expand Down Expand Up @@ -609,6 +626,7 @@ mod tests {
git_ref_to_proto("refs/remotes/git/main", &normal_ref_target("555555")),
git_ref_to_proto("refs/remotes/gita/main", &normal_ref_target("666666")),
],
has_git_refs_migrated_to_remote: false,
..Default::default()
};

Expand All @@ -619,6 +637,7 @@ mod tests {
"main".to_owned() => BranchTarget {
local_target: normal_ref_target("111111"),
remote_targets: btreemap! {
"git".to_owned() => normal_ref_target("444444"), // refs/heads/main
"gita".to_owned() => normal_ref_target("333333"),
},
},
Expand All @@ -631,6 +650,13 @@ mod tests {
"refs/remotes/gita/main".to_owned() => normal_ref_target("666666"),
},
);

// Once migrated, "git" remote branches shouldn't be populated again.
let mut proto = view_to_proto(&view);
assert!(proto.has_git_refs_migrated_to_remote);
proto.branches.clear();
let view = view_from_proto(proto);
assert!(view.branches.is_empty());
}

#[test]
Expand Down
Loading

0 comments on commit 717d0d3

Please sign in to comment.