Re: [PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> -       int flags; /* REF_NODEREF? */
>> -       int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>> +       /*
>> +        * One or more of REF_HAVE_OLD, REF_NODEREF,
>> +        * REF_DELETING, and REF_IS_PRUNING:
>> +        */
>> +       int flags;
>
> Nit:
> I'd find it more readable if it would be:
>     /*
>      * One or more of
>      * REF_HAVE_OLD,
>      * REF_NODEREF,
>      * REF_DELETING,
>      * REF_IS_PRUNING:
>      * whose definition is found at the top of this file.
>      */

I wouldn't do either, though, as you would have to keep repeating
yourself here and over there.  Wouldn't it be sufficient to:

 - Have a header that says "these are bits meant to go to struct
   ref_update.flags" at the beginning of series of #define's;

 - Say "ref_update.flags bits are defined above" here.  The phrasing
   can be "One or more of REF_HAVE_OLD, etc. defined above." as long
   as it is clear that this is not meant to be an exhausitive list.

Also, unless we are taking advantage of the fact that MSB is special
in signed integral types [*1*], I would prefer to see us use an
unsigned type to store these bits in a variable of an integral type.
That would let the readers assume that they have fewer tricky things
possibly going on in the code.


[Footnote]

*1* For example, you can give the MSB to the REF_ERROR bit, and say

	if (structure->flags < 0)
        	there is an error;
	else
        	other things;

only if flags is a signed type.  Also if you are relying on the fact
that MSB is special in this kind of thing:

	structure->flags >>= 3;

then you obviously cannot use unsigned type for collection of bits.