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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2024, 22:58 UTC
Message-ID: <xmqqsf1ekf34.fsf@gitster.g>
In-Reply-To: <CAP8UFD3qR8E0gvUQtzzkLPWv4Db45kFS4pEqHKQr5siciVJ-zQ@mail.gmail.com>

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

Show 10 quoted lines

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

Show 7 quoted lines

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

Show 7 quoted lines

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