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

resumption_label misspelled when CHARSET_EBCDIC enabled #10396

Closed
wants to merge 1 commit into from

Conversation

@IdoBn
Copy link
Contributor

IdoBn commented Nov 9, 2019

The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled. Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.

Evaluating the hex values in the old code:

"".join([chr(i) for i in [0x72, 0x65, 0x64, 0x20, 0x62, 0x69, 0x6E, 0x64, 0x65, 0x72]])
'red binder'

Evaluating the hex values in the new code:

"".join([chr(i) for i in [0x72, 0x65, 0x73, 0x20, 0x62, 0x69, 0x6E, 0x64, 0x65, 0x72]])
'res binder'
@IdoBn IdoBn changed the title CLA: trivial resumption_label misspelled when CHARSET_EBCDIC enabled Nov 9, 2019
@kroeckx
kroeckx approved these changes Nov 9, 2019
@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Nov 9, 2019

I agree that this is trivial

Copy link
Contributor

mspncp left a comment

@IdoBn thank you for your contribution. I just have one formality: The CLA: trivial does not belong in the subject line, it goes in the body of the commit message, normally at the end. Since the subject line is shown in git's shortlog, it should show a meaningful description of the commit.

Also, the lines should ideally not exceed the 80 character limit.

May I suggest something like the following? (Feel free to make adjustments)

Fix misspelled resumption_label for CHARSET_EBCDIC

The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled.
Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.

CLA: trivial

If you know how to do it, you can just amend your commit and force-push it to your branch. If you don't, just let us know that you agree with the commit message, then we can change it while merging.

For the records: I agree it's a trivial change.

The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled.
Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.

CLA: trivial
@IdoBn IdoBn force-pushed the IdoBn:master branch from 04b07ca to 4ca4f68 Nov 9, 2019
@IdoBn

This comment has been minimized.

Copy link
Contributor Author

IdoBn commented Nov 9, 2019

Thanks for the response,
I amended the commit and force-pushed it.

@mspncp
mspncp approved these changes Nov 9, 2019
Copy link
Contributor

mspncp left a comment

Thanks for the quick response. LGTM.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Nov 9, 2019

@kroeckx the fix needs to be cherry-picked to 1.1.1., please reconfirm.

@mspncp mspncp added the triaged: bug label Nov 9, 2019
@kroeckx
kroeckx approved these changes Nov 9, 2019
@mspncp mspncp self-assigned this Nov 10, 2019
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Nov 10, 2019

I'll merge tomorrow morning...

openssl-machine pushed a commit that referenced this pull request Nov 11, 2019
The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled.
Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.

CLA: trivial

Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #10396)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2019
The resumption_label variable when CHARSET_EBCDIC was enabled, was misspelled.
Instead of evaluating to 'res binder' as expected, it evaluated to 'red binder'.

CLA: trivial

Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #10396)

(cherry picked from commit 6ed12ce)
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Nov 11, 2019

Merged to master and 1.1.1, thanks!

380aecb Fix misspelled resumption_label for CHARSET_EBCDIC [1.1.1]
6ed12ce Fix misspelled resumption_label for CHARSET_EBCDIC [master]

@mspncp mspncp closed this Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.