-
Notifications
You must be signed in to change notification settings - Fork 6k
8285380: Fix typos in security #8340
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
Conversation
👋 Welcome back ihse! A progress list of the required criteria for merging this PR into |
Webrevs
|
The folks on security-dev will know for sure but I assume that the changes to the imported Apache Santuario code should be dropped as it will make upgrades more complicated. |
@AlanBateman So there is even more 3rd party code in there? :-( I tried to ignore fixes for all files that I could identify as 3rd party. It's actually a bit annoying that we have imported source code thrown around like this in the source tree, so there is no clear boundary between code we own and code we import from someone else... |
security-dev can say for sure but the only 3rd party code I see in this change is in the src/java.xml.crypto/share/classes/com/sun/org/apache tree (the package name gives a hint has it was it was re-packaged). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks.
@magicus This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 58 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
The However, I am ok with including the spelling fixes for the 3rd-party Apache code (this tree and the one Alan mentions above which you already reverted, but can put it back) with this PR. I am a Committer on the Apache Santuario project so I can push these spelling fixes (they should be non-controversial) to their code base and there won't be any conflicts when we merge up the JDK to the next Santuario release. |
/integrate |
Going to push as commit f631c98.
Your commit was automatically rebased without conflicts. |
I ran
codespell
on modules owned by the security team (java.security.jgss java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss
), and accepted those changes where it indeed discovered real typos.I will update copyright years using a script before pushing (otherwise like every second change would be a copyright update, making reviewing much harder).
The long term goal here is to make tooling support for running
codespell
. The trouble with automating this is of course all false positives. But before even trying to solve that issue, all true positives must be fixed. Hence this PR.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8340/head:pull/8340
$ git checkout pull/8340
Update a local copy of the PR:
$ git checkout pull/8340
$ git pull https://git.openjdk.java.net/jdk pull/8340/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8340
View PR using the GUI difftool:
$ git pr show -t 8340
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8340.diff