Skip to content

Escape base and head in repos_commits.go#1774

Merged
gmlewis merged 1 commit intogoogle:masterfrom
chughpiyush:issue_1642
Jan 11, 2021
Merged

Escape base and head in repos_commits.go#1774
gmlewis merged 1 commit intogoogle:masterfrom
chughpiyush:issue_1642

Conversation

@chughpiyush
Copy link
Copy Markdown
Contributor

@chughpiyush chughpiyush commented Jan 10, 2021

Fixes: #1642 as reported in #1654

I added code to escape base and head part of the URL in CompareCommits and CompareCommitsRaw methods.
Also did changes in relevant test methods, which are now testing for the following 3 cases:

        {base: "b", head: "h"},
   	{base: "123base", head: "head123"},
   	{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},

P.S. - Here's the original pull request that had to be reverted.

@google-cla google-cla Bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jan 10, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 10, 2021

Codecov Report

Merging #1774 (a9d15b4) into master (c2c4a6d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1774   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          98       98           
  Lines        6373     6377    +4     
=======================================
+ Hits         6216     6220    +4     
  Misses         85       85           
  Partials       72       72           
Impacted Files Coverage Δ
github/repos_commits.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2c4a6d...a9d15b4. Read the comment docs.

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Very nice, @chughpiyush - thank you.
LGTM.

Awaiting second LGTM before merging.

@gmlewis gmlewis changed the title Changes to escape base and head in repos_commits.go Escape base and head in repos_commits.go Jan 10, 2021
@gmlewis gmlewis requested a review from wesleimp January 10, 2021 16:40
Copy link
Copy Markdown
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏼

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jan 11, 2021

Thank you, @wesleimp !
Merging.

@gmlewis gmlewis merged commit 5f64fee into google:master Jan 11, 2021
@chughpiyush
Copy link
Copy Markdown
Contributor Author

Thank you both @gmlewis and @wesleimp !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CompareCommits() doesn't encode given base/head refs?

3 participants