I'm doubtful of this fix, but as far as I can see, checking enums for validity has always been a perilous business. The alternative is to cast before checking, but it gets messy because the signedness of an enum is compiler-dependent. Since I believe this argument is never "user-controlled", I'm tempted to remove the "useless" test.

What is the specific warning here? This looks like the error is that this should be || instead of && as a a < 0 ∧ a > b where b > 0 is never going to be true.

…/libgit2/src/path.c:1890:14: Comparison of unsigned enum expression < 0 is always false

Changing to || doesn't fix the warning, but I can do the following, which does :

if ((signed int)gitfile < 0 || gitfile >= ARRAY_SIZE(gitfiles))

and I think gitfile >= ARRAY_SIZE(gitfile) would have caught it, since it now looks like a very big unsigned int.

Hm right, this only seems to be an older clang thing. I don't know what its version 900 corresponds to in "real" clang versions, but on Debian I see this with clang 5 but not with 6. I don't know if they've decided to change the signedness of enums, but this is up to the particular implementation so removing this check would break it for some (apparently including later clang versions).

I'd be fine with just removing the check altogether. As @tiennou said, this function is not exposed, and all of our internal callers make sure to just pass proper enum values.

Apple LLVM version 9.1.0 (clang-902.0.39.2)
Target: x86_64-apple-darwin17.7.0

included in Xcode 9.4.1 (which doesn't feel that old). I'm 👍 with removing the check, because I don't think there's a clean way to do it otherwise.

It's internal now, but it is useful outside of the library and when we expose it we'd have to content with random inputs (I've been meaning to expose this for a while as the fork I send to production has this as public).

	if (!(gitfile > 0 && gitfile <= ARRAY_SIZE(gitfiles))) {

does not produce the warning for me in clang 5 and it also fixes the logic.

Ok, I've used this, fixed up, and rebased.

I normally wouldn't like relying on the fact that this enum is actually the first one. But there already is a comment in "src/path.h", stating that the enum is sensitive to ordering. So this is fine

I'm also quite sure there's no simple way to check for an enum signedness, without resorting to build-system shenanigans or explicitly making the enum signed. Hence the test case I've added, which should hopefully catch a change in definition.