Remove/replace some code #4478
Remove/replace some code #4478
Conversation
richsalz
added
branch: master
apps/CA.pl.in
Outdated
| # create a certificate request | ||
| $RET = run("$REQ -new -nodes -keyout $NEWKEY -out $NEWREQ $DAYS $EXTRA{req}"); | ||
| print "Request is in $NEWREQ, private key is in $NEWKEY\n" if $RET == 0; | ||
| die "Flag removed; please use -extra-req-nodes instead.\n"; |
There was a problem hiding this comment.
Are you sure that it's not -extra-req -nodes? And the message is confusing, because it gives you impression that it replaces -newreq-nodes, but it doesn't. It looks like alternative is -newreq -extra-req -nodes.
But what exactly is the license-able claim? Existence of the flag or its implementation? Because if latter, then one can as well do something like
} elsif ($WHAT =~ '/\-newreq(\-nodes)?/' ) {
# create a certificate request
$RET = run("$REQ -new $1 -keyout $NEWKEY -out $NEWREQ $DAYS $EXTRA{req}");
print "Request is in $NEWREQ, private key is in $NEWKEY\n" if $RET == 0;
|
You cannot copyright a concept; the code has to be replaced. Your idea is a good one, additional commit pushed. |
apps/CA.pl.in
Outdated
| @@ -131,9 +131,9 @@ if ($WHAT eq '-newcert' ) { | |||
| # create a certificate request | |||
| $RET = run("$REQ -new -keyout $NEWKEY -out $NEWREQ $DAYS $EXTRA{req}"); | |||
| print "Request is in $NEWREQ, private key is in $NEWKEY\n" if $RET == 0; | |||
| } elsif ($WHAT eq '-newreq-nodes' ) { | |||
| } elsif ($WHAT eq /-newreq(\-nodes)?/ ) { | |||
There was a problem hiding this comment.
No-no-no, suggestion was to remove this elsif and modify the preceding one. Well, now you can simply remove the preceding elsif. But either way modifying just two lines was not actual suggestion. Also note that it renders CHANGES entry misleading. I mean it reads "removed -newreq-nodes" while what happens here is "reimplemented -newreq-nodes".
However! It's not actually given that it makes sense to keep it. I mean even if reimplementation does it, we still might find it appropriate to omit it. Or at least to declare deprecated. In which case it would be appropriate to issue warning that reads "stop using this, use <this> instead."
There was a problem hiding this comment.
And there is typo, it's not "$WHAT eq ..." but "$WHAT =~ ..."
There was a problem hiding this comment.
And it was /\-new..., not /-new.... Well, /-new.../ works, but it looks ambiguous and hence less readable.
|
I think we have to keep it in the .1 release. But I updated the commit and fixes all the silly mistakes you pointed out just before. Thanks. |
dot-asm
removed
the
approval: review pending
Rewrite the -req-nodes flag from CA.pl (idea from Andy) Rewrite ERR_string_error_n
|
On closer look I have additional comment... |
Rewrite the -req-nodes flag from CA.pl (idea from Andy) Rewrite ERR_string_error_n Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #4478)
|
okay, separate PR. what's the issue you've got? |
| @@ -127,13 +127,9 @@ if ($WHAT eq '-newcert' ) { | |||
| # create a pre-certificate | |||
| $RET = run("$REQ -x509 -precert -keyout $NEWKEY -out $NEWCERT $DAYS"); | |||
| print "Pre-cert is in $NEWCERT, private key is in $NEWKEY\n" if $RET == 0; | |||
| } elsif ($WHAT eq '-newreq' ) { | |||
| } elsif ($WHAT =~ /\-newreq(\-nodes)?/ ) { | |||
There was a problem hiding this comment.
Formally speaking one wants equivalent code, and the replacement is not. One has to add ^ in beginning and $ at the end to make it equivalent to the original. Could you throw these two in, please?
|
Oh! I didn't notice that it was committed already... It probably doesn't really have to be additional request, I'd say just add additional commit and I'll approve it separately... Well, you decide... |
|
See #4483 |
|
Too late now...but we really should split these things into two halfs - removal and replacement, i.e. two different commits |
|
Given that the original contributor has now given permission for the new licence should we revert this? |
|
Nah, this is better. |
Remove the -req-nodes flag from CA.pl
Rewrite ERR_string_error_n