From: Junio C Hamano Date: Mon, 26 Feb 2024 22:58:39 GMT Subject: Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits Message-ID: In-Reply-To: Christian Couder writes: > On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante > 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 >> --- >> builtin/add.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > The patch looks correct, thanks!