Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repository: Fix issue with fragment identifier in branch name #1529

Merged
merged 2 commits into from
Jan 12, 2023

Conversation

pier-oliviert
Copy link
Contributor

@pier-oliviert pier-oliviert commented Jan 11, 2023

Resolves #1250


Behavior

If a fragment identifier (#) is part of the name of the branch, Repository#branch will return 404 if it doesn't escape the identifier.

Before the change?

  • Accessing a branch of name #1234-my-test would not return the branch

After the change?

  • Accessing a branch of name #1234-my-test will return the proper branch

Other information

  • The specs will fail I believe until we have a branch that reflects the test since it will require a VCR entry.

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • If customer of this library were already escaping the branch name, it will result in a double escape issue and the branch won't be found.

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

If a fragment identifier (#) is part of the name of the branch,
Repository#branch will return 404 if it doesn't escape the identifier.
@nickfloyd
Copy link
Contributor

Hey @pier-oliviert It looks like we might be missing a casset here. CI is looking for /spec/cassettes/Octokit_Client_Repositories/_branches/returns_a_single_branch_that_includes_character_that_needs_to_be_escaped.json but it's not one that exists in this PR or in your remote.

Would you mind looking into this - I'd be glad to give this a review and get it merged in as soon as CI is ✅ .

Thank you for the contributions here! ❤️

@pier-oliviert
Copy link
Contributor Author

@nickfloyd Yeah, I wasn't sure what was the best way to do this. Since I need a specific branch name for my PR to make sense (a branch name with the # in it) and that the spec looks at branch that exists on this repository, it's kind of a chicken/egg problem.

I can forge the cassette myself, and I'm happy to do it, but I wanted to be sure it was fine to forge it. The other option would be for you to create the branch for me on this repo, so I can run the spec on my end and add the cassette to the PR.

The name of the branch, as defined in this PR is: feature/#123-fragment-url

@nickfloyd
Copy link
Contributor

I created a branch for you named feature/#123-fragment-url - thanks for the clarity on what was going on.

@nickfloyd nickfloyd added the Type: Bug Something isn't working as documented label Jan 12, 2023
@nickfloyd nickfloyd merged commit 12da1b7 into octokit:main Jan 12, 2023
@pier-oliviert pier-oliviert deleted the fix-fragment-url-in-branch-name branch January 13, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Certain valid characters in refs aren't being escaped properly
2 participants