Skip to content

Feature #875: Delete referring branches #935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Added
- support rebasing branches with conflicts ([#895](https://github.com/extrawurst/gitui/issues/895))
- add supporting deletion of referring branches [[@alessandroasm](https://github.com/alessandroasm)] ([#875](https://github.com/extrawurst/gitui/issues/875))
- add a key binding to stage / unstage items [[@alessandroasm](https://github.com/alessandroasm)] ([#909](https://github.com/extrawurst/gitui/issues/909))
- switch to status tab after merging or rebasing with conflicts ([#926](https://github.com/extrawurst/gitui/issues/926))

Expand Down
160 changes: 160 additions & 0 deletions asyncgit/src/sync/branch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,41 @@ pub fn get_branches_info(
Ok(branches_for_display)
}

/// returns the local branches that track the provided remote branch
pub fn get_branch_trackers(
repo_path: &str,
branch_name: &str,
) -> Result<HashSet<String>> {
let repo = utils::repo(repo_path)?;

let local_branches: HashSet<_> = repo
.branches(Some(BranchType::Local))?
.filter_map(|b| {
let branch = b.ok()?.0;

branch
.upstream()
.ok()
.filter(|upstream| {
let upstream_name =
bytes2string(upstream.get().name_bytes())
.ok();
upstream_name.map_or(false, |upstream_name| {
branch_name == upstream_name
})
})
.and_then(|_| {
branch
.name_bytes()
.ok()
.and_then(|bn| bytes2string(bn).ok())
})
})
.collect();

Ok(local_branches)
}

///
#[derive(Debug, Default)]
pub struct BranchCompare {
Expand Down Expand Up @@ -228,6 +263,28 @@ pub fn get_branch_remote(
}
}

/// returns remote of the upstream tracking branch for `branch` (using branch ref)
pub fn get_branch_remote_by_ref(
repo_path: &str,
branch_ref: &str,
) -> Result<Option<String>> {
let repo = utils::repo(repo_path)?;
let remote_name = repo.branch_upstream_remote(branch_ref).ok();
if let Some(remote_name) = remote_name {
let remote_name_str = bytes2string(remote_name.as_ref())?;
let remote_branch_name =
branch_ref.rsplit('/').next().map(|branch_name| {
format!(
"refs/remotes/{}/{}",
remote_name_str, branch_name
)
});
Ok(remote_branch_name)
} else {
Ok(None)
}
}

/// returns whether the pull merge strategy is set to rebase
pub fn config_is_pull_rebase(repo_path: &str) -> Result<bool> {
let repo = utils::repo(repo_path)?;
Expand Down Expand Up @@ -584,6 +641,98 @@ mod tests_branches {
);
}

#[test]
fn test_branch_remote_trackers() {
let (r1_path, _remote1) = repo_init_bare().unwrap();
let (r2_path, _remote2) = repo_init_bare().unwrap();
let (_r, repo) = repo_init().unwrap();

let r1_path = r1_path.path().to_str().unwrap();
let r2_path = r2_path.path().to_str().unwrap();

//Note: create those test branches in our remotes
clone_branch_commit_push(r1_path, "r1branch");
clone_branch_commit_push(r2_path, "r2branch");

let root = repo.path().parent().unwrap();
let repo_path = root.as_os_str().to_str().unwrap();

//add the remotes
repo.remote("r1", r1_path).unwrap();
repo.remote("r2", r2_path).unwrap();

//pull stuff from the two remotes
debug_cmd_print(repo_path, "git pull r1");
debug_cmd_print(repo_path, "git pull r2");

//create local tracking branches
debug_cmd_print(
repo_path,
"git checkout --track r1/r1branch",
);
debug_cmd_print(
repo_path,
"git checkout --track r2/r2branch",
);

assert_eq!(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move these into a separate unittest that is clearly focused on testing only the new get_branch_trackers fn. even if you copy an existing test but cleanup the assertions that have nothing to do with testing get_branch_trackers - this should simplify readability for later maintenance

get_branch_trackers(
repo_path,
"refs/remotes/r1/r1branch"
)
.unwrap()
.into_iter()
.collect::<Vec<String>>(),
vec![String::from("r1branch")]
);

assert_eq!(
get_branch_trackers(
repo_path,
"refs/remotes/r2/r2branch"
)
.unwrap()
.into_iter()
.collect::<Vec<String>>(),
vec![String::from("r2branch")]
);
}

#[test]
fn test_get_remote_by_ref() {
let (r_path, _remote) = repo_init_bare().unwrap();
let (_r, repo) = repo_init().unwrap();

let r_path = r_path.path().to_str().unwrap();

//Note: create the test branches in our remote
clone_branch_commit_push(r_path, "remotebranch");

let root = repo.path().parent().unwrap();
let repo_path = root.as_os_str().to_str().unwrap();

//add the remote
repo.remote("origin", r_path).unwrap();

//pull stuff from the remotes
debug_cmd_print(repo_path, "git pull origin");

//create local tracking branches
debug_cmd_print(
repo_path,
"git checkout --track origin/remotebranch",
);

assert_eq!(
get_branch_remote_by_ref(
repo_path,
"refs/heads/remotebranch"
)
.unwrap(),
Some(String::from("refs/remotes/origin/remotebranch"))
);
}

#[test]
fn test_branch_remote_no_upstream() {
let (_r, repo) = repo_init().unwrap();
Expand All @@ -603,6 +752,17 @@ mod tests_branches {
let repo_path = root.as_os_str().to_str().unwrap();

assert!(get_branch_remote(repo_path, "foo").is_err());

assert_eq!(
get_branch_trackers(
repo_path,
"refs/remotes/foo/foobranch"
)
.unwrap()
.into_iter()
.collect::<Vec<String>>(),
Vec::<String>::new()
);
}
}

Expand Down
3 changes: 2 additions & 1 deletion asyncgit/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ pub use blame::{blame_file, BlameHunk, FileBlame};
pub use branch::{
branch_compare_upstream, checkout_branch, config_is_pull_rebase,
create_branch, delete_branch, get_branch_remote,
get_branches_info, merge_commit::merge_upstream_commit,
get_branch_remote_by_ref, get_branch_trackers, get_branches_info,
merge_commit::merge_upstream_commit,
merge_ff::branch_merge_upstream_fastforward,
merge_rebase::merge_upstream_rebase, rename::rename_branch,
validate_branch_name, BranchCompare, BranchInfo,
Expand Down
33 changes: 33 additions & 0 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,7 @@ impl App {
Ok(flags)
}

#[allow(clippy::too_many_lines)]
fn process_confirmed_action(
&mut self,
action: Action,
Expand Down Expand Up @@ -769,6 +770,11 @@ impl App {
flags.insert(NeedsUpdate::ALL);
}
Action::DeleteLocalBranch(branch_ref) => {
let upstream =
sync::get_branch_remote_by_ref(CWD, &branch_ref)
.ok()
.flatten();

if let Err(e) = sync::delete_branch(CWD, &branch_ref)
{
self.queue.push(InternalEvent::ShowErrorMsg(
Expand All @@ -777,6 +783,13 @@ impl App {
}
flags.insert(NeedsUpdate::ALL);
self.select_branch_popup.update_branches()?;

// If we have a remote, we must ask if user wants to delete it
if let Some(upstream) = upstream {
self.queue.push(InternalEvent::ConfirmAction(
Action::DeleteUpstreamBranch(upstream),
));
}
}
Action::DeleteRemoteBranch(branch_ref) => {
self.queue.push(
Expand All @@ -800,6 +813,26 @@ impl App {
flags.insert(NeedsUpdate::ALL);
self.select_branch_popup.update_branches()?;
}
Action::DeleteUpstreamBranch(upstream) => {
self.queue.push(InternalEvent::ConfirmedAction(
Action::DeleteRemoteBranch(upstream),
));
}
Action::DeleteTrackingBranches(branches) => {
for branch in &branches {
let branch_ref = format!("refs/heads/{}", branch);
if let Err(e) =
sync::delete_branch(CWD, &branch_ref)
{
self.queue.push(InternalEvent::ShowErrorMsg(
e.to_string(),
));
}
}

flags.insert(NeedsUpdate::ALL);
self.select_branch_popup.update_branches()?;
}
Action::DeleteTag(tag_name) => {
if let Err(error) = sync::delete_tag(CWD, &tag_name) {
self.queue.push(InternalEvent::ShowErrorMsg(
Expand Down
34 changes: 32 additions & 2 deletions src/components/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
CommandInfo, Component, DrawableComponent, EventState,
},
keys::SharedKeyConfig,
queue::{InternalEvent, Queue},
queue::{Action, InternalEvent, Queue},
strings,
ui::{self, style::SharedTheme},
};
Expand All @@ -15,13 +15,14 @@ use asyncgit::{
extract_username_password, need_username_password,
BasicAuthCredential,
},
get_branch_remote, get_default_remote,
get_branch_remote, get_branch_trackers, get_default_remote,
},
AsyncGitNotification, AsyncPush, PushRequest, RemoteProgress,
RemoteProgressState, CWD,
};
use crossbeam_channel::Sender;
use crossterm::event::Event;
use std::collections::HashSet;
use tui::{
backend::Backend,
layout::Rect,
Expand Down Expand Up @@ -60,6 +61,7 @@ pub struct PushComponent {
theme: SharedTheme,
key_config: SharedKeyConfig,
input_cred: CredComponent,
tracking_branches: Option<Vec<String>>,
}

impl PushComponent {
Expand All @@ -84,6 +86,7 @@ impl PushComponent {
),
theme,
key_config,
tracking_branches: None,
}
}

Expand Down Expand Up @@ -141,6 +144,19 @@ impl PushComponent {
remote
};

self.tracking_branches = if self.modifier.delete() {
let remote_ref =
format!("refs/remotes/{}/{}", remote, self.branch);
Some(
get_branch_trackers(CWD, &remote_ref)
.unwrap_or_else(|_| HashSet::new())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need an empty hashset here? can't we go with None in case there was a problem?

.into_iter()
.collect(),
)
} else {
None
};

self.pending = true;
self.progress = None;
self.git_push.request(PushRequest {
Expand Down Expand Up @@ -175,6 +191,20 @@ impl PushComponent {
self.queue.push(InternalEvent::ShowErrorMsg(
format!("push failed:\n{}", err),
));
} else if self.modifier.delete() {
// Check if we need to delete the tracking branches
let tracking_branches = self
.tracking_branches
.take()
.unwrap_or_else(Vec::new);

if !tracking_branches.is_empty() {
self.queue.push(InternalEvent::ConfirmAction(
Action::DeleteTrackingBranches(
tracking_branches.into_iter().collect(),
),
));
}
}
self.hide();
}
Expand Down
18 changes: 18 additions & 0 deletions src/components/reset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,24 @@ impl ConfirmComponent {
branch_ref,
),
),
Action::DeleteUpstreamBranch(upstream) => (
strings::confirm_title_delete_remote_branch(
&self.key_config,
),
strings::confirm_msg_delete_referring_remote_branch(
&self.key_config,
upstream,
),
),
Action::DeleteTrackingBranches(local_branches) => (
strings::confirm_title_delete_branch(
&self.key_config,
),
strings::confirm_msg_delete_tracking_branches(
&self.key_config,
local_branches,
),
),
Action::DeleteTag(tag_name) => (
strings::confirm_title_delete_tag(
&self.key_config,
Expand Down
2 changes: 2 additions & 0 deletions src/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ pub enum Action {
ResetLines(String, Vec<DiffLinePosition>),
StashDrop(Vec<CommitId>),
StashPop(CommitId),
DeleteUpstreamBranch(String),
DeleteTrackingBranches(Vec<String>),
DeleteLocalBranch(String),
DeleteRemoteBranch(String),
DeleteTag(String),
Expand Down
Loading