{"thread":{"id":"63497","subject":"[Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","startedAt":"2025-05-21T20:24:28Z","lastAt":"2025-05-30T08:13:18Z","messageCount":6,"participants":["fernando gouveia lima fernandolimabusiness@gmail.com","Junio C Hamano","Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518612","messageId":"20250521202409.26879-1-fernandolimabusiness@gmail.com","threadId":"63497","inReplyTo":null,"subject":"[Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"","fromEmail":"fernando gouveia lima fernandolimabusiness@gmail.com","sentAt":"2025-05-21T20:24:09Z","receivedAt":"2025-05-21T20:24:28Z","isPatch":true,"sender":{"key":"fernando gouveia lima fernandolimabusiness@gmail.com","avatar":null},"body":"From: Fernando Gouveia Lima <fernandolimabusiness@gmail.com>\n\nTwo comparisions between int and size_t, and int and unsigned long int\ncause warning sign compare to fire.\n\nAvoid this by changing the type of variable \"i\" in add_ref_decoration() to unsigned long int and\ncasting the variable \"filename->len\" to int in fmt_output_subject().\n\nSigned-off-by: Fernando Gouveia Lima <fernandolimabusiness@gmail.com>\n---\nThis is my first contribution and i got this idea on the Microproject\npage. I hope this is the first of many contributions to the git\ncommunity :).\n\n log-tree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 1d05dc1c70..e0848fcccc 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,\n \t\t\t      int flags UNUSED,\n \t\t\t      void *cb_data)\n {\n-\tint i;\n+\tlong unsigned int i;\n \tstruct object *obj;\n \tenum object_type objtype;\n \tenum decoration_type deco_type = DECORATION_NONE;\n@@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,\n \t}\n \tstrbuf_addf(filename, \"%04d-%s\", nr, subject);\n \n-\tif (max_len < filename->len)\n+\tif (max_len < (int) filename->len)\n \t\tstrbuf_setlen(filename, max_len);\n \tstrbuf_addstr(filename, suffix);\n }\n-- \n2.34.1\n\n"},{"id":"518616","messageId":"xmqqsekx8yef.fsf@gitster.g","threadId":"63497","inReplyTo":"20250521202409.26879-1-fernandolimabusiness@gmail.com","subject":"Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T21:08:40Z","receivedAt":"2025-05-21T21:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:\n\n> @@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,\n>  \t\t\t      int flags UNUSED,\n>  \t\t\t      void *cb_data)\n>  {\n> -\tint i;\n> +\tlong unsigned int i;\n>  \tstruct object *obj;\n>  \tenum object_type objtype;\n>  \tenum decoration_type deco_type = DECORATION_NONE;\n\nThe complaint is about this line of code:\n\n\tfor (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {\n\nwhere ref_namespace array is of NAMESPACE__COUNT size, which is the\nlast enum in \"enum ref_namespace\".  Anybody can tell it is a small\ninteger (like 9) that would fit comfotably well within the platform\nnatural \"int\" on anybody that can run Git.\n\nARRAY_SIZE() is defined in turns of sizeof(), so its type is \"size_t\";\nit cannot be narrower than \"int\", but that is immaterial.  Comparing\n'i' that counts up from 0 will never cause a problem.\n\nAnd the compiler ought to know all of the above, or should stay\nsilent.\n\nThe compiler is wrong in this case.  As I already have said on this\nlist a few times, -Wsign-compare is often garbage and we should not\nbend over backwards to butcher perfectly good code only to silence\nits false positives.\n\n> @@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,\n>  \t}\n>  \tstrbuf_addf(filename, \"%04d-%s\", nr, subject);\n>  \n> -\tif (max_len < filename->len)\n> +\tif (max_len < (int) filename->len)\n>  \t\tstrbuf_setlen(filename, max_len);\n\nThis conversion is wrong, even if your compiler is made silent with\nthis change.  The function begins like this:\n\n        void fmt_output_subject(struct strbuf *filename,\n                                const char *subject,\n                                struct rev_info *info)\n        {\n                const char *suffix = info->patch_suffix;\n                int nr = info->nr;\n                int start_len = filename->len;\n                int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);\n\nThe filename variable is a pointer to a strbuf, whose .len member is\nof type size_t, which can be wider than a platform natural \"int\".\nAssigning it to \"int start_len\" risks overflowing and wrapping\naround, and \"int max_len\" is also wrong.\n\nSo casting a large filename->len that would not fit within platform\nnatural \"int\" may make your compiler silent when you compare it with\nmax_len that is also platform natural \"int\".  But at that point,\nwhat are you comparing with what?  Imagine on a platform with 32-bit\nint and when the actual value of filename->len is very very long and\nlonger than say 3 billion bytes.\n\nIn this function, I think a more reasonable thing to do is to\nexpress various length invoved in size_t, and make sure computations\namong them does not over/underflow.\n\n\tSide note: Those who are maintaining GSoC microproject ideas\n\tpage.  This is the second time in a few days that I had to\n\tlook at a patch that makes the code worse by blindly trying\n\tto squelch the -Wsign-compare warnings.  Can you please take\n\tthat out of the list of suggested tasks?  Thanks.\n\nFernando, please do not get discouraged by _our_ code being sloppy\nand GSoC microproject ideas page being under-specified.  Neither is\nyour fault.\n\nAnd welcome to Git development community.\n\n\n[Footnote]\n\nQuite honestly, -Wsign-compare is mostly garbage [*] and I wish we\ndid not add it to the developer settings.  A more effective way to\nsquelch them is not by sprinkling the casts like this, but to remove\nit from config.mak.dev ;-)\n\nhttps://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n"},{"id":"518770","messageId":"aDCQWr3MBX4L7sbA@pks.im","threadId":"63497","inReplyTo":"xmqqsekx8yef.fsf@gitster.g","subject":"Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-23T15:12:26Z","receivedAt":"2025-05-23T15:12:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 21, 2025 at 02:08:40PM -0700, Junio C Hamano wrote:\n> Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:\n> Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we\n> did not add it to the developer settings.  A more effective way to\n> squelch them is not by sprinkling the casts like this, but to remove\n> it from config.mak.dev ;-)\n> \n> https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n\nI'm still not of the opinion that it is garbage. We have tons of\nlocations where we mismatch integer types only because we never got a\nwarning from the compiler, and these have caused multiple stack\noverflows in the past. The signal-to-noise ratio is high, that much is\ncertainly true. But if it helps us to avoid security issues in the\nfuture I think that is acceptable.\n\nI do agree though this not a good project for newcomers, as fixing those\nbugs is quite intricate overall. So we should definitely remove this\nproject from the microprojects page.\n\nAnd in case I'm the only one who thinks that the warning has merit I'm\nalso happy to be overruled and have it be removed from our developer\nsettings.\n\nPatrick\n"},{"id":"518771","messageId":"aDCTbF3Vm+Ob3Osh@nand.local","threadId":"63497","inReplyTo":"aDCQWr3MBX4L7sbA@pks.im","subject":"Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T15:25:32Z","receivedAt":"2025-05-23T15:25:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 23, 2025 at 05:12:26PM +0200, Patrick Steinhardt wrote:\n> On Wed, May 21, 2025 at 02:08:40PM -0700, Junio C Hamano wrote:\n> > Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:\n> > Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we\n> > did not add it to the developer settings.  A more effective way to\n> > squelch them is not by sprinkling the casts like this, but to remove\n> > it from config.mak.dev ;-)\n> >\n> > https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n>\n> I'm still not of the opinion that it is garbage. We have tons of\n> locations where we mismatch integer types only because we never got a\n> warning from the compiler, and these have caused multiple stack\n> overflows in the past. The signal-to-noise ratio is high, that much is\n> certainly true. But if it helps us to avoid security issues in the\n> future I think that is acceptable.\n>\n> I do agree though this not a good project for newcomers, as fixing those\n> bugs is quite intricate overall. So we should definitely remove this\n> project from the microprojects page.\n\nAgreed with the latter point.\n\n> And in case I'm the only one who thinks that the warning has merit I'm\n> also happy to be overruled and have it be removed from our developer\n> settings.\n\nBetween the two of you, I tend to agree more with Junio's assessment\nthat the output of -Wsign-compare is generally not useful.\n\nI don't think it's *entirely* useless, and your patches squelching its\nvarious warnings has shown that to be the case. But having merged in\nthose patches to GitHub's private fork (and thus dealt with all of the\n-Wsign-compare warnings that are unique to GitHub's fork), I generally\nfound that the warnings were not legitimate issues or downright wrong.\n\nAnd indeed, merging those patches in required a fair amount of work\nprior to the merge in order to get rid of those warnings. Having done\nthat work along with Elijah (whose perspective on this I'd be curious to\nhear), I am not convinced that it made the code any better or more\nsecure.\n\nI don't agree with everything in Dan Carpenter's post above there, but\nI share their general sentiment. I don't feel so strongly about it\ncurrently as to suggest we rip it out of our config.mak.dev, but I do\nthink it would be worth at least discussing what benefits and drawbacks\nit has.\n\nIf there are a reasonable number of genuine overflow bugs or similar\nthat -Wsign-compare helped us find, then that's good. But if the vast\nmajority of the warnings are false positives, then -Wsign-compare is at\nbest an impediment to work around, and at worst may cause us to make a\nsigned-ness mistake. In that case I would feel more strongly about\nremoving it.\n\nFernando -- like Junio mentioned above, none of this is your fault\nwhatsoever ;-). Welcome indeed.\n\nThanks,\nTaylor\n"},{"id":"518780","messageId":"xmqqy0unxo7d.fsf@gitster.g","threadId":"63497","inReplyTo":"aDCQWr3MBX4L7sbA@pks.im","subject":"Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-23T16:54:14Z","receivedAt":"2025-05-23T16:54:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I'm still not of the opinion that it is garbage. We have tons of\n> locations where we mismatch integer types only because we never got a\n> warning from the compiler, and these have caused multiple stack\n> overflows in the past.\n\nI know we spotted many possible overflows and wraparound in the\npast, but -Wsign-compare being not about sizes but signedness, I'd\nconsider them more as happy accidents, rather than intended outcome.\n\nIf the code had 'a < (int)b' comparison where 'a' is 'int' and 'b'\nis 'size_t' [*], the code would still be wrong, but the compiler\nwould not have said anything.\n\n\t[*] which is what a typical \"I've suppressed the compiler\n\twarning that was annoying me\" patch would do if the original\n\twere written 'a < b'.\n\n'a -= b' can be equally bad depending on the value range of 'b', but\nit is not about -Wsign-compare and would go unreported, right?\n\nSo I think noise from -Wsign-compare are certainly not \"false\npositives\" (in the sense that the comparisons are between signed and\nunsigned---the warning option is reporting what it was asked to\nreport), but are not-false-but-useless positives; what they try to\ncatch is somehow different from what they could catch to help us.\nAnd that is why I have been skeptical.\n\n> I do agree though this not a good project for newcomers, as fixing those\n> bugs is quite intricate overall. So we should definitely remove this\n> project from the microprojects page.\n\nYeah, that is something I am quite certain about.\n"},{"id":"519209","messageId":"aDlolSPcYTSaFGV2@pks.im","threadId":"63497","inReplyTo":"xmqqy0unxo7d.fsf@gitster.g","subject":"Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-30T08:13:09Z","receivedAt":"2025-05-30T08:13:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 23, 2025 at 09:54:14AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > I do agree though this not a good project for newcomers, as fixing those\n> > bugs is quite intricate overall. So we should definitely remove this\n> > project from the microprojects page.\n> \n> Yeah, that is something I am quite certain about.\n\nI have created https://github.com/git/git.github.io/pull/779 to get rid\nof this microproject now.\n\nPatrick\n"}]}