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

[Jonathan Tan <jonathantanmy@google.com>]

> Convert pickaxe_blame preprocessor constants in blame.h to an enum.
> Also replace previous instances of the constants with the new enum values.

First of all, thanks for your initiative in finding a microproject and
making a patch for it!

About your commit message title, I know that 50 characters is a soft
limit, but we should adhere to it if possible. Also, in Git, the letter
following the colon is usually in lowercase. So I would write it like:

  blame: make PICKAXE_BLAME_* an enum

(Feel free to use that or think of a different one.)

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

(This doesn't mean that you are wrong to remove them - I just gave my
opinion, and a reason for my opinion. If you had a reason to remove
them, you can mention that, and we can discuss this together. Or, if you
read my reason and agree with it, you can say that and put the
"PICKAXE_" back.)

> -#define PICKAXE_BLAME_MOVE		01
> -#define PICKAXE_BLAME_COPY		02
> -#define PICKAXE_BLAME_COPY_HARDER	04
> -#define PICKAXE_BLAME_COPY_HARDEST	010
> -
>  #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,
> +};

In Git, we often look at historical commits, so it is good to keep
history as clean as possible. In particular, we shouldn't move things
around unless we have another reason to. Here, for example, you are
moving the constants from above BLAME_DEFAULT_* to below. You should
move them back. (Or if you have a reason for moving, mention that and we
can discuss it.)

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

Apart from all that, one thing that I expected in this patch is the
changing of the type of local variables and parameters. For example, in
blame.c, I would have expected find_copy_in_parent() (for example) to
have its "opt" parameter changed from "int" to "enum
pickaxe_blame_option". One of the reasons why we want to use enums is
for type safety, and that can only be done if we use the enum type when
possible.