Skip to content
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

Fixes #256: various coverity issues in the HTTP/1 adaptor #657

Merged
merged 3 commits into from
Aug 8, 2022

Conversation

kgiusti
Copy link
Contributor

@kgiusti kgiusti commented Aug 4, 2022

No description provided.

@kgiusti kgiusti requested a review from ganeshmurthy August 4, 2022 19:26
@kgiusti kgiusti linked an issue Aug 4, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #657 (67484cc) into main (7e42ed1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
- Coverage   25.91%   25.90%   -0.01%     
==========================================
  Files         128      128              
  Lines       31186    31189       +3     
  Branches     4972     4972              
==========================================
  Hits         8081     8081              
- Misses      22050    22053       +3     
  Partials     1055     1055              
Flag Coverage Δ
unittests 25.90% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/adaptors/http1/http1_server.c 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jiridanek
Copy link
Contributor

@kgiusti once more... with feeling...]

you should be able to run scripts/git-clang-format origin/main --diff on your machine; it will format everything that changed from what's in origin/main. maybe git fetch before, to have accurate origin/main content. and remove --diff to directly apply changes

@kgiusti
Copy link
Contributor Author

kgiusti commented Aug 8, 2022

@kgiusti once more... with feeling...]

you should be able to run scripts/git-clang-format origin/main --diff on your machine; it will format everything that changed from what's in origin/main. maybe git fetch before, to have accurate origin/main content. and remove --diff to directly apply changes

Ok - good to know.

That second clang-format failure (fixed by the last commit) surprised me since - once I fetched/rebased on the latest main with the pre-commit hook - I was able to check in what was apparently a clang-format violation.

Here's the workflow that failed:

  1. make change to code
  2. git add changed files
  3. run git clang-format
  4. audit clang-format changes - all good
  5. git add again to pick up changes
  6. git commit - succeeded!
  7. Profit!!!

Best I can tell is that I would've caught the second clang-format issue if I re-ran git clang-format again after adding the fixes from the previous run of git clang-format. Essentially adding step 5.a: run git clang-format, then repeat until Profit!

I think the reason for that may be there were two clang-format violations on the same line of code (align vars, then align equal sign).

What's funny is that step 6 commit worked even though the equal signs were unaligned.

@kgiusti kgiusti merged commit 69dcda7 into skupperproject:main Aug 8, 2022
@kgiusti kgiusti deleted the ISSUE-256 branch August 8, 2022 14:08
@jiridanek
Copy link
Contributor

@kgiusti That second clang-format failure (fixed by the last commit) surprised me since - once I fetched/rebased on the latest main with the pre-commit hook - I was able to check in what was apparently a clang-format violation.

The pre-commit only checks what you are currently committing (with git commit). It does not check commits that are already committed and you are only rebasing them. There is a command (pre-commit run --all-files, I think, or something like that) that will check it all, but it does not work with my git clang-format action, because that is written to always check outstanding changes... It is the problem from #660 (comment) that the tooling only works on file-level, and checking changed lines has to be done hackishly.

Things will be much better when whole codebase is formatted, then we can run clang-format the way git-precommit.com means it to be run!

But when you know what the clang-format GHA is running and you run it yourself, it should be manageable, I hope. If there was a good way to figure out base commit when running pre-commit, then the precommit action could run the same thing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various Coverity issues found in http1 adaptor
4 participants