suricata icon indicating copy to clipboard operation
suricata copied to clipboard

Spelling

Open jsoref opened this issue 1 year ago • 6 comments

Make sure these boxes are signed before submitting your Pull Request -- thank you.

  • [x] I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
  • [x] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
  • [ ] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:

Describe changes:

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at https://github.com/jsoref/suricata/commit/ba63450813ff7b77c5d88f36be3fe32a067561fb#commitcomment-80730680

The action reports that the changes in this PR would make it happy: https://github.com/jsoref/suricata/commit/0de1df9b732895b346229aeee0abed3960006916

Warning this PR is incompatible with some of the workflows in this repository. Specifically, there's one that tries to build each commit of a PR. That takes about 2 minutes per commit. It could handle not more than 180 commits assuming it did no setup work. This PR has 300+ commits. GitHub historically has had a limit of 250 commits beyond which various pieces of its PR functionality has more or less broken.

Note I don't expect this PR to be merged as-is. If possible, I'd encourage someone to review commit by commit and consider the words being changed. In general a specific word change is either acceptable as a unit or should be rejected as a unit. It is possible to split by directory or file extension or some other algorithm, but doing that risks things where a change spans the criteria.

Note There's code formatter which doesn't actually seem happy with files in their current state on master, so they're effectively. a tripping hazard for anyone who might touch those files. At the risk of upsetting the formatter check, I've chosen not to apply its suggestions. But I encourage you to review its suggestions (which sadly are not provided in the workflow output) and consider how the project would want those files to be formatted. The easiest thing to do for this PR would be to drop the changes to those files.

Note this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.

I will try to annotate this PR, but it will take some time.

#suricata-verify-pr: #suricata-verify-repo: #suricata-verify-branch: #suricata-update-pr: #suricata-update-repo: #suricata-update-branch: #libhtp-pr: #libhtp-repo: #libhtp-branch:

jsoref avatar Aug 09 '22 13:08 jsoref

Overall I like this and would like to enable the action later. I guess my main issue right now is the 300+ commits. Do you see some sensible grouping? Like the "brand name" ones could all be together, but any other such grouping?

victorjulien avatar Aug 09 '22 15:08 victorjulien

Brand names indeed are the easiest thing to peel off.

It's possible that the .rst or doc/ stuff are sufficiently independent from the rest of the changes that they could be handled separately.

Otherwise, not really.

The majority of the changes are to ~200 .c files and there really wasn't anything particularly special about them.

Some groups ask me to split out comments from code changes, but that's incredibly dangerous/error-prone (in addition to being incredibly time consuming) -- speaking as someone who makes PRs like this frequently.

It's of course trivial for me to squash changes, but I prefer to do that as late in the process as possible -- it's much harder to unscramble eggs than it is to pull an intact egg out of a bowl.

jsoref avatar Aug 09 '22 16:08 jsoref

Overall I like this and would like to enable the action later. I guess my main issue right now is the 300+ commits. Do you see some sensible grouping? Like the "brand name" ones could all be together, but any other such grouping?

I second the grouping suggestion with the goal of a much smaller number of commits.

The groupings might be

  • branding
  • typos in source modules, configurations files (e.g., rust/src/* and src/*)
  • doc

jlucovsky avatar Aug 10 '22 14:08 jlucovsky

Ok. Would you like me to split two of those off into distinct PRs?

The easiest way for me to do this is:

  1. have a PR w/ my current style of commit messages
  2. people tell me which changes they don't like
  3. I drop them
  4. they say the changes are ok
  5. I squash and fix the commit message style to match house style

(For the remainder, which could be in this PR, I rebase once the other two are merged dropping things that have already been applied.)

jsoref avatar Aug 10 '22 14:08 jsoref

I still need to review the commits affecting code, but a couple of comments already:

  1. I'm fine with all commits in a single PR
  2. I think we can already do a few groupings: brand, everything under doc/userguide, everything under doc/devguide, all things affecting build files (configure.ac/Makefile.am)

victorjulien avatar Aug 10 '22 15:08 victorjulien

Ok. I'll just wait for you to confirm you're happy w/ things before I slice the changes (unless someone else wants to do the slicing, I'm happy to let someone else do it).

jsoref avatar Aug 10 '22 18:08 jsoref

Closing due to inactivity. If you're interested in picking this back up, please open a new PR addressing the comments. Thanks!

catenacyber avatar May 05 '23 16:05 catenacyber

I've successfully rebased this, new PR will be coming soon.

jasonish avatar May 05 '23 17:05 jasonish

Cool :-)

catenacyber avatar May 05 '23 18:05 catenacyber

@jsoref we've finally merged (#8829) this after much delay and git shuffling. Thanks a lot for your work. Are you interested doing an additional PR to cover the changes in master since this PR? It can be in just a single commit and then we'll rearrange things to our liking. We'd like to implement the github action once the tree is "clean". Let us know if you're still interested.

Thanks again!

victorjulien avatar May 06 '23 20:05 victorjulien

Sure

jsoref avatar May 07 '23 03:05 jsoref