Re: [PATCH] fixup! upload-pack: change multi_ack to an enum

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 2, 2020, 19:28 UTC
Message-ID: <CAP8UFD3gKCh8qkzE--M5ZiJmJmnRzbPJgg=kLW3HxJHMk9x26Q@mail.gmail.com>
In-Reply-To: <20200602190521.32877-1-jonathantanmy@google.com>

On Tue, Jun 2, 2020 at 9:05 PM Jonathan Tan <jonathantanmy@google.com> wrote:

>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

Thanks for the patch!

> I think enum values should be all-caps, so here is a fixup for that.

It's true that they are often all-caps, but there are a number of places where some enum values are lower case like:

- apply.h
- dir.c
- pretty.c
...

> I also fixed a spacing issue (2 spaces between "enum" and "{").

Thanks for that.

Show 13 quoted lines

> Also, maybe replace the first paragraph of the 1st patch:
>
>   As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
>   more thoroughly, let's actually start using some bitfields of
>   that struct, which were previously unused.
>
> with:
>
>   As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
>   more thoroughly, let's actually start using some bitfields of
>   that struct. These bitfields were introduced in 3145ea957d
>   ("upload-pack: introduce fetch server command", 2018-03-15) but were
>   never used.

Yeah, it seems better.

> Other than that, this patch set looks good to me.

Thank you for your review. I am ok to resend a V3 of the part 2 patch series with all these changes. If having all-caps enum values is not required though, I wonder if it's worth resending everything.