Add git delete branch command #25862
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, |
extensions/git/src/git.ts
Outdated
| @@ -650,6 +650,11 @@ export class Repository { | |||
| await this.run(args); | |||
| } | |||
|
|
|||
| 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 |
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.