Skip to content
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

Delete many branches locally #4059

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waelmahrous
Copy link

  • PR Description
    This PR will allow the user to delete multiple branches at the same time. See Delete Multiple Branches #4041.
  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

{
Key: opts.GetKey(opts.Config.Universal.RemoveMany),
Handler: self.withItem(self.deleteMultiple),
GetDisabledReason: self.require(self.itemsSelected(self.branchesAreReal)),
Copy link

Choose a reason for hiding this comment

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

IMO introducing a new "RemoveMany" command might not be strictly useful. I think we should also enable deleting multiple branches using multi-select

Copy link
Owner

Choose a reason for hiding this comment

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

I agree: there's no need for a new keybinding here; we just need to do something different if the user presses the usual 'remove' key when multiple branches are selected

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree as well, but it's not easy at all (the "just" in your comment suggests that it might be).

The bespoke extra command that's added in this PR deletes only local branches; this is easy to do, and if it's an extra command, it could be justifiable if the command name and tooltip makes this very clear (which this PR doesn't). However, if we include the multi-select functionality in the normal delete menu, then I'd expect this to work the same as for a single selection:

  • it should offer to delete only the local branches, or only the remote branches, or both (the latter two should be disabled if not all of the selected branches have upstream branches)
  • it should prompt for confirmation if some of the local branches aren't fully merged

As an intermediate solution that might be good enough for now, and will be a lot easier, we could simply disable the remoteDeleteItem and deleteBothItem in case there's a multiselection, with a DisabledReason saying that this is not supported for multiselections.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, turns out that it's not that hard (sometimes you have to try it to find out). Here's a PR: #4073

Basically, the main change is that whenever we make a decision, we replace that with either lo.Every or lo.Some on the selected branches. The most tricky part was deleting several remote branches: we have to group them by remote and then loop over the remotes and issue one push --delete call with the selected branches for that remote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants