3528 Change branch terminal-run commands into normal commands w/ proper error handling

[sergiolms]

Description

Part 2 including changes to branch delete.
Resolves #3528

Example of error:

Checklist

• I have followed the guidelines in the Contributing document
• My changes follow the coding style of this project
• My changes build without any errors or warnings
• My changes have been formatted and linted
• My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
• My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
• My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

[eamodio]

@sergiolms Please apply similar changes from #3596 to this PR

[axosoft-ramint]

Since I don't have too much context going into this, could you help me test it by:

1. Describing how to initiate the commands being changed (all the different ways we can get into the delete branch flow that you are aware of)
2. Describing what happens before this PR and what happens after the PR changes (essentially, what behavior should I expect to be different when using this PR)
3. Describing what areas may have regressions as a result of the PR changes that I should spot-check, if any

I did a quick code-scan and it looks good, just had one nitpick below:

[axosoft-ramint]

Nitpick: since this takes an array of branches as a param, shouldn't we call this function deleteBranches? Same with the provider service function for consistency, though that one can take either a single or multiple branches.

[sergiolms]

Hey @axosoft-ramint ! Sure, let me go over your questions:

1. Describing how to initiate the commands being changed (all the different ways we can get into the delete branch flow that you are aware of)

This behavior hasn't changed. I usually trigger the delete branch action from the context menu in the graph. But since the context menu opens the command palette, you can directly execute the GitLens: Git Delete Branch action from the command palette as well.
Note that if you do the latter you will be asked to select the branch since it has no context.

2. Describing what happens before this PR and what happens after the PR changes (essentially, what behavior should I expect to be different when using this PR)
3. Describing what areas may have regressions as a result of the PR changes that I should spot-check, if any

Before this PR, executing the git action would run a terminal with the commands needed to delete the branch.
See the screenshot, where I ran a create branch action to create a branch called test_branch and then I ran a delete branch action to delete the same branch:

However, now we run the commands through our git binary and wrap any possible error. See the following example (it was forced to have a bad name, this is not possible because we filter invalid characters out):

So the expected behavior is that when a git branch command is run (create, rename or delete), it doesn't open a terminal anymore and performs the action. If anything happens, it should display a toast with the error information.

Nitpick: since this takes an array of branches as a param, shouldn't we call this function deleteBranches? Same with the provider service function for consistency, though that one can take either a single or multiple branches.

Yeah makes sense. I followed the naming convention of createBranch and renameBranch thinking of the git branch command, so I would group them all under the branch name if that makes sense. But it makes more sense for a method that may take multiple branches to be called deleteBranches instead. Good catch!

[eamodio]

The naming is one issue here, and maybe it should be broken into 2 methods, a deleteBranch and deleteBranches, but imo the bigger issue is the error handling. The first error breaks the whole operation. Unless we want to create a good AggregateError model so that a caller can handle all the issue, I would suggest that we leave the deleting multiple branches up to the calling code (we can wrap it in its own function to be shared -- in git/actions/branch probably), and just have the method here deal with a single branch (and optionally its remote) so that errors can be handled, prompted and maybe resumes independently.

[eamodio]

We should be handling resumable/retriable errors. For example, if not merged, we should offer to retry with force.

[eamodio]

Why are we creating a BranchError here? Isn't the one being thrown a BranchError?

[sergiolms]

Indeed, I don't really like this solution either. When we return the BranchError at git level we don't know the branch names, we just have an array of args. We could either guess the branch names, or change the number of args of git.branch to be more specific so we can have easier access. In this case we're reusing the BranchError but adding the branch name for context.
Another option is to turn BranchError into a Builder pattern that we can always call like showGenericErrorMessage(ex.WithBranch(branchName)) or something along those lines to provide context on top layers.

[eamodio]

Same here

[eamodio]

The naming is one issue here, and maybe it should be broken into 2 methods, a deleteBranch and deleteBranches, but imo the bigger issue is the error handling. The first error breaks the whole operation. Unless we want to create a good AggregateError model so that a caller can handle all the issue, I would suggest that we leave the deleting multiple branches up to the calling code (we can wrap it in its own function to be shared -- in git/actions/branch probably), and just have the method here deal with a single branch (and optionally its remote) so that errors can be handled, prompted and maybe resumes independently.

[sergiolms]

Now when an error such as unmerged branch happens, we display a prompt to retry it:

We can turn this prompt into something reusable like a confirmation prompt we can parametrize as we find more cases

[axosoft-ramint]

Several build errors here, as well as conflicts with main. Will test once it's rebased and the errors are resolved

[axosoft-ramint]

Not sure what commands are affected, and the only one I noticed was branch delete, so I tried it with a few cases and looks like it works and doesn't open my terminal.

LGTM - let me know if there are other cases I should test

[axosoft-ramint]

When attempting to delete a remote branch that no longer exists, I do not get the right error message in the notification area. I get a push error that doesn't describe the issue well:

It is true that the operation happening under the hood is git push -d origin cool-branch but I expected an error relating to "unable to delete branch because the remote branch was already deleted" or however you have the messaging set up.

The error I get in the logs is more descriptive/accurate:

[sergiolms]

When attempting to delete a remote branch that no longer exists, I do not get the right error message in the notification area. I get a push error that doesn't describe the issue well:
[...]
It is true that the operation happening under the hood is git push -d origin cool-branch but I expected an error relating to "unable to delete branch because the remote branch was already deleted" or however you have the messaging set up.

True. Since this action is running under git push it wasn't covered by the git branch errors. I added an additional case to git push to handle that case: