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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/12/2015 08:15 PM, Junio C Hamano wrote:
> 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.

This would be an easy change but for the fact that REF_NODEREF is
defined in refs.h whereas the other constants are internal to refs.c. If
it's OK with you guys, I'd rather leave it the way it is for now and
come back to it later.

For example I want to rename the constants to REF_NODEREF ->
REF_NO_DEREF and REF_ISPRUNING -> REF_IS_PRUNING [1], but am leaving
that for when the refs code is not in so much flux. I can reorganize the
constants and docs then.

Michael

[1] As I type this I realize that the comment misspells the name of
REF_ISPRUNING. I'll fix that, too.

-- 
Michael Haggerty
mhagger@alum.mit.edu