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

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

Christian Couder <christian.couder@gmail.com> writes:

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

Perhaps.

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

Very true.

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

But doesn't the proposed log message already say so?

In any case, it would very much help to fold long lines and have a
blank line in between paragraphs to make the log message more
readable.

Thanks, both.



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