Re: [GSoC][PATCH v3] apply: Change #define to enum and variable types from int to enum

[Junio C Hamano <gitster@pobox.com>]

Vinayak Dev <vinayakdev.sci@gmail.com> writes:

> diff --git a/apply.c b/apply.c
> index 5cc5479c9c..b2a03d9fc3 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -205,8 +205,10 @@ struct fragment {
>   * or deflated "literal".
>   */
>  #define binary_patch_method leading

Notice and explain what this line is doing, perhaps?

> -#define BINARY_DELTA_DEFLATED	1
> -#define BINARY_LITERAL_DEFLATED 2
> +enum binary_type_deflated {
> +	BINARY_DELTA_DEFLATED = 1,
> +	BINARY_LITERAL_DEFLATED
> +};

These days, we not just allow but encourage enum definitions to have
a trailing comma after the last item, UNLESS we want to signal that
the one at the end right now MUST stay to be the last one (e.g. a
sentinel at the end).

A patch that adds a new item to, removes an existing item from, or
shuffles existing items in the list can be free of unnecessary patch
noise to omit the last comma.

As a faithful rewrite, forcing the same values to be given as before
by saying that "_DEFLATED must be 1" is a good thing to do, but once
the dust settled from the patch, it would be a good idea to go back
to the code and see if the values MUST be these, or if it is fine to
use any value as long as they are distinct.  If it is the latter,
then it would make a good follow-up patch to remove "= 1", with an
explanation why it is a safe thing to do.

>  static void free_fragment_list(struct fragment *list)
>  {
> @@ -918,14 +920,17 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,
>   * their names against any previous information, just
>   * to make sure..
>   */
> -#define DIFF_OLD_NAME 0
> -#define DIFF_NEW_NAME 1
> +
> +enum diff_name {
> +	DIFF_OLD_NAME = 0,
> +	DIFF_NEW_NAME
> +};

Ditto.

>  static int gitdiff_verify_name(struct gitdiff_data *state,
>  			       const char *line,
>  			       int isnull,
>  			       char **name,
> -			       int side)
> +			       enum diff_name side)
>  {
>  	if (!*name && !isnull) {
>  		*name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);
> @@ -1910,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
>  	int llen, used;
>  	unsigned long size = *sz_p;
>  	char *buffer = *buf_p;
> -	int patch_method;
> +	enum binary_type_deflated patch_method;

This is not quite sufficient to achieve the goal of helping "modern
debuggers" that was stated in the proposed log message, is it?
parse_binary_hunk() copies the value from this local variable to a
member in the fragment being parsed, like so:

	frag->binary_patch_method = patch_method;

but the thing is, as we have seen earlier, a compiler macro is used
to (ab)use the "leading" member and call it "binary_patch_method".
The type of that member is "unsigned long".

Now if our ultimate goal were to use enum instead of macro, then an
obvious "solution" would be to stop abusing "leading".  Instead, you
would add "enum binary_type_deflated binary_patch_method" member to
the fragment struct and use the enum throughout.

But is it worth it?

Using enum instead of macro is *NOT* a goal.  If doing so makes our
code better, we may do so---which tells us that use of enum is not a
goal but a means to make our code better.  Is adding an extra member
that is used only for binary patches improve our code?  I dunno.

Thanks.