On Wed, 15 Feb 2023 at 23:19, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Vinayak Dev <vinayakdev.sci@xxxxxxxxx> 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?
It does appear that the macro binary_patch_method actually refers to
an unsigned long.
I think leading represents the number of leading context lines in a
hunk/fragment,
and the variable is reused to store the type for a binary fragment.
> > -#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.
Ok. Point noted.
> 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.
Removing the 1 _may_ be a safe thing to do, because the value
fragment->patch_method
is finally used in a switch statement, which makes it apparent that
the difference in the values
is actually necessary, and maybe not the actual value they hold (Also,
random but distinct
values still pass the test cases).
> > 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.
I think that since enums start with zero by default, you are right in
saying that the '=0' here can be removed.
I will do so.
> > 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.
It does appear to be so. Introducing a new enum binary_type_deflated
binary_patch_method inside of
struct fragment would indeed be a solution.
This could be plausible, because struct fragment does not appear outside of
apply.c. Your suggestion appears to be the only right way to do it, so
as to achieve the
goal of this patch series.
> 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.
apply.c actually seemed to be the best place to start such a change, because the
macros in the file suit such a change the best for a start. It would
have brought
unnecessary overhead in many other files, specifically those which
would affect a _lot_ of
the source code. It is just that the changes appear contained enough
to not cause a lot of
noise.
Your suggestions are very correct, I will make sure to keep them in
mind in the future.
Thanks a lot!
Vinayak
