Christian Couder <christian.couder@xxxxxxxxx> writes: > On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante > <giganteeugenio2@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> builtin/add.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > The patch looks correct, thanks!
