Use u8 for internal verbosity level calculation

[joshka]

This avoids integer overflow / underflow errors when calculating the
verbosity level. Prior to this change, constructing a Verbosity with
either parameter equal to u8::MAX would result in incorrect verbosity
level calculation.

This change adds tests to ensure that the verbosity level is correctly
calculated for each log level. These were run before and after the
change to ensure that the change is correct. The tests using 255 as
either input value were failing before the change and passing after the
change.

---

Addresses comments left in #114:

• Implement Serialization and Deserialization for Verbosity #114 (comment)
• Implement Serialization and Deserialization for Verbosity #114 (comment)

I pulled this out to a separate PR to make it easier to just address the points made in this change. I hope that's not too much of a burden.

[joshka]

Copying out the main comment for future visibility:

A single -q would underflow (0u8 - 1u8). This would work fine in debug mode, and fail in release mode.

While in general that is indeed true that users calling the executable should not in general write paramaters that would cause it to crash, similarly apps should be robust in the face of users doing strange things. I don't think it's safe for a library to crash with an overflow given bad inputs like this. It may be better to clamp the total number of parameters parsed to some given number.

Given Clap is used to process command lines, that some commands will be part of administrative scripts running with elevated privileges, being able to control a crash in a command with specific parameters lead to potential (very low impact) CVEs.

It's possible that switching this internally to u8 would be a better idea, and make the OFF condition return 0 instead of -1. This would allow for u8 and saturating to 0 throughout (except for when calculating the delta between quote and verbosity).

I agree it makes sense to pull this change out as separate commit. I think it probably makes sense as a new PR for review purposes, or is the idea of this split to pull out this change strictly for commit history / message purposes?

[epage]

This change adds tests to ensure that the verbosity level is correctly
calculated for each log level. These were run before and after the
change to ensure that the change is correct.

A tip I've learned over time: Put tests like this in a commit prior to behavior changes, with it passing! This way, the behavior change PR shows how the behavior changed in practice by how the tests were changed.

[coveralls]

Pull Request Test Coverage Report for Build 11057801667

Details

• 15 of 18 (83.33%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+51.5%) to 66.667%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	15	18	83.33%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	1	66.67%

Totals
Change from base Build 10688474891:	51.5%
Covered Lines:	32
Relevant Lines:	48

---

💛 - Coveralls

[epage]

It's possible that switching this internally to u8 would be a better idea, and make the OFF condition return 0 instead of -1. This would allow for u8 and saturating to 0 throughout (except for when calculating the delta between quote and verbosity).

I think that would be a reasonable change. This could be done in its own commit before any clamping is done.

[joshka]

It's possible that switching this internally to u8 would be a better idea, and make the OFF condition return 0 instead of -1. This would allow for u8 and saturating to 0 throughout (except for when calculating the delta between quote and verbosity).

I think that would be a reasonable change. This could be done in its own commit before any clamping is done.

To be clear, that's what this PR is doing... switching to u8 making the OFF / None condition = 0 instead of -1

The clamping in this PR is required as part of changing to u8, because adding or subtracting u8 values requires i16 to represent the result. Then to translate the i16 to u8, it has to be clamped to the correct range (0, u8::MAX).

[joshka]

Update refactors the tests into a loop and adds characterization tests which are wrong in the first commit, and then fixes the problems in the second commit.

[epage]

Update refactors the tests into a loop and adds characterization tests which are wrong in the first commit, and then fixes the problems in the second commit.

FYI 7f435cd left the "wrong" parts out. I've adjusted your PR to show what I was intending when I asked for the test commit.

[joshka]

Gotcha. I was looking at this as move the value to be u8, so things have to become 0-based. And I think you were seeing at it as move to 0-based and once that happens it's easy to change to u8. Both end up at the same spot with a slight different journey, top down vs bottom up.

[epage]

Got be clear, my focus was not on the i8 -> u8 change but so that the change in tests in 9936958 shows how the behavior changed in contrast to a7f7bb2 which adds test cases with the behavior change, making it so you can't see how the behavior changed.