Allow deleting remote tags/branches from local tag/branch views

[AzraelSec]

PR Description
This PR allows users to remove remote tags using a new ad-hoc UI. Users can now delete remote tags using an interactive menu that will enable them to remove the local tags, remote tags, or both.

In addition, the UI for branch deletion has been changed to align with the tag deletion interface.

Note: A single git command to remove both local and remote branches (and tags) does not exist (correct me if I'm wrong); in case of a complete deletion, lazygit will try to remove the local branch before the remote one.

Note: The pre-existing confirmation popup is still shown in case of a required branch force deletion.

Closes #2635

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[jesseduffield]

Nice work! Sorry for the belated review.

A couple things:

• We know ahead of time whether a branch has an upstream so I would only show the option to delete a remote branch if the branch actually has an upstream
• I'm starting to think perhaps the local+remote option might be too complicated. With branches we add an extra confirmation if the local branch is not fully merged into the main branch. It feels weird to include that check in the local+remote option if it only pertains to the local branch. If we didn't have the local+remote option, but we wanted to remove both, we'd first remove the remote branch, then the local branch, and this does admittedly feel a bit clunky. I'm undecided. What do you think @AzraelSec (also tagging @mark2185 and @stefanhaller )

[jesseduffield]

We don't want to hardcode origin as the remote here because the user may want to delete the tag from another remote. We should prompt the user to select the remote first (We do this already in func (self *TagsController) push so you can copy that approach)

[jesseduffield]

Hang on, we should be able to get the upstream remote and remote branch from the branch itself, so there's no need for the user to specify the remote!

[AzraelSec]

@jesseduffield I've managed to retrieve the current branch using the GetCheckedOutRef method from the RefsHelper struct. Is this what you were thinking about?

I'm not 100% sure, but if you can derive the tag upstream remote from the branch while deleting it, shouldn't you be able to do the same while pushing it? In contrast, if you can choose a different upstream for a tag, couldn't it be different from the branch one?

This feature is obviously much more straightforward for the branch controller.

[jesseduffield]

GetCheckedOutRef won't work because we want the currently selected branch, so depending on where you are it would be self.c.Contexts().Branches.GetSelected()

I'm not sure what you mean re: pushing a branch. If we already have an upstream for the branch it shouldn't prompt you for the upstream when you go to push.

I'm also not sure what you mean regarding the tag. Tags don't maintain an upstream like branches do.

[AzraelSec]

Sorry @jesseduffield, I misspoke - let me better clarify my doubt.

Your comment suggested inferring the remote for tags (on deletion) using the information of the branch it belongs to. The relationship between the remote on which I perform the tag storage and the upstream remote of the branch it references is unclear to me.

The user can push the branch and the tag to different remotes, right?

[jesseduffield]

Sorry I meant that we can infer the remote from the branch. As for tags, tags aren't associated to branches or upstream so we would need to show the remote prompt for tags

[AzraelSec]

Alright, thanks! 🙏🏻

[jesseduffield]

Suggested change

	Select(Equals("l Delete local branch")).
	Select(Contains("Delete local branch")).

I prefer excluding the keybinding because we could change how we display those over time and that would break the test unnecessarily

[jesseduffield]

Can we also test that the remote branches view no longer shows the remote branch?

[mark2185]

If we didn't have the local+remote option, but we wanted to remove both, we'd first remove the remote branch, then the local branch, and this does admittedly feel a bit clunky.

I'd definitely have it as two separate steps with each having its own "are you sure?" prompt.

As for having it under one keymap, although I agree it would be nice to do it in one fell swoop, I do wonder how soon will we have a "help I deleted everything" issue cropping up.

[AzraelSec]

Hey @jesseduffield, @mark2185 👋🏻 No problem with the delay in the review - take your time, no rush!

We know ahead of time whether a branch has an upstream so I would only show the option to delete a remote branch if the branch actually has an upstream

You're right; I'll try to implement this behaviour.

I'm starting to think perhaps the local+remote option might be too complicated. With branches we add an extra confirmation if the local branch is not fully merged into the main branch. It feels weird to include that check in the local+remote option feels weird if it only pertains to the local branch. If we didn't have the local+remote option, but we wanted to remove both, we'd first remove the remote branch, then the local branch, and this does admittedly feel a bit clunky. I'm undecided. What do you think @AzraelSec (also tagging @mark2185 and @stefanhaller )

I honestly agree with both of you: having a shortcut for removing a branch from everywhere could be handy, but the pitfalls would be more than the advantages. There's a pretty high risk of messing things up while working in a rush - I'd remove this option. Additionally, considering it's just a shortcut that executes the local and remote branch removal in sequence, adding it in the future would be a manageable effort.

To recap:

1. I'll introduce a selection menu without the possibility of removing a local+remote branch/tag at the same time
2. I'll introduce a confirmation dialog also for the remote removal actions
3. I'll address the comments left here on the PR

This will probably take a day or two, depending on how busy I'll be at work. If it's okay with you, I'll nudge you when the PR is ready for review again: I'd reduce the risk of running into conflicts, considering the number of files this branch touches.

[jesseduffield]

Sounds good!

[AzraelSec]

Sorry for the delay, @jesseduffield. I should have addressed all your comments, but let me know if this needs any additional change. Thanks!

[jesseduffield]

Nice work. I've got some thoughts on the UX (haven't looked at the code yet)

Branches:

• Something I want is to support muscle memory, so rather than show a menu conditional on whether a branch is tracking an upstream, we should unconditionally show the menu. We should however hide the menu item for deleting the remote branch if there is no remote branch.
• Currently we get an error pressing 'd' on the checked out branch, but we may have a legitimate need to delete the remote for that branch. So I'd show the menu with both options (again, for the sake of muscle memory) but show the error if the first item (for deleting the local branch) is invoked
• When deleting a branch's remote I would also unset the remote similar to what we do via pressing 'u' on the branch
• In the confirmation prompt for deleting the remote branch we should show both the remote and remote branch name. In fact I'd also show that in the menu item to be extra clear.

Tags:

• We should remove the 'the' from 'Delete local tag' and 'Delete a remote tag', for consistency with the branch menu and because it's more like 'delete a remote tag'
• Can we add 'OpensMenu: true' to the remote branch menu item to signify you'll need to type in a remote tag? OpensMenu is not the best name here, it's more like OpensPrompt but it's the same idea.

Confirmation popups:
So far we've had a few times where we've replaced a confirmation prompt with a menu and to keep the number of keypresses the same, we remove the confirmtion prompt. The idea being that if the user presses a key accidentally, the menu itself acts as a confirmation prompt. So the question is: do we do the same here? Deleting a local branch is something I do all the time, so I'd prefer it to only require two keystrokes. It's also fairly easy to reverse by pressing 'z'. Deleting remote branches, on the other hand, is irreversible. So I think we should have a confirmation prompt for remote branches but not for local branches.

Lemme know your thoughts.

[jesseduffield]

We wanna avoid using hjkl here cos those are used for navigation. What to use instead... how about 'c' (completely arbitrary)

[AzraelSec]

@jesseduffield Branches:

• Something I want is to support muscle memory, so rather than show a menu conditional on whether a branch is tracking an upstream, we should unconditionally show the menu. We should however hide the menu item for deleting the remote branch if there is no remote branch.

It seems reasonable, especially if showing the menu will prevent us from overcomplicating the UX with the confirmation popup for the local deletion. A menu with a single option could sound quite odd, though, and probably a "disabled" flavour for the unavailable items would be better (do you see it viable for the future?). Anyway, I agree with you on focusing on muscular memory in the short term and always showing the menu - even if one of the options is not viable.

• Currently we get an error pressing 'd' on the checked out branch, but we may have a legitimate need to delete the remote for that branch. So I'd show the menu with both options (again, for the sake of muscle memory) but show the error if the first item (for deleting the local branch) is invoked

To be consistent with what we said in the previous point, wouldn't it be better to show only the remote menu item in this case? Otherwise, I'd propose to keep the remote deletion option even if the branch has no remote upstream (see the previous point).

• When deleting a branch's remote I would also unset the remote similar to what we do via pressing 'u' on the branch

I suggest keeping the remote branch deletion and upstream unsetting separate operations. I don't have many arguments, but I don't see many advantages in unifying them in a single operation. I'd allow the user to choose between just deleting the remote branch and unsetting its upstream. In your opinion, what's the advantage of merging them?

• In the confirmation prompt for deleting the remote branch we should show both the remote and remote branch name. In fact I'd also show that in the menu item to be extra clear.

I agree 👍 I don't see an easy way of showing the local+remote branch name inside the menu item, though. I fear it would get too much space. Would it work for you to change just the prompt?

Confirmation popups: So far we've had a few times where we've replaced a confirmation prompt with a menu and to keep the number of keypresses the same, we remove the confirmtion prompt. The idea being that if the user presses a key accidentally, the menu itself acts as a confirmation prompt. So the question is: do we do the same here? Deleting a local branch is something I do all the time, so I'd prefer it to only require two keystrokes. It's also fairly easy to reverse by pressing 'z'. Deleting remote branches, on the other hand, is irreversible. So I think we should have a confirmation prompt for remote branches but not for local branches.

I agree with you: I introduced the confirmation popup for the local branch deletion because it was the outcome of our last discussion. However, it is, IMO, a bit clucky UX for the frequent+reversible operations. I'm happy to change the code to keep the confirmation popup only for the remote deletion process.

[AzraelSec]

@jesseduffield, instead of printing an error message when the user tries to delete the local checked-out branch, would it work for you to use the item tooltip to notify that the operation is unavailable? Considering we don't have a way of making a menu item unclickable (as far as I know), I'm attaching to it a handler that just returns, but in the future, it would be nice to disable the option, as I mentioned in the previous comment. What do you think?

[jesseduffield]

@AzraelSec sorry that I haven't gotten back to this yet. I'll try to get to it soon!

[AzraelSec]

@AzraelSec sorry that I haven't gotten back to this yet. I'll try to get to it soon!

Don't worry, @jesseduffield. Take your time - I'll rebase this branch on master today at lunch, and I'll solve the conflicts so you can review an updated version.

[jesseduffield]

Sorry this took me so long!

A couple things:

• I'd like to have a concept of a disabled menu item which you can't even hit space/enter on, with the label being styled with strikethrough. Currently when you try to delete the local checked out branch nothing happens which would surprise the user if the didn't read the tooltip. I reckon for this PR we should just show the error and we can worry about the concept of a disabled menu item later.
• When deleting a remote tag, there's no sign that the tag was actually deleted. When there's no sign that something has happened (other than the loader stopping) we should show a toast notification using self.c.Toast(...) e.g. printing 'Remote tag deleted'. Note that due to the way our tests run, you can't actually assert on the presence of the toast text so don't even try! It would be good, however, to assert that the remote tag is gone. We don't actually show remote tags in lazygit atm so I think we should have an assertion which asks git directly. We could put such an assertion function in pkg/integration/components/git.go

[jesseduffield]

Suggested change

	Description: "Try all the combination of local and remote branch deletions",
	Description: "Try all combinations of local and remote branch deletions",

[jesseduffield]

Could we add a method to MenuDriver for this? Something like:

func (self *MenuDriver) Tooltip(option *TextMatcher) *MenuDriver {
	self.t.Views().Tooltip().Content(option)

	return self
}

Cos we'll pretty much only need this when dealing with menus

[jesseduffield]

Suggested change

	SelectRemoteTagUpstream: "Remote from which remove tag '{{.tagName}}':",
	SelectRemoteTagUpstream: "Remote from which to remove tag '{{.tagName}}':",

[AzraelSec]

No worries, @jesseduffield 💪🏻

• I'd like to have a concept of a disabled menu item which you can't even hit space/enter on, with the label being styled with strikethrough. Currently when you try to delete the local checked out branch nothing happens which would surprise the user if the didn't read the tooltip. I reckon for this PR we should just show the error and we can worry about the concept of a disabled menu item later.

You're right; I'll introduce a simple error popup. Do you want me to remove the tooltip as well, or do you prefer to keep it?

[jesseduffield]

Let's keep the tooltip

[AzraelSec]

@jesseduffield, this should be ready to be reviewed again. Let me know if there's anything else I can do to improve the UX or the code itself. Thanks!

[jesseduffield]

Great work @AzraelSec you've got a knack for this! Lemme know if you're interested in picking up another issue :)

[AzraelSec]

Thank you, @jesseduffield! Yup, I'll take a look at the open issues and pick up something else - let me know if you have something you want me to take care of 💪🏻