Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: >> 'diff_filespec_is_binary' inside 'diff.c' would have to be changed. > > The code in question is not being used as a "bag of bits". Rather, > it's a tristate binary with values "not-set", "true", and "false". > Whereas a typical binary could be represented by a single bit, this > one needs the extra bit to handle the "not-set" case. Moreover, it is > idiomatic in the Git codebase for -1 to represent "not-set", so I > think this code is fine as-is since its meaning is clear to those > familiar with the codebase, thus does not need any changes made to it. Correct. In general, bitfield structure members in our codebase should be already fine. Most of them are "unsigned : 1" and there is not enough room in a single-bit bitfield to go signed. > There are cases in the codebase in which a signed type is being used > as a "bag of bits" instead of the more desirable unsigned type. > ... > So, refresh_index() is correctly expecting an unsigned value for > `flags` but refresh() in `builtin/add.c` has undesirably declared > `flags` as signed. Thanks for a good example. In a signed flag word that is used as a bag of bits, the MSB is special, and unless you take advantage of that special casing (which happens almost never), you should use an unsigned word instead, to document that you are not doing anything funny with the MSB, like "the flag word is negative, so the MSB must be on", "I want to copy the bit immediately below MSB to MSB", etc.
