Skip to content

Escape special characters in ref name for update ref - #2454

Merged
gmlewis merged 2 commits into
google:masterfrom
shravan1k:escape-ref-url-for-ref-patch
Sep 12, 2022
Merged

Escape special characters in ref name for update ref#2454
gmlewis merged 2 commits into
google:masterfrom
shravan1k:escape-ref-url-for-ref-patch

Conversation

@shravan1k

Copy link
Copy Markdown
Contributor

Closes#2453

Most of the functions that deal with refs make sure to escape the special characters within the ref's name. The UpdateRef function doesn't seem to call the function that does this substitution of special characters with their encodings. This PR solves this problem and should now successfully patch remote refs if the GetRef function says the corresponding ref exists.

@google-cla

google-claBot commented Sep 8, 2022

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov

codecovBot commented Sep 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2454 (185b4c5) into master (52cc49e) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2454 +/- ##
=======================================
Coverage 98.03% 98.03% =======================================
Files 122 122 Lines 10689 10689 =======================================
Hits 10479 10479 Misses 144 144 Partials 66 66 
Impacted FilesCoverage Δ
github/git_refs.go100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shravan1kshravan1k reopened this Sep 9, 2022

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @shravan1k !
Please add a unit test that demonstrates this change in functionality.

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 9, 2022

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whups - checked the wrong box.

Thank you, @shravan1k !
Please add a unit test that demonstrates this change in functionality.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Beautiful, @shravan1k ! Thank you!
LGTM.

Awaiting second LGTM from any other contributor to this repo before merging.

@marcelosousamarcelosousa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 12, 2022
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @marcelosousa !
Merging.

Sign up for freeto 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.

Update ref doesn't escape special characters in ref name

3 participants

@shravan1k@gmlewis@marcelosousa