Clean up local dangling reference to deleted remote branches. #8487
Conversation
|
Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message. |
cliAutomation
added
the
external
|
@asakatida Thanks for the contribution. Can you please elaborate in the PR description what this PR is trying to achieve and link it to an open issue. Without this context we will not be able to review the PR. |
There was a problem hiding this comment.
@asakatida : Thank you for this PR to improve the GitHub CLI experience! Really appreciate changes for the tests, too ❤️
I have only 1 concern with regards to separation of concerns in the changes but fortunately I think it is something we can work through to get these changes through.
This comment was marked as spam.
This comment was marked as spam.
| @@ -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`, | |||
There was a problem hiding this comment.
this still leaves this test erroring and I am not clear on how to correct the expectations
| return err | ||
| { | ||
| track := fmt.Sprintf("origin/%s", branch) | ||
| args := []string{"branch", "-d", "-r", track} |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
git branch -D -r <branch> will only delete the remote branch. It will not touch the local branch.
There was a problem hiding this comment.
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)
| if err != nil { | ||
| return err | ||
| { | ||
| track := fmt.Sprintf("origin/%s", branch) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@asakatida this PR seems to have gone stale some time ago; perhaps the next steps were not clear since Sam left GitHub around this time. Is this something you are still interested in getting merged? |
|
@williammartin the next steps were clear, but at that point it was easier to migrate my workflows away from using this client. So, not something that needs merging from my end anymore |
|
Got it, thanks for trying and good luck. I'll leave the issue open in case anyone else wants to address it. |
Fixes #8515