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

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

[Wambui Karuga]

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

Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com>
---
 blame.c         |  8 ++++----
 blame.h         | 12 +++++++-----
 builtin/blame.c | 17 ++++++++---------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/blame.c b/blame.c
index 36a2e7ef11..4ad86d1217 100644
--- a/blame.c
+++ b/blame.c
@@ -2183,8 +2183,8 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,
 	 * and this code needs to be after diff_setup_done(), which
 	 * usually makes find-copies-harder imply copy detection.
 	 */
-	if ((opt & PICKAXE_BLAME_COPY_HARDEST)
-	    || ((opt & PICKAXE_BLAME_COPY_HARDER)
+	if ((opt & BLAME_COPY_HARDEST)
+	    || ((opt & BLAME_COPY_HARDER)
 		&& (!porigin || strcmp(target->path, porigin->path))))
 		diff_opts.flags.find_copies_harder = 1;
 
@@ -2429,7 +2429,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,
 	/*
 	 * Optionally find moves in parents' files.
 	 */
-	if (opt & PICKAXE_BLAME_MOVE) {
+	if (opt & BLAME_MOVE) {
 		filter_small(sb, &toosmall, &origin->suspects, sb->move_score);
 		if (origin->suspects) {
 			for (i = 0, sg = first_scapegoat(revs, commit, sb->reverse);
@@ -2448,7 +2448,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,
 	/*
 	 * Optionally find copies from parents' files.
 	 */
-	if (opt & PICKAXE_BLAME_COPY) {
+	if (opt & BLAME_COPY) {
 		if (sb->copy_score > sb->move_score)
 			filter_small(sb, &toosmall, &origin->suspects, sb->copy_score);
 		else if (sb->copy_score < sb->move_score) {
diff --git a/blame.h b/blame.h
index 4a9e1270b0..a4bbf3a8c5 100644
--- a/blame.h
+++ b/blame.h
@@ -8,14 +8,16 @@
 #include "prio-queue.h"
 #include "diff.h"
 
-#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,
+};
+
 /*
  * One blob in a commit that is being suspected
  */
diff --git a/builtin/blame.c b/builtin/blame.c
index b6534d4dea..fb71517b5c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -771,11 +771,11 @@ static int blame_copy_callback(const struct option *option, const char *arg, int
 	 * -C -C -C enables copy from existing files for
 	 *          everybody
 	 */
-	if (*opt & PICKAXE_BLAME_COPY_HARDER)
-		*opt |= PICKAXE_BLAME_COPY_HARDEST;
-	if (*opt & PICKAXE_BLAME_COPY)
-		*opt |= PICKAXE_BLAME_COPY_HARDER;
-	*opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;
+	if (*opt & BLAME_COPY_HARDER)
+		*opt |= BLAME_COPY_HARDEST;
+	if (*opt & BLAME_COPY)
+		*opt |= BLAME_COPY_HARDER;
+	*opt |= BLAME_COPY | BLAME_MOVE;
 
 	if (arg)
 		blame_copy_score = parse_score(arg);
@@ -788,8 +788,7 @@ static int blame_move_callback(const struct option *option, const char *arg, int
 
 	BUG_ON_OPT_NEG(unset);
 
-	*opt |= PICKAXE_BLAME_MOVE;
-
+	*opt |= BLAME_MOVE;
 	if (arg)
 		blame_move_score = parse_score(arg);
 	return 0;
@@ -993,8 +992,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	blame_date_width -= 1; /* strip the null */
 
 	if (revs.diffopt.flags.find_copies_harder)
-		opt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |
-			PICKAXE_BLAME_COPY_HARDER);
+		opt |= (BLAME_COPY | BLAME_MOVE |
+			BLAME_COPY_HARDER);
 
 	/*
 	 * We have collected options unknown to us in argv[1..unk]
-- 
2.23.0

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

[Jonathan Tan]

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

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

[Junio C Hamano]

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.

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

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

[Jonathan Tan]

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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

Doesn't that also show that enums are not quite a good fit for set
of bits (i.e. 1<<n)?

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

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> > 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.
> 
> Doesn't that also show that enums are not quite a good fit for set
> of bits (i.e. 1<<n)?

Well, I agree that it could be better, but with the C language as we
have it now, I still slightly prefer an enum to a list of #define. Both
ways, we still have to manually enter values for each flag, but with
enum, we get better debugger display (at least in gdb) and in the
function declaration and definition, we can put a specific type (instead
of "unsigned" or "int"). gdb supports the notion that a few people use
enums this way too, but if we decide as a project to not use enums in
this way, that's fine too. For what it's worth, I tried doing a search
online, but most of the results I got was for C# (where it is
recommended - they have a "[Flags]" attribute for enums), so maybe I am
indeed in the minority.

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

[Wambui Karuga]

On Mon, Oct 14, 2019 at 11:27:54AM -0700, Jonathan Tan wrote:
> > Jonathan Tan <jonathantanmy@google.com> writes:
> > 
> > >> > 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.
> > 
> > Doesn't that also show that enums are not quite a good fit for set
> > of bits (i.e. 1<<n)?
> 
> Well, I agree that it could be better, but with the C language as we
> have it now, I still slightly prefer an enum to a list of #define. Both
> ways, we still have to manually enter values for each flag, but with
> enum, we get better debugger display (at least in gdb) and in the
> function declaration and definition, we can put a specific type (instead
> of "unsigned" or "int"). gdb supports the notion that a few people use
> enums this way too, but if we decide as a project to not use enums in
> this way, that's fine too. For what it's worth, I tried doing a search
> online, but most of the results I got was for C# (where it is
> recommended - they have a "[Flags]" attribute for enums), so maybe I am
> indeed in the minority.

I'll try to pick another set of constants to convert - before this is
agreed on.

Thanks,
wambui karuga

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

[Jonathan Tan]

> > Well, I agree that it could be better, but with the C language as we
> > have it now, I still slightly prefer an enum to a list of #define. Both
> > ways, we still have to manually enter values for each flag, but with
> > enum, we get better debugger display (at least in gdb) and in the
> > function declaration and definition, we can put a specific type (instead
> > of "unsigned" or "int"). gdb supports the notion that a few people use
> > enums this way too, but if we decide as a project to not use enums in
> > this way, that's fine too. For what it's worth, I tried doing a search
> > online, but most of the results I got was for C# (where it is
> > recommended - they have a "[Flags]" attribute for enums), so maybe I am
> > indeed in the minority.
> 
> I'll try to pick another set of constants to convert - before this is
> agreed on.

Thanks - perhaps that's the best way to proceed for now. Do you remember
where you found the idea to convert #define to enum? Maybe I could add a
note there to avoid converting bitsets/bitflags.

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

[Wambui Karuga]

On Mon, Oct 14, 2019 at 02:46:29PM -0700, Jonathan Tan wrote:
> > > Well, I agree that it could be better, but with the C language as we
> > > have it now, I still slightly prefer an enum to a list of #define. Both
> > > ways, we still have to manually enter values for each flag, but with
> > > enum, we get better debugger display (at least in gdb) and in the
> > > function declaration and definition, we can put a specific type (instead
> > > of "unsigned" or "int"). gdb supports the notion that a few people use
> > > enums this way too, but if we decide as a project to not use enums in
> > > this way, that's fine too. For what it's worth, I tried doing a search
> > > online, but most of the results I got was for C# (where it is
> > > recommended - they have a "[Flags]" attribute for enums), so maybe I am
> > > indeed in the minority.
> > 
> > I'll try to pick another set of constants to convert - before this is
> > agreed on.
> 
> Thanks - perhaps that's the best way to proceed for now. Do you remember
> where you found the idea to convert #define to enum? Maybe I could add a
> note there to avoid converting bitsets/bitflags.

I found it in a mailing list thread[1], but seems that a comment on bit
field values was added to a similar issue on GitGitGadget[2].

[1] https://public-inbox.org/git/20190923180649.GA2886@szeder.dev/
[2] https://github.com/gitgitgadget/git/issues/357

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

[SZEDER Gábor]

On Mon, Oct 14, 2019 at 11:27:54AM -0700, Jonathan Tan wrote:
> > Jonathan Tan <jonathantanmy@google.com> writes:
> > 
> > >> > 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.
> > 
> > Doesn't that also show that enums are not quite a good fit for set
> > of bits (i.e. 1<<n)?
> 
> Well, I agree that it could be better, but with the C language as we
> have it now, I still slightly prefer an enum to a list of #define. Both
> ways, we still have to manually enter values for each flag, but with
> enum, we get better debugger display (at least in gdb) and in the
> function declaration and definition, we can put a specific type (instead
> of "unsigned" or "int").

And then with any halfway decent editor (i.e. with ctags support) we
can jump to the definition of that enum, see all the constants and
read the comments.  A proper IDE might even display all that as a
tooltip when hovering over.

> gdb supports the notion that a few people use
> enums this way too, but if we decide as a project to not use enums in
> this way, that's fine too.

In a curious coincidence, today we celebrate the 12th anniversary of
Git starting using an enum as a set of bitflags.  Yay! :)

> For what it's worth, I tried doing a search
> online, but most of the results I got was for C# (where it is
> recommended - they have a "[Flags]" attribute for enums), so maybe I am
> indeed in the minority.

I looked around among the various sources I have lying around, and
found enums used as a set of bitflags in libgit2, linux, gcc, glibc,
curl, vim, dash (but not Bash :).


Anyway.  Using an enum for a set of related bitflags has some very
practical benefits.  OTOH, I'm still not sure what its drawbacks are,
especially what those drawbacks are that outweight the benefits.

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

[Wambui Karuga]

On Thu, Oct 10, 2019 at 11:44:39AM -0700, Jonathan Tan wrote:
> > 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.)
Okay, thanks for the suggestion. I'll use it.
> 
> > -	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.)
> 
I wasn't really sure about omitting the "PICKAXE_" prefix, but I looked
at some of the other defined enums and it seemed like what would act as
the prefix in #defines was only used in the enum declaration. For
example I looked at:
	enum apply_ws_error_action {
		nowarn_ws_error,
		warn_on_ws_error,
		die_on_ws_error,
		correct_ws_error
	};

For comparison, I took "apply_" as the prefix that would translate to 
"#define APPLY_" which isn't included in the member variables.
I do agree about retaining the extra context though, so I can definitely 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.)
> 
I'll move them back. I have experience with all the "#define" constants
being immediately after the "#includes" which is why I moved them, but I'll try to stick to the
convention from now on.

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

> 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.
I overlooked this, sorry for that.
I'll send an updated patch with these corrections.

Thanks!

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

[Jonathan Tan]

> > 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.)
> > 
> I wasn't really sure about omitting the "PICKAXE_" prefix, but I looked
> at some of the other defined enums and it seemed like what would act as
> the prefix in #defines was only used in the enum declaration. For
> example I looked at:
> 	enum apply_ws_error_action {
> 		nowarn_ws_error,
> 		warn_on_ws_error,
> 		die_on_ws_error,
> 		correct_ws_error
> 	};
> 
> For comparison, I took "apply_" as the prefix that would translate to 
> "#define APPLY_" which isn't included in the member variables.
> I do agree about retaining the extra context though, so I can definitely put the
> "PICKAXE_" back.

[snip]

> > 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.)
> > 
> I'll move them back. I have experience with all the "#define" constants
> being immediately after the "#includes" which is why I moved them, but I'll
> try to stick to the
> convention from now on.

Thanks for providing your justifications.

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

Well, Junio provides a good reason for putting "= 02" [1], so please do
that.

[1] https://public-inbox.org/git/xmqqsgnzj4vs.fsf@gitster-ct.c.googlers.com/

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

[Wambui Karuga]

On Fri, Oct 11, 2019 at 11:48:03AM -0700, Jonathan Tan wrote:
> > > 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.)
> > > 
> > I wasn't really sure about omitting the "PICKAXE_" prefix, but I looked
> > at some of the other defined enums and it seemed like what would act as
> > the prefix in #defines was only used in the enum declaration. For
> > example I looked at:
> > 	enum apply_ws_error_action {
> > 		nowarn_ws_error,
> > 		warn_on_ws_error,
> > 		die_on_ws_error,
> > 		correct_ws_error
> > 	};
> > 
> > For comparison, I took "apply_" as the prefix that would translate to 
> > "#define APPLY_" which isn't included in the member variables.
> > I do agree about retaining the extra context though, so I can definitely put the
> > "PICKAXE_" back.
> 
> [snip]
> 
> > > 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.)
> > > 
> > I'll move them back. I have experience with all the "#define" constants
> > being immediately after the "#includes" which is why I moved them, but I'll
> > try to stick to the
> > convention from now on.
> 
> Thanks for providing your justifications.
> 
> > > Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> > > line but that is not necessary.
> > > 
> > Noted.
> 
> Well, Junio provides a good reason for putting "= 02" [1], so please do
> that.
> 
> [1] https://public-inbox.org/git/xmqqsgnzj4vs.fsf@gitster-ct.c.googlers.com/

Yes, I'll include it in an updated patch
Thanks

wambui karuga