Skip to content

Fix deleting a remote tag when a remote branch with the same name exists, or vice versa #5075

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

Merged
merged 4 commits into from
Nov 28, 2025
+105 −3
Merged

Fix deleting a remote tag when a remote branch with the same name exists, or vice versa #5075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19a4454
Cleanup: remove unnecessary keypress
stefanhaller Nov 27, 2025
4d4b143
Add tests demonstrating the problem
stefanhaller Nov 27, 2025
7809823
Fix deleting a remote branch when a remote tag with the same name exists
stefanhaller Nov 27, 2025
11a6a73
Fix deleting a remote tag when a remote branch with the same name exists
stefanhaller Nov 27, 2025
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
5 changes: 3 additions & 2 deletions pkg/commands/git_commands/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"strings"

"github.com/jesseduffield/gocui"
"github.com/samber/lo"
)

type RemoteCommands struct {
Expand Down Expand Up @@ -52,15 +53,15 @@ func (self *RemoteCommands) UpdateRemoteUrl(remoteName string, updatedUrl string
func (self *RemoteCommands) DeleteRemoteBranch(task gocui.Task, remoteName string, branchNames []string) error {
cmdArgs := NewGitCmd("push").
Arg(remoteName, "--delete").
Arg(branchNames...).
Arg(lo.Map(branchNames, func(b string, _ int) string { return "refs/heads/" + b })...).
Copy link

@miduddin miduddin Nov 28, 2025

Choose a reason for hiding this comment

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

I noticed that now we have two lo.Map calls, one here and one on the caller of DeleteRemoteBranch. Is it preferable to have these two joined together?

Also, the caller works with the type models.RemoteBranch, there is a FullRefName() method that returns "refs/remotes/<remote_name>/<branch_name>" which unfortunately can't be used for this, but on the tag side that works with models.Tag the FullRefName() can be used. It's a bit weird from the consistency standpoint so I don't know if we want to use them without some extra work, but I figured to just mention it.

Copy link
Collaborator Author

@stefanhaller stefanhaller Nov 28, 2025

Choose a reason for hiding this comment

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

Thanks for the careful review, both are good observations.

  1. No, I think it's fine this way; the fact that we have two lo.Map calls doesn't bother me, and it helps with having clearer APIs. The alternatives would be: a) pass an instance of models.RemoteBranch to RemoteCommands.DeleteRemoteBranch, so that it can do both transformations inside; that's not possible, because git_commands shouldn't have a dependency on the models. Or b) require the caller to pass in the full ref name, but callers shouldn't have to have this knowledge, it's a clearer API to only pass in the name.

  2. We can't use FullRefName() anyway because we only want to pass in the bare names, but even if we could, we shouldn't use FullRefName() here. It's the full ref name from the perspective of the local repo, which is not what we need here. We need it from the perspective of the remote repo, and it's the responsibility of the DeleteRemoteTag/DeleteRemoteBranch functions to know how to construct these. It's true that for tags both are the same, but that's only because git doesn't have a concept of remote tags like it does for branches. You could imagine that a future version of git adds this, and then we might change Tag.FullRefName to return the local ref for a remote tag, which would break the DeleteRemoteTag function if it were to use it.

Copy link

@miduddin miduddin Nov 28, 2025

Choose a reason for hiding this comment

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

I see, I do agree that the current implementation is good/clear enough as is, but I still wonder if there is a case to be made for putting the "refs/..." construction into one place since it has specific meaning. I don't have enough knowledge on the project structure though, so I'll leave the decisions to you, please don't treat this comment as a blocker.

Copy link
Collaborator Author

@stefanhaller stefanhaller Nov 28, 2025

Choose a reason for hiding this comment

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

I'd say that the current place (inside of DeleteRemoteTag/DeleteRemoteBranch) is exactly the right place. (And I don't feel we need an abstraction for this, like a FullRefFromTheRemotesPerspective or some such; that feels like overkill to me.)

Copy link

@miduddin miduddin Nov 28, 2025

Choose a reason for hiding this comment

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

Haha no, I was thinking along the lines of using models.Branch instead of models.RemoteBranch which would make it more aligned with what the actual git command accepts (a full local refname), but I have no idea how to make it a good fit since we are working with remote branch view in the first place.

Copy link
Collaborator Author

@stefanhaller stefanhaller Nov 28, 2025

Choose a reason for hiding this comment

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

Oh, that would feel weird to me, and would make it rather harder to understand than easier.

Anyway, thanks again for the review!

ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).Run()
}

func (self *RemoteCommands) DeleteRemoteTag(task gocui.Task, remoteName string, tagName string) error {
cmdArgs := NewGitCmd("push").
Arg(remoteName, "--delete", tagName).
Arg(remoteName, "--delete", "refs/tags/"+tagName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).Run()
Expand Down
48 changes: 48 additions & 0 deletions pkg/integration/tests/branch/delete_remote_branch_when_tag_with_same_name_exists.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package branch

import (
"github.com/jesseduffield/lazygit/pkg/config"
. "github.com/jesseduffield/lazygit/pkg/integration/components"
)

var DeleteRemoteBranchWhenTagWithSameNameExists = NewIntegrationTest(NewIntegrationTestArgs{
Description: "Delete a remote branch when a remote tag with the same name exists",
ExtraCmdArgs: []string{},
Skip: false,
SetupConfig: func(config *config.AppConfig) {},
SetupRepo: func(shell *Shell) {
shell.EmptyCommit("initial commit")
shell.CloneIntoRemote("origin")
shell.CreateLightweightTag("xyz", "HEAD")
shell.PushBranch("origin", "HEAD:refs/tags/xyz") // abusing PushBranch to push a tag
shell.PushBranch("origin", "HEAD:refs/heads/xyz")
},
Run: func(t *TestDriver, keys config.KeybindingConfig) {
t.Views().Remotes().
Focus().
Lines(
Contains("origin").IsSelected(),
).
PressEnter()

t.Views().RemoteBranches().
IsFocused().
Lines(
Contains("master").IsSelected(),
Contains("xyz"),
).
SelectNextItem().
Press(keys.Universal.Remove)

t.ExpectPopup().
Confirmation().
Title(Equals("Delete branch 'xyz'?")).
Content(Equals("Are you sure you want to delete the remote branch 'xyz' from 'origin'?")).
Confirm()

t.Views().RemoteBranches().
Lines(
Contains("master").IsSelected(),
)
},
})
1 change: 0 additions & 1 deletion pkg/integration/tests/tag/delete_local_and_remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ var DeleteLocalAndRemote = NewIntegrationTest(NewIntegrationTestArgs{
Confirm()
}).
IsEmpty().
Press(keys.Universal.New).
Tap(func() {
t.Shell().AssertRemoteTagNotFound("origin", "new-tag")
})
Expand Down
52 changes: 52 additions & 0 deletions pkg/integration/tests/tag/delete_remote_tag_when_branch_with_same_name_exists.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package tag

import (
"github.com/jesseduffield/lazygit/pkg/config"
. "github.com/jesseduffield/lazygit/pkg/integration/components"
)

var DeleteRemoteTagWhenBranchWithSameNameExists = NewIntegrationTest(NewIntegrationTestArgs{
Description: "Delete a remote tag when a remote branch with the same name exists",
ExtraCmdArgs: []string{},
Skip: false,
SetupConfig: func(config *config.AppConfig) {},
SetupRepo: func(shell *Shell) {
shell.EmptyCommit("initial commit")
shell.CloneIntoRemote("origin")
shell.CreateLightweightTag("xyz", "HEAD")
shell.PushBranch("origin", "HEAD:refs/tags/xyz") // abusing PushBranch to push a tag
shell.PushBranch("origin", "HEAD:refs/heads/xyz")
},
Run: func(t *TestDriver, keys config.KeybindingConfig) {
t.Views().Tags().
Focus().
Lines(
Contains("xyz").IsSelected(),
).
Press(keys.Universal.Remove)

t.ExpectPopup().
Menu().
Title(Equals("Delete tag 'xyz'?")).
Select(Contains("Delete remote tag")).
Confirm()

t.ExpectPopup().Prompt().
Title(Equals("Remote from which to remove tag 'xyz':")).
InitialText(Equals("origin")).
SuggestionLines(
Contains("origin"),
).
Confirm()

t.ExpectPopup().
Confirmation().
Title(Equals("Delete tag 'xyz'?")).
Content(Equals("Are you sure you want to delete the remote tag 'xyz' from 'origin'?")).
Confirm()

t.ExpectToast(Equals("Remote tag deleted"))

t.Shell().AssertRemoteTagNotFound("origin", "xyz")
},
})
2 changes: 2 additions & 0 deletions pkg/integration/tests/test_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ var tests = []*components.IntegrationTest{
branch.CreateTag,
branch.Delete,
branch.DeleteMultiple,
branch.DeleteRemoteBranchWhenTagWithSameNameExists,
branch.DeleteRemoteBranchWithCredentialPrompt,
branch.DeleteRemoteBranchWithDifferentName,
branch.DeleteWhileFiltering,
Expand Down Expand Up @@ -430,6 +431,7 @@ var tests = []*components.IntegrationTest{
tag.CrudAnnotated,
tag.CrudLightweight,
tag.DeleteLocalAndRemote,
tag.DeleteRemoteTagWhenBranchWithSameNameExists,
tag.ForceTagAnnotated,
tag.ForceTagLightweight,
tag.Reset,
Expand Down