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

Update OPENSSL_buf2hexstr() to use DEFAULT_SEPARATOR. #22041

Closed
wants to merge 1 commit into from

Conversation

abbypan
Copy link
Contributor

@abbypan abbypan commented Sep 8, 2023

let o_str.c OPENSSL_buf2hexstr follow DEFAULT_SEPARATOR, not static :

CLA: trivial

crypto/o_str.c Outdated
* hex representation @@@ (Contents of buffer are always kept in ASCII, also
* on EBCDIC machines)
* hex representation with DEFAULT_SEPARATOR @@@ (Contents of buffer are always kept in ASCII, also
* on EBCDIC machines)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add end of line white space.

Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this comment change. The function is documented sufficiently in the corresponding .pod file.
If you want to change it maybe use ````using the separator DEFAULT_SEPARATOR``` and then
fix up the lines to not be larger than 80 chars.

Copy link
Contributor Author

@abbypan abbypan Sep 12, 2023

Choose a reason for hiding this comment

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

The end of line white space is removed.
The comment fall back, change 'len' to 'buflen' (<=80 chars).
Removed the merge commit.

@t8m
Copy link
Member

t8m commented Sep 11, 2023

Please remove the merge commits. We do not accept PRs with merge commits.

@t8m t8m added branch: master Merge to master branch triaged: refactor The issue/pr requests/implements refactoring tests: exempted The PR is exempt from requirements for testing cla: trivial One of the commits is marked as 'CLA: trivial' labels Sep 11, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 11, 2023
@slontis slontis changed the title CLA: trivial let o_str.c OPENSSL_buf2hexstr follow DEFAULT_SEPARATOR Update OPENSSL_buf2hexstr() to use DEFAULT_SEPARATOR instead of a magic number. Sep 12, 2023
@slontis slontis changed the title Update OPENSSL_buf2hexstr() to use DEFAULT_SEPARATOR instead of a magic number. Update OPENSSL_buf2hexstr() to use DEFAULT_SEPARATOR. Sep 12, 2023
@slontis
Copy link
Member

slontis commented Sep 12, 2023

You can collapse your commits using
git rebase -i

Use reword on the first commit and then squash the other 2

You could reword it to something like this..

Update OPENSSL_buf2hexstr() to use DEFAULT_SEPARATOR.
CLA: trivial

Followed by a force push
git push -f

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Sep 12, 2023
@t8m
Copy link
Member

t8m commented Sep 12, 2023

Please edit the commit message (again with git rebase -i or with git commit --amend to put one empty line between the first line - commit message subject and the rest of the commit message lines - commit message body).

@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Sep 12, 2023
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK with CLA: trivial.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Sep 12, 2023
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Okay with trivial

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 13, 2023
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@paulidale paulidale removed the hold: cla required The contributor needs to submit a license agreement label Sep 17, 2023
@paulidale
Copy link
Contributor

Merged, thanks for the contribution.

@paulidale paulidale closed this Sep 17, 2023
openssl-machine pushed a commit that referenced this pull request Sep 17, 2023
CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22041)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22041)

Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants