Skip to content

Fix bugs in "info" commands flags #9986

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.

Sign up for GitHub

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

Jump to bottom
Closed
richsalz wants to merge 1 commit into from
Closed

Fix bugs in "info" commands flags #9986

richsalz wants to merge 1 commit into from

Conversation

@richsalz
Copy link
Contributor

@richsalz richsalz commented Sep 23, 2019

Remove -c/-e/-m aliases, OpenSSL commands don't do that.
Fix typo's in the documentation.
Fix -module flag to print the right thing.

@richsalz
Fix bugs in "info" commands flags
ed3472f 
Remove -c/-e/-m aliases, OpenSSL commands don't do that.
Fix typo's in the documentation.
Fix -module flag to print the right thing.
@richsalz
Copy link
Contributor Author

richsalz commented Sep 23, 2019

This is a new command for 3.0, so no compatibility issues.

@richsalz
Copy link
Contributor Author

richsalz commented Sep 23, 2019

(I kinda wish this and version were one command.)

@levitte
Copy link
Member

levitte commented Sep 23, 2019

OpenSSL commands don't do that

Er, so? openssl version has short commands, I was thinking that for the things that exist in both this and that, it is nice with some compatible options.

(I kinda wish this and version were one command.)

Yeah well. Quite a lot of the info has nothing to do with versions per se, and openssl version is much too verbose for practice use with scripts. openssl info is primarily designed for them.

@richsalz
Copy link
Contributor Author

richsalz commented Sep 23, 2019

Consistency is important. You should either have just short commands, like version, or have just long commands. I prefer just short commands. Shall I change this?

@levitte
Copy link
Member

levitte commented Sep 23, 2019

openssl enc has -a and -base64 meaning the same thing, FYI. But ok, that was the only already existing example I could find.

I disagree with you regarding this design choice (not bug), but if I must choose only one, then I'll go for the long options. There's a reason I didn't assign short options to everything, I ran out of reasonable letter that weren't already taken. Besides, almost all our sub-commands (I think only tsget and version use short options throughout) use long options, so if you want to be consistent, that's something to draw on.

@richsalz
Copy link
Contributor Author

richsalz commented Sep 23, 2019

Well, we can't change the existing programs and I think adding aliases is going to cause problems. And tsget was never a model of good OpenSSL integration :) I am sorry if I insulted you by saying "bug" I hope it helps that lines 2 and 3 of the commit pointed out real bugs.

levitte
levitte approved these changes Sep 23, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

Feeling satisfied the air is cleared 😉

@levitte levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Sep 23, 2019
@richsalz richsalz mentioned this pull request Sep 23, 2019
Closed
@richsalz
Copy link
Contributor Author

richsalz commented Sep 24, 2019

This is ready, can it get merged soon? I will have to retrofit these changes into #9953.

levitte pushed a commit that referenced this pull request Sep 24, 2019
@richsalz @levitte
Fix bugs in "info" commands flags
0773687 
Remove -c/-e/-m aliases, OpenSSL commands don't do that.
Fix typo's in the documentation.
Fix -module flag to print the right thing.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9986)
@levitte
Copy link
Member

levitte commented Sep 24, 2019

Done

0773687 Fix bugs in "info" commands flags

@levitte levitte closed this Sep 24, 2019
@levitte levitte removed the approval: review pending This pull request needs review by a committer label Sep 24, 2019
@levitte
Copy link
Member

levitte commented Sep 24, 2019 •
edited
Loading

Oh damn... as I hit submit, I noticed Travis had a red cross. Eh well

@richsalz
Copy link
Contributor Author

richsalz commented Sep 24, 2019

I'll fix any issues ASAP within an hour or two if you don't beat me too it. I am hoping "Travis failures are unrelated" to coin a phrase :)

@levitte
Copy link
Member

levitte commented Sep 24, 2019

False alarm, it was just a timeout: https://travis-ci.org/openssl/openssl/jobs/588543043

@richsalz richsalz deleted the remove-info-aliases branch October 2, 2019 18:35
@richsalz richsalz mentioned this pull request Oct 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@richsalz @levitte @paulidale