Feature #875: Delete referring branches #935

alessandroasm · 2021-10-10T13:08:01Z

This Pull Request fixes/closes #875.

It changes the following:

After deleting a branch, check if there are any upstream / tracking branches and ask if user wants to delete them as well.

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog

After deleting a branch, check if there are any upstream / tracking branches and ask if user wants to delete them as well.

asyncgit/src/sync/branch/mod.rs

extrawurst

also please undo the function split up and silence the too-many-lines warning for now via a annotation. we should do refactorings as separat changes

- Adding tests of `get_branch_trackers` in the existing unittests - Adding clippy annotations to disable "too many lines" error on `process_confirmed_action` - Fixing upstream ref string when deleting a local branch

alessandroasm · 2021-10-11T12:38:13Z

Hello @extrawurst, thanks for the quick review and suggestions. I've just updated the PR with the following:

Reverted the refactoring and added the annotation to disable clippy::too_many_lines error
Added some assertions for get_branch_trackers in the existing unittests
Fixed the string passed to Action::DeleteBranch. I was passing the branch name instead of a full ref 🤦‍♂️

extrawurst · 2021-10-13T09:32:51Z

asyncgit/src/sync/branch/mod.rs

@@ -582,6 +617,28 @@ mod tests_branches {
 				.unwrap(),
 			String::from("r2")
 		);
+
+		assert_eq!(


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

extrawurst · 2021-10-13T09:37:23Z

src/app.rs

@@ -769,6 +770,21 @@ impl App {
 				flags.insert(NeedsUpdate::ALL);
 			}
 			Action::DeleteBranch(branch_ref, true) => {
+				let upstream = branch_ref
+					.rsplit('/')


lets move this convert ref to branch name code into asyncgit (maybe as a get_branch_remote_by_ref) to be able to unittest it. I think rsplit is incorrect here. I fixed another issue with this the other day. check out c6abbaf

src/components/reset.rs

extrawurst · 2021-10-13T09:45:00Z

src/strings.rs

+	branches_ref: &[String],
+) -> String {
+	format!(
+		"Do you want to delete the referring tracking branches: {} ?",


since it is a list and we have a multiline message box, lets list the branch names one per line after the question

src/app.rs

extrawurst

added a few comments. please bring it up to date with master, I refactored a the branch deletion to better distinguish local vs remote branch delete

…g-branches

- A new method was created in asyncgit to find a upstream branch, given the local branch ref (`get_branch_remote_by_ref`) - Added unit tests for `get_branch_remote_by_ref` and `get_branch_trackers`

alessandroasm · 2021-10-14T12:27:22Z

I've made the changes requested.
Please have a look at my reply about Action::DeleteRemoteBranch.

extrawurst · 2021-10-23T13:45:13Z

src/components/push.rs

+				format!("refs/remotes/{}/{}", remote, self.branch);
+			Some(
+				get_branch_trackers(CWD, &remote_ref)
+					.unwrap_or_else(|_| HashSet::new())


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

extrawurst · 2021-11-21T21:20:02Z

marking as Draft until the remaining comments are addressed

extrawurst · 2022-12-29T23:23:47Z

@alessandroasm are you still planning to move this forward?

stale · 2024-03-17T15:20:58Z

This issue has been automatically marked as stale because it has not had any activity half a year. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

Feature extrawurst#875: Delete referring branches

b26eddf

After deleting a branch, check if there are any upstream / tracking branches and ask if user wants to delete them as well.

alessandroasm mentioned this pull request Oct 10, 2021

Branch-delete: Ask to delete tracking branch #875

Open

extrawurst reviewed Oct 10, 2021

View reviewed changes

asyncgit/src/sync/branch/mod.rs Show resolved Hide resolved

extrawurst requested changes Oct 11, 2021

View reviewed changes

Adding tests and clippy annotation

4f44f77

- Adding tests of `get_branch_trackers` in the existing unittests - Adding clippy annotations to disable "too many lines" error on `process_confirmed_action` - Fixing upstream ref string when deleting a local branch

alessandroasm marked this pull request as ready for review October 11, 2021 12:38

alessandroasm requested a review from extrawurst October 12, 2021 11:36

extrawurst reviewed Oct 13, 2021

View reviewed changes

src/components/reset.rs Show resolved Hide resolved

extrawurst reviewed Oct 13, 2021

View reviewed changes

src/app.rs Show resolved Hide resolved

extrawurst requested changes Oct 13, 2021

View reviewed changes

alessandroasm added 2 commits October 13, 2021 07:44

Merge remote-tracking branch 'origin/master' into 875-delete-referrin…

a2c14ac

…g-branches

Refactoring of process_confirmed_action

e862988

- A new method was created in asyncgit to find a upstream branch, given the local branch ref (`get_branch_remote_by_ref`) - Added unit tests for `get_branch_remote_by_ref` and `get_branch_trackers`

extrawurst reviewed Oct 23, 2021

View reviewed changes

extrawurst marked this pull request as draft November 21, 2021 21:20

stale bot added the dormant Marked by stale bot on close label Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature #875: Delete referring branches #935

Feature #875: Delete referring branches #935

alessandroasm commented Oct 10, 2021 •

edited

extrawurst left a comment

alessandroasm commented Oct 11, 2021

extrawurst Oct 13, 2021

extrawurst Oct 13, 2021

extrawurst Oct 13, 2021

extrawurst left a comment

alessandroasm commented Oct 14, 2021

extrawurst Oct 23, 2021

extrawurst commented Nov 21, 2021

extrawurst commented Dec 29, 2022

stale bot commented Mar 17, 2024

Feature #875: Delete referring branches #935

Are you sure you want to change the base?

Feature #875: Delete referring branches #935

Conversation

alessandroasm commented Oct 10, 2021 • edited

extrawurst left a comment

Choose a reason for hiding this comment

alessandroasm commented Oct 11, 2021

extrawurst Oct 13, 2021

Choose a reason for hiding this comment

extrawurst Oct 13, 2021

Choose a reason for hiding this comment

extrawurst Oct 13, 2021

Choose a reason for hiding this comment

extrawurst left a comment

Choose a reason for hiding this comment

alessandroasm commented Oct 14, 2021

extrawurst Oct 23, 2021

Choose a reason for hiding this comment

extrawurst commented Nov 21, 2021

extrawurst commented Dec 29, 2022

stale bot commented Mar 17, 2024

alessandroasm commented Oct 10, 2021 •

edited