Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 26, 2024, 09:59 UTC
Message-ID: <CAP8UFD3qR8E0gvUQtzzkLPWv4Db45kFS4pEqHKQr5siciVJ-zQ@mail.gmail.com>
In-Reply-To: <20240224112638.72257-2-giganteeugenio2@gmail.com>

On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante <giganteeugenio2@gmail.com> wrote:

>
> The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
> while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.

It's not clear from the patch that refresh() passes 'flags' as an argument to refresh_index(), so it might help reviewers a bit if you could tell that.

> Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
> this commit changes the type of 'flags' to unsigned.

We prefer to use "let's change this and that" or just "change this and that" rather than "this commit changes this and that", see https://git-scm.com/docs/SubmittingPatches/#imperative-mood.

It might help if you could add a bit more explanation about why it's a good thing to use an unsigned variable instead of a signed one. For example you could say that it documents that we are not doing anything funny with the MSB.

> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The patch looks correct, thanks!