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

[Jonathan Tan <jonathantanmy@google.com>]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> -	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.

Thanks for clarifying.

> >>  #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.

For reference, the discussion is here [1]. Szeder Gábor has a dissenting
argument in [2] that makes sense to me (gdb on my desktop also shows an
enum used as a bitset as "(A | B)"). But in any case, perhaps we should
decide if it's fine to use an enum here before Wambui Karuga continues
development.

[1] https://public-inbox.org/git/xmqqsgobg0rv.fsf@gitster-ct.c.googlers.com/
[2] https://public-inbox.org/git/20191007171249.GB11529@szeder.dev/

> > 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).

Good point.