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.

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

Clean up local dangling reference to deleted remote branches. #8487

Closed
wants to merge 1 commit into from
Closed

Clean up local dangling reference to deleted remote branches. #8487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 21 additions & 7 deletions git/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,14 +336,28 @@ func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error {
}

func (c *Client) DeleteLocalBranch(ctx context.Context, branch string) error {
args := []string{"branch", "-D", branch}
cmd, err := c.Command(ctx, args...)
if err != nil {
return err
{
args := []string{"branch", "-D", branch}
cmd, err := c.Command(ctx, args...)
if err != nil {
return err
}
_, err = cmd.Output()
if err != nil {
return err
}
}
_, err = cmd.Output()
if err != nil {
return err
{
track := fmt.Sprintf("origin/%s", branch)
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I think we're going to need to inject something here. We can't know for sure that their remote is called origin. @samcoe can you advise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we will want to make sure that we have the correct remote name. Perhaps that is a n indication that we should create a new method DeleteLocalTrackingBranch that takes in the remote name and the remote branch name.

Or we reverse course and go with git push --delete since it only requires the branch name and automatically figures out the tracking branch to delete based on the push URL.

args := []string{"branch", "-d", "-r", track}
Copy link
Member

Choose a reason for hiding this comment

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

@asakatida I'm missing something here. Why do you think we need to run git branch -D <branch> followed by git branch -d -r <branch>. What about git branch -D -r <branch> doesn't work?

Copy link
Author

Choose a reason for hiding this comment

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

git branch -D -r <branch> will only delete the remote branch. It will not touch the local branch.

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh! I naively assumed that -r was additive! I guess it makes sense because a local branch main isn't necessarily related to remote tracking branches e.g. origin/main or upstream/main (although I think in some cases git manages a 1:1 relationship)

cmd, err := c.Command(ctx, args...)
if err != nil {
return err
}
_, err = cmd.Output()
if err != nil {
return err
}
}
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion git/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ func TestClientDeleteLocalBranch(t *testing.T) {
}{
{
name: "delete local branch",
wantCmdArgs: `path/to/git branch -D trunk`,
wantCmdArgs: `path/to/git branch -D trunk path/to/git branch -d -r origin/trunk`,
Copy link
Author

Choose a reason for hiding this comment

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

this still leaves this test erroring and I am not clear on how to correct the expectations

},
{
name: "git error",
Expand Down
42 changes: 42 additions & 0 deletions pkg/cmd/pr/close/close_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,9 @@ func TestPrClose_deleteBranch_sameRepo(t *testing.T) {
defer cmdTeardown(t)

cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git rev-parse --git-dir`, 128, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")

output, err := runCommand(http, true, `96 --delete-branch`)
assert.NoError(t, err)
Expand Down Expand Up @@ -196,6 +198,7 @@ func TestPrClose_deleteBranch_crossRepo(t *testing.T) {

cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")

output, err := runCommand(http, true, `96 --delete-branch`)
assert.NoError(t, err)
Expand Down Expand Up @@ -231,7 +234,9 @@ func TestPrClose_deleteBranch_sameBranch(t *testing.T) {

cs.Register(`git checkout main`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/trunk`, 0, "")
cs.Register(`git rev-parse --git-dir`, 128, "")
cs.Register(`git branch -D trunk`, 0, "")
cs.Register(`git branch -d -r origin/trunk`, 0, "")

output, err := runCommand(http, true, `96 --delete-branch`)
assert.NoError(t, err)
Expand Down Expand Up @@ -308,3 +313,40 @@ func TestPrClose_withComment(t *testing.T) {
assert.Equal(t, "", output.String())
assert.Equal(t, "✓ Closed pull request #96 (The title of the PR)\n", output.Stderr())
}

func TestPrClose_deleteBranch_inGitRepo(t *testing.T) {
http := &httpmock.Registry{}
defer http.Verify(t)

baseRepo, pr := stubPR("OWNER/REPO:main", "OWNER/REPO:trunk")
pr.Title = "The title of the PR"
shared.RunCommandFinder("96", pr, baseRepo)

http.Register(
httpmock.GraphQL(`mutation PullRequestClose\b`),
httpmock.GraphQLMutation(`{"id": "THE-ID"}`,
func(inputs map[string]interface{}) {
assert.Equal(t, inputs["pullRequestId"], "THE-ID")
}),
)
http.Register(
httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/trunk"),
httpmock.StringResponse(`{}`))

cs, cmdTeardown := run.Stub()
defer cmdTeardown(t)

cs.Register(`git rev-parse --verify refs/heads/trunk`, 0, "")
cs.Register(`git rev-parse --git-dir`, 0, ".git")
cs.Register(`git checkout main`, 0, "")
cs.Register(`git branch -D trunk`, 0, "")
cs.Register(`git branch -d -r origin/trunk`, 0, "")

output, err := runCommand(http, true, `96 --delete-branch`)
assert.NoError(t, err)
assert.Equal(t, "", output.String())
assert.Equal(t, heredoc.Doc(`
✓ Closed pull request #96 (The title of the PR)
✓ Deleted branch trunk and switched to branch main
`), output.Stderr())
}
8 changes: 8 additions & 0 deletions pkg/cmd/pr/merge/merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@ func TestPrMerge_deleteBranch(t *testing.T) {
cs.Register(`git checkout main`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`)
Expand Down Expand Up @@ -695,6 +696,7 @@ func TestPrMerge_deleteBranch_nonDefault(t *testing.T) {
cs.Register(`git checkout fruit`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`)
Expand Down Expand Up @@ -744,6 +746,7 @@ func TestPrMerge_deleteBranch_onlyLocally(t *testing.T) {
cs.Register(`git checkout main`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`)
Expand Down Expand Up @@ -794,6 +797,7 @@ func TestPrMerge_deleteBranch_checkoutNewBranch(t *testing.T) {
cs.Register(`git checkout -b fruit --track origin/fruit`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`)
Expand Down Expand Up @@ -842,6 +846,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) {

cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")

output, err := runCommand(http, nil, "main", true, `pr merge --merge --delete-branch blueberries`)
if err != nil {
Expand Down Expand Up @@ -1081,6 +1086,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) {
cs.Register(`git checkout main`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

pm := &prompter.PrompterMock{
Expand Down Expand Up @@ -1153,6 +1159,7 @@ func TestPrMerge_alreadyMerged_withMergeStrategy_TTY(t *testing.T) {

cs.Register(`git rev-parse --verify refs/heads/`, 0, "")
cs.Register(`git branch -D `, 0, "")
cs.Register(`git branch -d -r origin/`, 0, "")

pm := &prompter.PrompterMock{
ConfirmFunc: func(p string, d bool) (bool, error) {
Expand Down Expand Up @@ -1324,6 +1331,7 @@ func TestPRMergeTTY_withDeleteBranch(t *testing.T) {
cs.Register(`git checkout main`, 0, "")
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
cs.Register(`git branch -D blueberries`, 0, "")
cs.Register(`git branch -d -r origin/blueberries`, 0, "")
cs.Register(`git pull --ff-only`, 0, "")

pm := &prompter.PrompterMock{
Expand Down