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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Feature #875: Delete referring branches #935

wants to merge 4 commits into from

Conversation

alessandroasm
Copy link
Contributor

@alessandroasm alessandroasm commented Oct 10, 2021 •
edited
Loading

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
@alessandroasm
Feature gitui-org#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 alessandroasm mentioned this pull request Oct 10, 2021
Open
extrawurst
extrawurst requested changes Oct 11, 2021
View reviewed changes
Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

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
@alessandroasm
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
Copy link
Contributor Author

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 🤦‍♂️
@alessandroasm alessandroasm marked this pull request as ready for review October 11, 2021 12:38
extrawurst
extrawurst reviewed Oct 13, 2021
View reviewed changes
asyncgit/src/sync/branch/mod.rs
@@ -582,6 +617,28 @@ mod tests_branches {
.unwrap(),
String::from("r2")
);

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
extrawurst
extrawurst reviewed Oct 13, 2021
View reviewed changes
src/app.rs Outdated
@@ -769,6 +770,21 @@ impl App {
flags.insert(NeedsUpdate::ALL);
}
Action::DeleteBranch(branch_ref, true) => {
let upstream = branch_ref
.rsplit('/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
extrawurst
extrawurst reviewed Oct 13, 2021
View reviewed changes
src/strings.rs Outdated
branches_ref: &[String],
) -> String {
format!(
"Do you want to delete the referring tracking branches: {} ?",
Copy link
Collaborator

Choose a reason for hiding this comment

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

since it is a list and we have a multiline message box, lets list the branch names one per line after the question
extrawurst
extrawurst reviewed Oct 13, 2021
View reviewed changes
extrawurst
extrawurst requested changes Oct 13, 2021
View reviewed changes
Copy link
Collaborator

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

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
@alessandroasm
Merge remote-tracking branch 'origin/master' into 875-delete-referrin…
a2c14ac 
…g-branches
@alessandroasm
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`
@alessandroasm
Copy link
Contributor Author

I've made the changes requested.
Please have a look at my reply about Action::DeleteRemoteBranch.
extrawurst
extrawurst reviewed Oct 23, 2021
View reviewed changes
src/components/push.rs
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?
@extrawurst
Copy link
Collaborator

marking as Draft until the remaining comments are addressed
@extrawurst extrawurst marked this pull request as draft November 21, 2021 21:20
@extrawurst
Copy link
Collaborator

@alessandroasm are you still planning to move this forward?
@stale Stale
Copy link

stale bot commented Mar 17, 2024

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.
@stale stale bot added the dormant Marked by stale bot on close label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dormant Marked by stale bot on close
2 participants
@alessandroasm @extrawurst