Skip to content

Commit

Permalink
view: add tracking state to RemoteRef
Browse files Browse the repository at this point in the history
The state field isn't saved yet. git import/export code paths are migrated,
but new tracking state is always calculated based on git.auto-local-branch
setting. So the tracking state is effectively a global flag.

As we don't know whether the existing remote branches have been merged in to
local branches, we assume that remote branches are "tracking" so long as the
local counterparts exist. This means existing locally-deleted branch won't
be pushed without re-tracking it. I think it's rare to leave locally-deleted
branches for long. For "git.auto-local-branch = false" setup, users might have
to untrack branches if they've manually "merged" remote branches and want to
continue that workflow. I considered using git.auto-local-branch setting in the
migration path, but I don't think that would give a better result. The setting
may be toggled after the branches got merged, and I'm planning to change it
default off for better Git interop.

Implementation-wise, the state enum can be a simple bool. It's enum just
because I originally considered to pack "forgotten" concept into it. I have
no idea which will be better for future extension.
  • Loading branch information
yuja committed Oct 15, 2023
1 parent 8b46712 commit 4fdafe3
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 41 deletions.
40 changes: 31 additions & 9 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use thiserror::Error;
use crate::backend::{BackendError, CommitId, ObjectId};
use crate::commit::Commit;
use crate::git_backend::GitBackend;
use crate::op_store::{RefTarget, RefTargetOptionExt, RemoteRef};
use crate::op_store::{RefTarget, RefTargetOptionExt, RemoteRef, RemoteRefState};
use crate::repo::{MutableRepo, Repo};
use crate::revset::{self, RevsetExpression};
use crate::settings::GitSettings;
Expand Down Expand Up @@ -263,23 +263,25 @@ pub fn import_some_refs(
for (ref_name, (old_target, new_target)) in &changed_remote_refs {
let new_remote_ref = RemoteRef {
target: new_target.clone(),
// TODO: preserve the old state
state: default_remote_ref_state_for(ref_name, git_settings),
};
if let RefName::RemoteBranch { branch, remote } = ref_name {
if new_remote_ref.is_tracking() {
let local_ref_name = RefName::LocalBranch(branch.clone());
mut_repo.merge_single_ref(&local_ref_name, old_target, &new_remote_ref.target);
}
// Remote-tracking branch is the last known state of the branch in the remote.
// It shouldn't diverge even if we had inconsistent view.
mut_repo.set_remote_branch(branch, remote, new_remote_ref);
// If a git remote-tracking branch changed, apply the change to the local branch
// as well.
if git_settings.auto_local_branch {
let local_ref_name = RefName::LocalBranch(branch.clone());
mut_repo.merge_single_ref(&local_ref_name, old_target, new_target);
}
} else {
if new_remote_ref.is_tracking() {
mut_repo.merge_single_ref(ref_name, old_target, &new_remote_ref.target);
}
if let RefName::LocalBranch(branch) = ref_name {
// Update Git-tracking branch like the other remote branches.
mut_repo.set_remote_branch(branch, REMOTE_NAME_FOR_LOCAL_GIT_REPO, new_remote_ref);
}
mut_repo.merge_single_ref(ref_name, old_target, new_target);
}
}

Expand Down Expand Up @@ -410,6 +412,21 @@ fn diff_refs_to_import(
})
}

fn default_remote_ref_state_for(ref_name: &RefName, git_settings: &GitSettings) -> RemoteRefState {
match ref_name {
// LocalBranch means Git-tracking branch
RefName::LocalBranch(_) | RefName::Tag(_) => RemoteRefState::Tracking,
RefName::RemoteBranch { .. } => {
if git_settings.auto_local_branch {
RemoteRefState::Tracking
} else {
RemoteRefState::New
}
}
RefName::GitRef(_) => unreachable!(),
}
}

/// Commits referenced by local branches, tags, or HEAD@git.
///
/// On `import_refs()`, this is similar to collecting commits referenced by
Expand Down Expand Up @@ -565,6 +582,8 @@ fn copy_exportable_local_branches_to_remote_view(
.view()
.local_remote_branches(remote_name)
.filter_map(|(branch, targets)| {
// TODO: filter out untracked branches (if we add support for untracked @git
// branches)
let old_target = targets.remote_target;
let new_target = targets.local_target;
(!new_target.has_conflict() && old_target != new_target).then_some((branch, new_target))
Expand All @@ -573,7 +592,10 @@ fn copy_exportable_local_branches_to_remote_view(
.map(|(branch, new_target)| (branch.to_owned(), new_target.clone()))
.collect_vec();
for (branch, new_target) in new_local_branches {
let new_remote_ref = RemoteRef { target: new_target };
let new_remote_ref = RemoteRef {
target: new_target,
state: RemoteRefState::Tracking,
};
mut_repo.set_remote_branch(&branch, remote_name, new_remote_ref);
}
}
Expand Down
27 changes: 26 additions & 1 deletion lib/src/op_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use once_cell::sync::Lazy;
use thiserror::Error;

use crate::backend::{id_type, CommitId, ObjectId, Timestamp};
use crate::content_hash::ContentHash;
use crate::merge::Merge;

content_hash! {
Expand Down Expand Up @@ -139,7 +140,7 @@ content_hash! {
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct RemoteRef {
pub target: RefTarget,
// TODO: add tracking flag or enum
pub state: RemoteRefState,
}
}

Expand All @@ -148,6 +149,7 @@ impl RemoteRef {
pub fn absent() -> Self {
RemoteRef {
target: RefTarget::absent(),
state: RemoteRefState::New,
}
}

Expand All @@ -168,6 +170,28 @@ impl RemoteRef {
pub fn is_present(&self) -> bool {
self.target.is_present()
}

/// Returns true if the ref is supposed to be merged in to the local ref.
pub fn is_tracking(&self) -> bool {
self.state == RemoteRefState::Tracking
}
}

/// Whether the ref is tracked or not.
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
#[repr(u8)]
pub enum RemoteRefState {
/// Remote ref is not merged in to the local ref.
New,
/// Remote ref has been merged in to the local ref. Incoming ref will be
/// merged, too.
Tracking,
}

impl ContentHash for RemoteRefState {
fn hash(&self, state: &mut impl digest::Update) {
(*self as u8).hash(state);
}
}

/// Helper to strip redundant `Option<T>` from `RefTarget` lookup result.
Expand Down Expand Up @@ -384,6 +408,7 @@ mod tests {
fn test_merge_join_branch_views() {
let remote_ref = |target: &RefTarget| RemoteRef {
target: target.clone(),
state: RemoteRefState::Tracking, // doesn't matter
};
let local_branch1_target = RefTarget::normal(CommitId::from_hex("111111"));
let local_branch2_target = RefTarget::normal(CommitId::from_hex("222222"));
Expand Down
1 change: 1 addition & 0 deletions lib/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,7 @@ impl MutableRepo {
self.view_mut().add_head(added_head);
}

// TODO: somehow merge tracking state of remote refs?
let changed_refs = itertools::chain!(
diff_named_refs(base.local_branches(), other.local_branches())
.map(|(name, diff)| (RefName::LocalBranch(name.to_owned()), diff)),
Expand Down
83 changes: 61 additions & 22 deletions lib/src/simple_op_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::file_util::persist_content_addressed_temp_file;
use crate::merge::Merge;
use crate::op_store::{
OpStore, OpStoreError, OpStoreResult, Operation, OperationId, OperationMetadata, RefTarget,
RemoteRef, RemoteView, View, ViewId, WorkspaceId,
RemoteRef, RemoteRefState, RemoteView, View, ViewId, WorkspaceId,
};
use crate::{git, op_store};

Expand Down Expand Up @@ -370,16 +370,28 @@ fn branch_views_from_proto_legacy(
let mut local_branches: BTreeMap<String, RefTarget> = BTreeMap::new();
let mut remote_views: BTreeMap<String, RemoteView> = BTreeMap::new();
for branch_proto in branches_legacy {
let local_target = ref_target_from_proto(branch_proto.local_target);
for remote_branch in branch_proto.remote_branches {
// If local branch doesn't exist, we assume that the remote branch hasn't been
// merged because git.auto-local-branch was off. That's probably more common
// than deleted but yet-to-be-pushed local branch. Alternatively, we could read
// git.auto-local-branch setting here, but that wouldn't always work since the
// setting could be toggled after the branch got merged.
let is_git_tracking = remote_branch.remote_name == git::REMOTE_NAME_FOR_LOCAL_GIT_REPO;
let state = if is_git_tracking || local_target.is_present() {
RemoteRefState::Tracking
} else {
RemoteRefState::New
};
let remote_view = remote_views.entry(remote_branch.remote_name).or_default();
let remote_ref = RemoteRef {
target: ref_target_from_proto(remote_branch.target),
state,
};
remote_view
.branches
.insert(branch_proto.name.clone(), remote_ref);
}
let local_target = ref_target_from_proto(branch_proto.local_target);
if local_target.is_present() {
local_branches.insert(branch_proto.name, local_target);
}
Expand All @@ -400,6 +412,8 @@ fn migrate_git_refs_to_remote(view: &mut View) {
assert!(!name.is_empty());
let remote_ref = RemoteRef {
target: target.clone(),
// Git-tracking branches should never be untracked.
state: RemoteRefState::Tracking,
};
git_view.branches.insert(name.to_owned(), remote_ref);
}
Expand Down Expand Up @@ -499,8 +513,13 @@ mod tests {
use crate::op_store::{OperationMetadata, RefTarget, WorkspaceId};

fn create_view() -> View {
let remote_ref = |target: &RefTarget| RemoteRef {
let new_remote_ref = |target: &RefTarget| RemoteRef {
target: target.clone(),
state: RemoteRefState::New,
};
let tracking_remote_ref = |target: &RefTarget| RemoteRef {
target: target.clone(),
state: RemoteRefState::Tracking,
};
let head_id1 = CommitId::from_hex("aaa111");
let head_id2 = CommitId::from_hex("aaa222");
Expand Down Expand Up @@ -529,8 +548,8 @@ mod tests {
remote_views: btreemap! {
"origin".to_string() => RemoteView {
branches: btreemap! {
"main".to_string() => remote_ref(&branch_main_origin_target),
"deleted".to_string() => remote_ref(&branch_deleted_origin_target),
"main".to_string() => tracking_remote_ref(&branch_main_origin_target),
"deleted".to_string() => new_remote_ref(&branch_deleted_origin_target),
},
},
},
Expand Down Expand Up @@ -578,7 +597,7 @@ mod tests {
// Test exact output so we detect regressions in compatibility
assert_snapshot!(
ViewId::new(blake2b_hash(&create_view()).to_vec()).hex(),
@"6ef5f01bb0bd239670c79966753c6af9ce18694bba1d5dbd1aa82de7f5c421dfc3bf91c1608eec480ae8e244093485bcff3e95db7acdc3958c7a8ead7a453b54"
@"7a7d8e33aff631bc3a8a281358e818f3c962d539ec2ced78a40b8221a42a707d51e546c5a6644c435b5764d8a51b29e63c3c107d5a8926d4be74288ea8ac879d"
);
}

Expand Down Expand Up @@ -613,8 +632,13 @@ mod tests {

#[test]
fn test_branch_views_legacy_roundtrip() {
let remote_ref = |target: &RefTarget| RemoteRef {
let new_remote_ref = |target: &RefTarget| RemoteRef {
target: target.clone(),
state: RemoteRefState::New,
};
let tracking_remote_ref = |target: &RefTarget| RemoteRef {
target: target.clone(),
state: RemoteRefState::Tracking,
};
let local_branch1_target = RefTarget::normal(CommitId::from_hex("111111"));
let local_branch3_target = RefTarget::normal(CommitId::from_hex("222222"));
Expand All @@ -629,18 +653,18 @@ mod tests {
let remote_views = btreemap! {
"git".to_owned() => RemoteView {
branches: btreemap! {
"branch1".to_owned() => remote_ref(&git_branch1_target),
"branch1".to_owned() => tracking_remote_ref(&git_branch1_target),
},
},
"remote1".to_owned() => RemoteView {
branches: btreemap! {
"branch1".to_owned() => remote_ref(&remote1_branch1_target),
"branch1".to_owned() => tracking_remote_ref(&remote1_branch1_target),
},
},
"remote2".to_owned() => RemoteView {
branches: btreemap! {
"branch2".to_owned() => remote_ref(&remote2_branch2_target),
"branch4".to_owned() => remote_ref(&remote2_branch4_target),
"branch2".to_owned() => new_remote_ref(&remote2_branch2_target),
"branch4".to_owned() => new_remote_ref(&remote2_branch4_target),
},
},
};
Expand All @@ -664,8 +688,13 @@ mod tests {
#[test]
fn test_migrate_git_refs_remote_named_git() {
let normal_ref_target = |id_hex: &str| RefTarget::normal(CommitId::from_hex(id_hex));
let normal_remote_ref = |id_hex: &str| RemoteRef {
let normal_new_remote_ref = |id_hex: &str| RemoteRef {
target: normal_ref_target(id_hex),
state: RemoteRefState::New,
};
let normal_tracking_remote_ref = |id_hex: &str| RemoteRef {
target: normal_ref_target(id_hex),
state: RemoteRefState::Tracking,
};
let branch_to_proto =
|name: &str, local_ref_target, remote_branches| crate::protos::op_store::Branch {
Expand All @@ -685,18 +714,26 @@ mod tests {
};

let proto = crate::protos::op_store::View {
branches: vec![branch_to_proto(
"main",
&normal_ref_target("111111"),
vec![
remote_branch_to_proto("git", &normal_ref_target("222222")),
remote_branch_to_proto("gita", &normal_ref_target("333333")),
],
)],
branches: vec![
branch_to_proto(
"main",
&normal_ref_target("111111"),
vec![
remote_branch_to_proto("git", &normal_ref_target("222222")),
remote_branch_to_proto("gita", &normal_ref_target("333333")),
],
),
branch_to_proto(
"untracked",
RefTarget::absent_ref(),
vec![remote_branch_to_proto("gita", &normal_ref_target("777777"))],
),
],
git_refs: vec![
git_ref_to_proto("refs/heads/main", &normal_ref_target("444444")),
git_ref_to_proto("refs/remotes/git/main", &normal_ref_target("555555")),
git_ref_to_proto("refs/remotes/gita/main", &normal_ref_target("666666")),
git_ref_to_proto("refs/remotes/gita/untracked", &normal_ref_target("888888")),
],
has_git_refs_migrated_to_remote: false,
..Default::default()
Expand All @@ -714,12 +751,13 @@ mod tests {
btreemap! {
"git".to_owned() => RemoteView {
branches: btreemap! {
"main".to_owned() => normal_remote_ref("444444"), // refs/heads/main
"main".to_owned() => normal_tracking_remote_ref("444444"), // refs/heads/main
},
},
"gita".to_owned() => RemoteView {
branches: btreemap! {
"main".to_owned() => normal_remote_ref("333333"),
"main".to_owned() => normal_tracking_remote_ref("333333"),
"untracked".to_owned() => normal_new_remote_ref("777777"),
},
},
},
Expand All @@ -729,6 +767,7 @@ mod tests {
btreemap! {
"refs/heads/main".to_owned() => normal_ref_target("444444"),
"refs/remotes/gita/main".to_owned() => normal_ref_target("666666"),
"refs/remotes/gita/untracked".to_owned() => normal_ref_target("888888"),
},
);

Expand Down
9 changes: 7 additions & 2 deletions lib/src/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ use itertools::Itertools;
use crate::backend::CommitId;
use crate::index::Index;
use crate::op_store;
use crate::op_store::{BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, WorkspaceId};
use crate::op_store::{
BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, RemoteRefState, WorkspaceId,
};
use crate::refs::{iter_named_ref_pairs, merge_ref_targets, TrackingRefPair};

#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Debug)]
Expand Down Expand Up @@ -250,7 +252,10 @@ impl View {
if let Some(remote_ref) = remote_view.branches.get_mut(name) {
remote_ref.target = target;
} else {
let remote_ref = RemoteRef { target };
let remote_ref = RemoteRef {
target,
state: RemoteRefState::New,
};
remote_view.branches.insert(name.to_owned(), remote_ref);
}
} else if let Some(remote_view) = self.data.remote_views.get_mut(remote_name) {
Expand Down
Loading

0 comments on commit 4fdafe3

Please sign in to comment.