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

sergiolms · 2024-09-20T09:12:58Z

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 · 2024-09-26T21:30:59Z

@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:

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)
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)
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 · 2024-09-30T15:48:37Z

src/env/node/git/localGitProvider.ts

@@ -1240,6 +1250,57 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		await this.git.branch(repoPath, '-m', oldName, newName);
 	}

+	@log()
+	async deleteBranch(


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.

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

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.

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)

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!

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 · 2024-10-01T14:45:21Z

src/commands/git/branch.ts

+			} catch (ex) {
+				Logger.error(ex);
+				// TODO likely need some better error handling here
+				return showGenericErrorMessage(


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

eamodio · 2024-10-01T14:47:08Z

src/commands/git/branch.ts

+				Logger.error(ex);
+				// TODO likely need some better error handling here
+				return showGenericErrorMessage(
+					new BranchError(ex.reason, ex, state.references.map(r => r.name).join(', ')).message,


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

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 · 2024-10-01T14:47:34Z

src/commands/git/branch.ts

@@ -627,7 +637,7 @@ export class BranchGitCommand extends QuickCommand {
 			} catch (ex) {
 				Logger.error(ex);
 				// TODO likely need some better error handling here
-				return showGenericErrorMessage('Unable to rename branch');
+				return showGenericErrorMessage(new BranchError(ex.reason, ex, state.name).message);


eamodio · 2024-10-01T14:57:44Z

src/env/node/git/localGitProvider.ts

@@ -1240,6 +1250,57 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		await this.git.branch(repoPath, '-m', oldName, newName);
 	}

+	@log()
+	async deleteBranch(


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 · 2024-10-03T15:20:40Z

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

src/env/node/git/git.ts

src/env/node/git/localGitProvider.ts

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 · 2024-11-11T14:24:47Z

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:

sergiolms self-assigned this Sep 20, 2024

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch 2 times, most recently from d2ada6c to 275d1f4 Compare September 26, 2024 14:56

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch 7 times, most recently from d3f465e to fb0d9ac Compare September 27, 2024 13:54

sergiolms requested a review from eamodio September 30, 2024 07:34

sergiolms marked this pull request as ready for review September 30, 2024 07:34

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from fb0d9ac to 31db995 Compare September 30, 2024 07:34

sergiolms requested a review from axosoft-ramint September 30, 2024 07:34

axosoft-ramint reviewed Sep 30, 2024

View reviewed changes

eamodio requested changes Oct 1, 2024

View reviewed changes

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from 66762d4 to 58551c8 Compare October 3, 2024 13:43

sergiolms requested a review from eamodio October 3, 2024 14:20

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from 4794ca0 to d093bc3 Compare October 4, 2024 11:02

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from d093bc3 to e9baa26 Compare October 15, 2024 14:20

axosoft-ramint self-requested a review October 15, 2024 14:58

axosoft-ramint requested changes Oct 22, 2024

View reviewed changes

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch 2 times, most recently from 128b31b to 5ed1cf7 Compare October 23, 2024 10:43

sergiolms requested a review from axosoft-ramint October 23, 2024 10:46

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from 5ed1cf7 to bed464f Compare October 23, 2024 12:51

axosoft-ramint approved these changes Oct 24, 2024

View reviewed changes

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from bed464f to 4f145da Compare November 7, 2024 12:37

axosoft-ramint reviewed Nov 7, 2024

View reviewed changes

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from 4f145da to ead29af Compare November 8, 2024 16:29

sergiolms added 11 commits November 11, 2024 12:27

add deleteBranch

7924d2a

add BranchError and BranchErrorReason

6337cd0

refactor code

1b92509

rename deleteBranch to deleteBranches

442e08c

safe delete branch

fbea501

add message builder to BranchError

6331812

convert deleteBranches into deleteBranch and call multiple times

3c8b554

add message & sort supress keys

96e1085

handle BranchError in localGitProvider

3f83b35

prompt to retry with force

2f17ba7

rebase, lint & code fixes

dac43a8

handle delete remote ref that does not exist err

965edeb

sergiolms force-pushed the feature/#3528-part2_change_branch_into_normal_cmd branch from ead29af to 965edeb Compare November 11, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sergiolms commented Sep 20, 2024 •

edited

Loading

eamodio commented Sep 26, 2024

axosoft-ramint left a comment

axosoft-ramint Sep 30, 2024

sergiolms Oct 1, 2024 •

edited

Loading

eamodio Oct 1, 2024

eamodio Oct 1, 2024

eamodio Oct 1, 2024

sergiolms Oct 1, 2024

eamodio Oct 1, 2024

eamodio Oct 1, 2024

sergiolms commented Oct 3, 2024

axosoft-ramint left a comment

axosoft-ramint left a comment

axosoft-ramint left a comment

sergiolms commented Nov 11, 2024

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

Are you sure you want to change the base?

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

Conversation

sergiolms commented Sep 20, 2024 • edited Loading

Description

Checklist

eamodio commented Sep 26, 2024

axosoft-ramint left a comment

Choose a reason for hiding this comment

axosoft-ramint Sep 30, 2024

Choose a reason for hiding this comment

sergiolms Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

eamodio Oct 1, 2024

Choose a reason for hiding this comment

eamodio Oct 1, 2024

Choose a reason for hiding this comment

eamodio Oct 1, 2024

Choose a reason for hiding this comment

sergiolms Oct 1, 2024

Choose a reason for hiding this comment

eamodio Oct 1, 2024

Choose a reason for hiding this comment

eamodio Oct 1, 2024

Choose a reason for hiding this comment

sergiolms commented Oct 3, 2024

axosoft-ramint left a comment

Choose a reason for hiding this comment

axosoft-ramint left a comment

Choose a reason for hiding this comment

axosoft-ramint left a comment

Choose a reason for hiding this comment

sergiolms commented Nov 11, 2024

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

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

sergiolms commented Sep 20, 2024 •

edited

Loading

sergiolms Oct 1, 2024 •

edited

Loading