Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2019, 04:13 UTC
Message-ID: <xmqqsgnzj4vs.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191010184439.94173-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 7 quoted lines

>> -	if ((opt & PICKAXE_BLAME_COPY_HARDEST)
>> -	    || ((opt & PICKAXE_BLAME_COPY_HARDER)
>> +	if ((opt & BLAME_COPY_HARDEST)
>> +	    || ((opt & BLAME_COPY_HARDER)
>
> Any reason why the names are renamed to omit "PICKAXE_"? In particular,
> these names are still global, so it is good to retain the extra context.

Absolutely.  Removing them is wrong, I would have to say.

Show 9 quoted lines

>>  #define BLAME_DEFAULT_MOVE_SCORE	20
>>  #define BLAME_DEFAULT_COPY_SCORE	40
>>  
>> +enum pickaxe_blame_action {
>> +	BLAME_MOVE = 01,
>> +	BLAME_COPY,
>> +	BLAME_COPY_HARDER = 04,
>> +	BLAME_COPY_HARDEST = 010,
>> +};

We had a bit of discussion recently about using (or rather, not abusing) enum for set of bits on a different topic.

> Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> line but that is not necessary.

That is absolutely necessary; it is not like "we do not care what exact value _COPY gets; it can be any value as long as it is _MOVE plus 1", as these are used in set of bits (and again, I do not think it is such a brilliant idea to use enum for such a purpose).

Thanks.