Add git delete branch command#25862
Add git delete branch command#25862joaomoreno merged 4 commits intomicrosoft:masterfrom letmaik:git-delete-branch
Conversation
|
@letmaik, thanks for your PR! By analyzing the history of the files in this pull request, we identified @joaomoreno and @jrieken to be potential reviewers. |
|
@letmaik, |
joaomoreno
left a comment
joaomoreno
left a comment
There was a problem hiding this comment.
Looks great, just needs a bit of error handling.
extensions/git/src/git.ts
Outdated
| } | ||
|
|
||
| async deleteBranch(name: string): Promise<void> { | ||
| const args = ['branch', '-d', name]; |
There was a problem hiding this comment.
Maybe we should be explicit with the branch name and use its full form refs/heads/${name} instead of simply name.
extensions/git/src/commands.ts
Outdated
| return; | ||
| } | ||
|
|
||
| await model.deleteBranch(ref); |
There was a problem hiding this comment.
A common error from this operation happens when the branch isn't yet merged:
It would be great if the underlying git library would catch this situation, wrap it neatly in a GitError with a new GitErrorCode and we try catch it here, prompting the user Hey, the branch isn't yet merge. Do you still want to delete it?.
|
I worked a bit more on it, let me know what you think. It comes up with a confirmation prompt now if the branch is not fully merged, and if the user confirms to proceed, then it does it again but with force. |
|
Do you want me to do anything else here? I checked the failing macOS build but I think this is just a random error in the tests and not related:
|
|
Awesome, thanks dude 🍻 Sorry for the delay |
4 participants
This implements #2558
This is my first contribution, so the approach I took in implementing it is probably quite naive. I'm looking forward to comments and suggestions on how to improve the code. Some things I wasn't sure about:
const currentHead = this.model.HEAD && this.model.HEAD.name;and then filtering byref.name !== currentHead. I'm not sure ifthis.model.HEADis the right way to get the current branch.const placeHolder = 'Select a branch to delete';but saw that other strings for prompts uselocalize(...). However, in thegit.checkoutcommand there is also such aplaceHolderconstant which does not uselocalizeso I wasn't sure here.git/src/model.tsthere is that weirdOperationenum with bit shifting for the values. I haven't understood what the exact purpose of that is, so I just followed the pattern and added a new entry at the end.