Skip to content

Refactor doc flags (1 of n)#10118

Closed
richsalz wants to merge 1 commit intoopenssl:masterfrom
richsalz:refactor-doc-flags
Closed

Refactor doc flags (1 of n)#10118
richsalz wants to merge 1 commit intoopenssl:masterfrom
richsalz:refactor-doc-flags

Conversation

@richsalz
Copy link
Copy Markdown
Contributor

@richsalz richsalz commented Oct 8, 2019

This is a WIP to show what the "move flags to common page" would look like. #10115

levitte
levitte requested changes Oct 8, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Apart from these concerns, I like what I see in this PR so far.

Comment thread doc/man1/openssl-dgst.pod Outdated
Comment thread doc/man1/openssl-srp.pod Outdated
Copy link
Copy Markdown
Member

@levitte levitte Oct 8, 2019

Choose a reason for hiding this comment

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

Please don't add the ellipsis

Copy link
Copy Markdown
Contributor Author

@richsalz richsalz Oct 8, 2019

Choose a reason for hiding this comment

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

cut/paste error, fixed.

Copy link
Copy Markdown
Member

@levitte levitte Oct 9, 2019

Choose a reason for hiding this comment

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

Apparently not...

Comment thread util/find-doc-nits Outdated
Comment thread doc/man1/openssl.pod Outdated
levitte
levitte reviewed Oct 8, 2019
View reviewed changes
Comment thread util/find-doc-nits Outdated
beldmit
beldmit reviewed Oct 8, 2019
View reviewed changes
Comment thread doc/man1/openssl.pod Outdated
@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 8, 2019

Should I take this out of WIP and similar smallish PR's?

@levitte
Copy link
Copy Markdown
Member

levitte commented Oct 8, 2019

Unless you intend to do more substantial work, I see no reason to keep it as WIP
(when wondering, I always ask myself if I think it's complete enough and ready for final review/merge)

@richsalz richsalz changed the title WIP: Refactor doc flags Refactor doc flags (1 of n) Oct 8, 2019
@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 8, 2019

I took it out of WIP.

@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 9, 2019

Rebased and pushed, ready for review.

@richsalz richsalz mentioned this pull request Oct 9, 2019
Closed
levitte
levitte requested changes Oct 9, 2019
View reviewed changes
Comment thread doc/man1/openssl-dgst.pod Outdated
Comment thread doc/man1/openssl-dgst.pod Outdated
Comment thread doc/man1/openssl-rand.pod Outdated
Comment thread doc/man1/openssl-srp.pod Outdated
Copy link
Copy Markdown
Member

@levitte levitte Oct 9, 2019

Choose a reason for hiding this comment

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

Apparently not...

Comment thread doc/man1/openssl-srp.pod Outdated
@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 9, 2019

All fixed, additional commit pushed.

levitte
levitte requested changes Oct 9, 2019
View reviewed changes
Comment thread doc/man1/openssl.pod Outdated
Copy link
Copy Markdown
Member

@levitte levitte Oct 9, 2019

Choose a reason for hiding this comment

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

Hmmm, not sure B< is right in this case... C<? Frankly, I'm not entirely sure. @t8m, do you have an opinion?

Copy link
Copy Markdown
Member

@t8m t8m Oct 10, 2019

Choose a reason for hiding this comment

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

I would vote for C< as well.

Copy link
Copy Markdown
Member

@t8m t8m Oct 11, 2019

Choose a reason for hiding this comment

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

@richsalz Please change this and I'll approve.

Copy link
Copy Markdown
Contributor Author

@richsalz richsalz Oct 11, 2019

Choose a reason for hiding this comment

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

Amended commit pushed. Is there guidance anywhere on when to use C<> vs B<>? We're not consistent, so it will be will-o-the-wisp, which is okay I guess.

I would prefer all commits are squashed when merging.

Comment thread doc/man1/openssl-crl.pod Outdated
levitte
levitte requested changes Oct 9, 2019
View reviewed changes
Comment thread doc/man1/openssl.pod Outdated
Copy link
Copy Markdown
Member

@levitte levitte Oct 9, 2019

Choose a reason for hiding this comment

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

We're all so used to say "provider" these days, aren't we? I constantly catch myself making that exact same mistake

Copy link
Copy Markdown
Contributor Author

@richsalz richsalz Oct 9, 2019

Choose a reason for hiding this comment

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

Ha! Fixed.

Comment thread doc/man1/openssl-srp.pod Outdated
Copy link
Copy Markdown
Member

@levitte levitte Oct 9, 2019

Choose a reason for hiding this comment

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

Did you mean to do this in this PR? Shouldn't that be in another -passin / -passout PR? Or if you meant for it to be part of this PR, aren't there more places where this change should happen?

Either way, the remaining sentence should be this:

See L<openssl(1)/Pass Phrase Options> for more information.

Copy link
Copy Markdown
Contributor Author

@richsalz richsalz Oct 9, 2019

Choose a reason for hiding this comment

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

No, it was an editing error. Restored.

@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 9, 2019

updated commit pushed.

t8m
t8m requested changes Oct 10, 2019
View reviewed changes
Comment thread doc/man1/openssl.pod Outdated
Comment thread doc/man1/openssl.pod Outdated
@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 10, 2019

fixup commit pushed.

@richsalz richsalz mentioned this pull request Oct 11, 2019
Closed
t8m
t8m approved these changes Oct 11, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Good work!

@t8m t8m added approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Oct 11, 2019
@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 11, 2019

squashed and rebased. no changes made. better comments on the commit.

@richsalz
Refactor many common flags into openssl.pod
a25f187 
Options moved: -rand, -writerand, -CApath, -CAfile, -no-CApath, -no-CAfile
Added rand to dgst and srp manpages (they were missing them).
New sections in openssl.pod: Random State Options, Trusted Certificate
Options.
Cleanup and add comments to find-doc-nits
Remove ".in" file support; unless giving specific arguments, this
only runs after configuration
@t8m
Copy link
Copy Markdown
Member

t8m commented Oct 14, 2019

My approval still holds.

@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 14, 2019

My approval still holds.

You must be tired of saying that. :) Almost as much as I am waiting for the second approval :)

@richsalz
Copy link
Copy Markdown
Contributor Author

richsalz commented Oct 14, 2019

This isn't "waiting for 2nd review" and can be merged now.

@t8m
Copy link
Copy Markdown
Member

t8m commented Oct 15, 2019

Unfortunately it is waiting for 2nd review from @openssl/omc Ping @levitte @mattcaswell ?

mattcaswell
mattcaswell approved these changes Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

LGTM

@mattcaswell mattcaswell 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 Oct 15, 2019
levitte pushed a commit that referenced this pull request Oct 15, 2019
@richsalz @t8m
Refactor many common flags into openssl.pod
a397aca 
Options moved: -rand, -writerand, -CApath, -CAfile, -no-CApath, -no-CAfile
Added rand to dgst and srp manpages (they were missing them).
New sections in openssl.pod: Random State Options, Trusted Certificate
Options.
Cleanup and add comments to find-doc-nits
Remove ".in" file support; unless giving specific arguments, this
only runs after configuration

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #10118)
@t8m
Copy link
Copy Markdown
Member

t8m commented Oct 15, 2019

Merged [master a397aca]

@t8m t8m closed this Oct 15, 2019
@richsalz richsalz deleted the refactor-doc-flags branch October 15, 2019 13:20
This was referenced Oct 15, 2019
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte requested changes

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@beldmit beldmit beldmit approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@richsalz @levitte @t8m @mattcaswell @beldmit