From f1adfd5a6f598a5eb08d39e57653b2c601595c34 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 27 Sep 2023 20:56:59 +0900 Subject: [PATCH 1/3] git: on import_refs(), don't preserve old branches referenced by remote refs If we made @git branches real .remote_targets["git"], remotely-rewritten commits could also be pinned by the @git branches, and therefore wouldn't be abandoned. We could exclude the "git" remote, but I don't think local commits should be pinned by remote refs in general. If we squashed a fetched commit, remote ref would point to a hidden commit anyway. --- lib/src/git.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 582bf4ee1c..db2f823489 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -356,15 +356,16 @@ fn diff_refs_to_import( Ok(changed_git_refs) } -/// Commits referenced by local/remote branches, tags, or HEAD@git. +/// Commits referenced by local branches, tags, or HEAD@git. /// /// On `import_refs()`, this is similar to collecting commits referenced by /// `view.git_refs()`. Main difference is that local branches can be moved by /// tracking remotes, and such mutation isn't applied to `view.git_refs()` yet. fn pinned_commit_ids(view: &View) -> impl Iterator { - let branch_ref_targets = view.branches().values().flat_map(|branch_target| { - iter::once(&branch_target.local_target).chain(branch_target.remote_targets.values()) - }); + let branch_ref_targets = view + .branches() + .values() + .map(|branch_target| &branch_target.local_target); itertools::chain!( branch_ref_targets, view.tags().values(), From 079aeaa875dcc534ad58b11d9382ca67f4047f29 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 21 Sep 2023 18:49:27 +0900 Subject: [PATCH 2/3] view: extract method that iterates local branches only I'll probably reorganize the local/remote branches structure, so let's minimize call sites which rely on the BranchTarget struct. --- cli/src/commands/mod.rs | 10 ++++++---- lib/src/git.rs | 10 ++-------- lib/src/repo.rs | 5 ++--- lib/src/rewrite.rs | 6 +++--- lib/src/view.rs | 9 +++++++++ 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index df48038ac1..15394375dc 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -1610,12 +1610,14 @@ fn cmd_status( formatter.write_str("No working copy\n")?; } - let mut conflicted_local_branches = vec![]; + let conflicted_local_branches = repo + .view() + .local_branches() + .filter(|(_, target)| target.has_conflict()) + .map(|(branch_name, _)| branch_name) + .collect_vec(); let mut conflicted_remote_branches = vec![]; for (branch_name, branch_target) in repo.view().branches() { - if branch_target.local_target.has_conflict() { - conflicted_local_branches.push(branch_name.clone()); - } for (remote_name, remote_target) in &branch_target.remote_targets { if remote_target.has_conflict() { conflicted_remote_branches.push((branch_name.clone(), remote_name.clone())); diff --git a/lib/src/git.rs b/lib/src/git.rs index db2f823489..643f7c489a 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -362,12 +362,8 @@ fn diff_refs_to_import( /// `view.git_refs()`. Main difference is that local branches can be moved by /// tracking remotes, and such mutation isn't applied to `view.git_refs()` yet. fn pinned_commit_ids(view: &View) -> impl Iterator { - let branch_ref_targets = view - .branches() - .values() - .map(|branch_target| &branch_target.local_target); itertools::chain!( - branch_ref_targets, + view.local_branches().map(|(_, target)| target), view.tags().values(), iter::once(view.git_head()), ) @@ -852,9 +848,7 @@ pub fn fetch( // help branch forget`. let nonempty_branches: HashSet<_> = mut_repo .view() - .branches() - .iter() - .filter(|&(_branch, target)| target.local_target.is_present()) + .local_branches() .map(|(branch, _target)| branch.to_owned()) .collect(); // TODO: Inform the user if the export failed? In most cases, export is not diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 72009eec66..88880d0cb7 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -872,9 +872,8 @@ impl MutableRepo { commit: &Commit, ) -> Result<(), EditCommitError> { fn local_branch_target_ids(view: &View) -> impl Iterator { - view.branches() - .values() - .flat_map(|branch_target| branch_target.local_target.added_ids()) + view.local_branches() + .flat_map(|(_, target)| target.added_ids()) } let maybe_wc_commit_id = self diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 042448aea2..07794ecde0 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -233,12 +233,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { // Build a map from commit to branches pointing to it, so we don't need to scan // all branches each time we rebase a commit. let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); - for (branch_name, branch_target) in mut_repo.view().branches() { - for commit in branch_target.local_target.added_ids() { + for (branch_name, target) in mut_repo.view().local_branches() { + for commit in target.added_ids() { branches .entry(commit.clone()) .or_default() - .insert(branch_name.clone()); + .insert(branch_name.to_owned()); } } diff --git a/lib/src/view.rs b/lib/src/view.rs index 8638886550..0311d68d13 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -160,6 +160,15 @@ impl View { self.data.branches.remove(name); } + /// Iterates local branch `(name, target)`s in lexicographical order. + pub fn local_branches(&self) -> impl Iterator { + self.data + .branches + .iter() + .map(|(name, branch_target)| (name.as_ref(), &branch_target.local_target)) + .filter(|(_, target)| target.is_present()) + } + pub fn get_local_branch(&self, name: &str) -> &RefTarget { if let Some(branch_target) = self.data.branches.get(name) { &branch_target.local_target From c7c2ce93862857e04ea553bbbf09e8c39b2f64ab Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 29 Sep 2023 13:21:52 +0900 Subject: [PATCH 3/3] view: add method that iterates remote branches only There aren't many callers, but let's add it for consistency. --- cli/src/commands/mod.rs | 14 ++++++-------- lib/src/view.rs | 11 +++++++++++ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 15394375dc..9762ae5e84 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -1616,14 +1616,12 @@ fn cmd_status( .filter(|(_, target)| target.has_conflict()) .map(|(branch_name, _)| branch_name) .collect_vec(); - let mut conflicted_remote_branches = vec![]; - for (branch_name, branch_target) in repo.view().branches() { - for (remote_name, remote_target) in &branch_target.remote_targets { - if remote_target.has_conflict() { - conflicted_remote_branches.push((branch_name.clone(), remote_name.clone())); - } - } - } + let conflicted_remote_branches = repo + .view() + .remote_branches() + .filter(|(_, target)| target.has_conflict()) + .map(|(full_name, _)| full_name) + .collect_vec(); if !conflicted_local_branches.is_empty() { writeln!( formatter.labeled("conflict"), diff --git a/lib/src/view.rs b/lib/src/view.rs index 0311d68d13..9d90f3cf76 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -201,6 +201,17 @@ impl View { } } + /// Iterates remote branch `((name, remote_name), target)`s in + /// lexicographical order. + pub fn remote_branches(&self) -> impl Iterator { + self.data.branches.iter().flat_map(|(name, branch_target)| { + branch_target + .remote_targets + .iter() + .map(|(remote_name, target)| ((name.as_ref(), remote_name.as_ref()), target)) + }) + } + pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> &RefTarget { if let Some(branch_target) = self.data.branches.get(name) { let maybe_target = branch_target.remote_targets.get(remote_name);