Skip to content

Improve error message in branch existence check#6148

Merged
adamzip merged 5 commits intodotnet:mainfrom
adamzip:improve-error-msg-in-set-repository-policies
Apr 1, 2026
Merged

Improve error message in branch existence check#6148
adamzip merged 5 commits intodotnet:mainfrom
adamzip:improve-error-msg-in-set-repository-policies

Conversation

@adamzip
Copy link
Copy Markdown
Contributor

@adamzip adamzip commented Mar 30, 2026

Copilot AI review requested due to automatic review settings March 30, 2026 15:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates DARC’s set-repository-policies operation to perform an explicit branch existence check via IRemote.BranchExistsAsync, aiming to produce clearer failures when branch verification can’t be performed.

Changes:

  • Replaced UxHelpers.VerifyAndConfirmBranchExistsAsync(...) with a direct BranchExistsAsync call.
  • Added exception handling and logging around the branch existence verification path.

premun
premun previously approved these changes Mar 31, 2026
Comment thread src/Microsoft.DotNet.Darc/Darc/Operations/SetRepositoryMergePoliciesOperation.cs Outdated
Copy link
Copy Markdown
Member

@premun premun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamzip should we still narrow down the exception we're catching? Seems like we might be masking other problems - like if GitHub 500s the user will think it's a PAT problem and will go and regenerate it only to find out it didn't help anything

@adamzip
Copy link
Copy Markdown
Contributor Author

adamzip commented Mar 31, 2026

@adamzip should we still narrow down the exception we're catching? Seems like we might be masking other problems - like if GitHub 500s the user will think it's a PAT problem and will go and regenerate it only to find out it didn't help anything

I wanted to avoid parsing the error message of the HttpRequestException if possible, so ideally the error message would just inform the user what it was trying to do, and then print whatever error message came out.
How about this then:

Error: Darc attempted to make a remote git requets on your behalf at {repo}@{branch}, using the configured credentials. The request failed with the following error message: [...]

@premun
Copy link
Copy Markdown
Member

premun commented Mar 31, 2026

That's maybe a better error message but do we need to parse the error message?

  • Isn't it better to catch HttpRequestException then?
  • Doesn't HttpRequestException have StatusCode that you can filter based on? Maybe that's enough to tell and let the rest go through?

@adamzip
Copy link
Copy Markdown
Contributor Author

adamzip commented Mar 31, 2026

That's maybe a better error message but do we need to parse the error message?

  • Isn't it better to catch HttpRequestException then?
  • Doesn't HttpRequestException have StatusCode that you can filter based on? Maybe that's enough to tell and let the rest go through?

I implemented your suggestion, feel free to change the message wording

@adamzip adamzip merged commit 5e3c016 into dotnet:main Apr 1, 2026
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants