{"thread":{"id":"62821","subject":"[PATCH] connect: address -Wsign-compare warnings","startedAt":"2025-01-17T08:22:37Z","lastAt":"2025-01-20T07:58:17Z","messageCount":6,"participants":["Mike Hommey","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"510764","messageId":"20250117074909.1430067-1-mh@glandium.org","threadId":"62821","inReplyTo":null,"subject":"[PATCH] connect: address -Wsign-compare warnings","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2025-01-17T07:49:09Z","receivedAt":"2025-01-17T08:22:37Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Most of the warnings were about loop variables being declared as ints\nwith a condition using a size_t, whereby switching the variable to\nsize_t fixes the warning.\n\nOne other case was comparing the result of strlen to an int passed\nas an argument, which turns out could just as well be passed as a\nsize_t, albeit trickling to other functions.\n\nSigned-off-by: Mike Hommey \n---\n connect.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 10fad43e98..91f3990014 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1,5 +1,4 @@\n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"config.h\"\n@@ -77,7 +76,7 @@ static NORETURN void die_initial_contact(int unexpected)\n /* Checks if the server supports the capability 'c' */\n int server_supports_v2(const char *c)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -96,7 +95,7 @@ void ensure_server_supports_v2(const char *c)\n \n int server_feature_v2(const char *c, const char **v)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -112,7 +111,7 @@ int server_feature_v2(const char *c, const char **v)\n int server_supports_feature(const char *c, const char *feature,\n \t\t\t int die_on_error)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -232,12 +231,12 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n \tstring_list_clear(&symref, 0);\n }\n \n-static void process_capabilities(struct packet_reader *reader, int *linelen)\n+static void process_capabilities(struct packet_reader *reader, size_t *linelen)\n {\n \tconst char *feat_val;\n \tsize_t feat_len;\n \tconst char *line = reader->line;\n-\tint nul_location = strlen(line);\n+\tsize_t nul_location = strlen(line);\n \tif (nul_location == *linelen)\n \t\treturn;\n \tserver_capabilities_v1 = xstrdup(line + nul_location + 1);\n@@ -271,14 +270,14 @@ static int process_dummy_ref(const struct packet_reader *reader)\n \t\t!strcmp(name, \"capabilities^{}\");\n }\n \n-static void check_no_capabilities(const char *line, int len)\n+static void check_no_capabilities(const char *line, size_t len)\n {\n \tif (strlen(line) != len)\n \t\twarning(_(\"ignoring capabilities after first line '%s'\"),\n \t\t\tline + strlen(line));\n }\n \n-static int process_ref(const struct packet_reader *reader, int len,\n+static int process_ref(const struct packet_reader *reader, size_t len,\n \t\t struct ref ***list, unsigned int flags,\n \t\t struct oid_array *extra_have)\n {\n@@ -306,7 +305,7 @@ static int process_ref(const struct packet_reader *reader, int len,\n \treturn 1;\n }\n \n-static int process_shallow(const struct packet_reader *reader, int len,\n+static int process_shallow(const struct packet_reader *reader, size_t len,\n \t\t\t struct oid_array *shallow_points)\n {\n \tconst char *line = reader->line;\n@@ -341,7 +340,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\t struct oid_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n-\tint len = 0;\n+\tsize_t len = 0;\n \tenum get_remote_heads_state state = EXPECTING_FIRST_REF;\n \n \t*list = NULL;\n@@ -394,7 +393,7 @@ static int process_ref_v2(struct packet_reader *reader, struct ref ***list,\n \t\t\t const char **unborn_head_target)\n {\n \tint ret = 1;\n-\tint i = 0;\n+\tsize_t i = 0;\n \tstruct object_id old_oid;\n \tstruct ref *ref;\n \tstruct string_list line_sections = STRING_LIST_INIT_DUP;\n@@ -552,7 +551,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t const struct string_list *server_options,\n \t\t\t int stateless_rpc)\n {\n-\tint i;\n+\tsize_t i;\n \tstruct strvec *ref_prefixes = transport_options ?\n \t\t&transport_options->ref_prefixes : NULL;\n \tconst char **unborn_head_target = transport_options ?\n-- \n2.48.1.1.g7a79e56076.dirty\n\n"},{"id":"510777","messageId":"Z4okjR8YfUGvnt1t@pks.im","threadId":"62821","inReplyTo":"20250117074909.1430067-1-mh@glandium.org","subject":"Re: [PATCH] connect: address -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-17T09:36:13Z","receivedAt":"2025-01-17T09:36:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 17, 2025 at 04:49:09PM +0900, Mike Hommey wrote:\n> diff --git a/connect.c b/connect.c\n> index 10fad43e98..91f3990014 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -77,7 +76,7 @@ static NORETURN void die_initial_contact(int unexpected)\n> /* Checks if the server supports the capability 'c' */\n> int server_supports_v2(const char *c)\n> {\n> -\tint i;\n> +\tsize_t i;\n> \n> \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n> \t\tconst char *out;\n\nI know that it's often frowned upon to change formatting while at it.\nBut in the context of these refactorings I think that it's quite helpful\nif you also moved the loop index variable declarations into the loops\nthemselves. This allows us to trivially see that it's not used anywhere\nelse.\n\n> @@ -232,12 +231,12 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n> \tstring_list_clear(&symref, 0);\n> }\n> \n> -static void process_capabilities(struct packet_reader *reader, int *linelen)\n> +static void process_capabilities(struct packet_reader *reader, size_t *linelen)\n> {\n> \tconst char *feat_val;\n> \tsize_t feat_len;\n> \tconst char *line = reader->line;\n> -\tint nul_location = strlen(line);\n> +\tsize_t nul_location = strlen(line);\n> \tif (nul_location == *linelen)\n> \t\treturn;\n> \tserver_capabilities_v1 = xstrdup(line + nul_location + 1);\n\nI think splitting out the strlen(3p)-related changes into a separate\ncommit might make sense.\n\nThanks for working on this, quite happy to see that this gets picked up\nby the community!\n\nPatrick\n"},{"id":"510817","messageId":"xmqqo705mjw7.fsf@gitster.g","threadId":"62821","inReplyTo":"20250117074909.1430067-1-mh@glandium.org","subject":"Re: [PATCH] connect: address -Wsign-compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T17:26:32Z","receivedAt":"2025-01-17T17:26:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Most of the warnings were about loop variables being declared as ints\n> with a condition using a size_t, whereby switching the variable to\n> size_t fixes the warning.\n>\n> One other case was comparing the result of strlen to an int passed\n> as an argument, which turns out could just as well be passed as a\n> size_t, albeit trickling to other functions.\n\nAs long as the blast radius is limited (like this one, which most of\nthe cascades were within the callchain of file-scope statics), and\nthe changes of type is going in the right direction (in this case, I\nsee all are using size_t for length that may come from or compared\nwith the result of strlen(), which falls into that category), such a\nchange is very much welcomed.\n\nEven if the primary objective is to squelch the -Wsign-compare and\neven if we are talking about a line in packet_reader object, which\nwould not exceed 64k bytes and using size_t is way overkill, that\nis. I personally do not think -Wsign-compare cleanliness is buying\nus all that much, compared to the amount of code churn. But this\none is well within the level that I can tolerate ;-).\n\nWill queue. Thanks.\n"},{"id":"510827","messageId":"20250117211830.75prk6e2u3qlatwt@glandium.org","threadId":"62821","inReplyTo":"xmqqo705mjw7.fsf@gitster.g","subject":"Re: [PATCH] connect: address -Wsign-compare warnings","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2025-01-17T21:18:30Z","receivedAt":"2025-01-17T21:18:40Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Most of the warnings were about loop variables being declared as ints\n> > with a condition using a size_t, whereby switching the variable to\n> > size_t fixes the warning.\n> >\n> > One other case was comparing the result of strlen to an int passed\n> > as an argument, which turns out could just as well be passed as a\n> > size_t, albeit trickling to other functions.\n> \n> As long as the blast radius is limited (like this one, which most of\n> the cascades were within the callchain of file-scope statics), and\n> the changes of type is going in the right direction (in this case, I\n> see all are using size_t for length that may come from or compared\n> with the result of strlen(), which falls into that category), such a\n> change is very much welcomed.\n> \n> Even if the primary objective is to squelch the -Wsign-compare and\n> even if we are talking about a line in packet_reader object, which\n> would not exceed 64k bytes and using size_t is way overkill, that\n> is. I personally do not think -Wsign-compare cleanliness is buying\n> us all that much, compared to the amount of code churn. But this\n> one is well within the level that I can tolerate ;-).\n> \n> Will queue. Thanks.\n\nDo you want me to address Patrick's comments?\n\nMike\n"},{"id":"510829","messageId":"xmqqldv9jfvp.fsf@gitster.g","threadId":"62821","inReplyTo":"20250117211830.75prk6e2u3qlatwt@glandium.org","subject":"Re: [PATCH] connect: address -Wsign-compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:21:30Z","receivedAt":"2025-01-17T21:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote:\n> ...\n> Do you want me to address Patrick's comments?\n\nI'll leave it up to Patrick, who is the primary developer who is\ndriving this effort from my point of view. My \"Will queue\" is\nmerely to queue it in 'seen' so that it won't be forgotten, and\nmeans nothing more than that.\n\nThanks.\n"},{"id":"510932","messageId":"Z44CDZJblkvQ_6po@pks.im","threadId":"62821","inReplyTo":"20250117211830.75prk6e2u3qlatwt@glandium.org","subject":"Re: [PATCH] connect: address -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T07:58:12Z","receivedAt":"2025-01-20T07:58:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jan 18, 2025 at 06:18:30AM +0900, Mike Hommey wrote:\n> On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote:\n> > Mike Hommey writes:\n> > \n> > > Most of the warnings were about loop variables being declared as ints\n> > > with a condition using a size_t, whereby switching the variable to\n> > > size_t fixes the warning.\n> > >\n> > > One other case was comparing the result of strlen to an int passed\n> > > as an argument, which turns out could just as well be passed as a\n> > > size_t, albeit trickling to other functions.\n> > \n> > As long as the blast radius is limited (like this one, which most of\n> > the cascades were within the callchain of file-scope statics), and\n> > the changes of type is going in the right direction (in this case, I\n> > see all are using size_t for length that may come from or compared\n> > with the result of strlen(), which falls into that category), such a\n> > change is very much welcomed.\n> > \n> > Even if the primary objective is to squelch the -Wsign-compare and\n> > even if we are talking about a line in packet_reader object, which\n> > would not exceed 64k bytes and using size_t is way overkill, that\n> > is. I personally do not think -Wsign-compare cleanliness is buying\n> > us all that much, compared to the amount of code churn. But this\n> > one is well within the level that I can tolerate ;-).\n\nIt does generate quite a bit of churn indeed. But it also made us look a\nlot closer in many places where such warnings are generated, and we\nfound multiple sites already where unexpected values can cause us to do\nweird stuff, including going out of bounds. So if this allows us to\ndetect (or even better avoid introducing) even a single out-of-bounds\nread/write that can be exploited I'm happy.\n\nI think overall it's going to be a net win in the long term as it forces\nus to think more carefullly about types, which we haven't really been\ndoing until now. And this is a frequent observation during code reviews,\nso it also gets a tiny fraction of reviewer's time back.\n\n> Do you want me to address Patrick's comments?\n\nI don't mind it too much, the end result would be the same anyway. Just\nkeep it in mind for future patch series.\n\nPatrick\n"}]}