{"thread":{"id":"63103","subject":"[GSoC PATCH] rm: fix sign comparison warnings","startedAt":"2025-03-12T20:19:12Z","lastAt":"2025-03-29T06:07:06Z","messageCount":15,"participants":["Arnav Bhate","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514105","messageId":"38de63ce-6d4e-4f1f-95b1-049df78d9cfc@gmail.com","threadId":"63103","inReplyTo":null,"subject":"[GSoC PATCH] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-12T20:19:08Z","receivedAt":"2025-03-12T20:19:12Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places in loops, where a signed and an\nunsigned data type are compared. Git uses a mix of signed and unsigned\ntypes to store lengths of arrays. This sometimes leads to using a signed\nindex for an array whose length is stored in an unsigned variable or\nvice versa.\n\nget_ours_cache_pos is a special case where the initial index\nis derived from a signed variable, however, upon checking the usage of\nthe function, it is clear that it cannot be negative, and hence can be\nreplaced by an unsigned variable.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. In some cases, introduce a new variable, where both\nsigned and unsigned data types have been used to store lengths of arrays\nin the same function, where previously only one variable was used to\niterate over both types. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n builtin/rm.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..4ebfa7539d 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -5,7 +5,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"advice.h\"\n@@ -42,7 +41,12 @@ static struct {\n \n static int get_ours_cache_pos(const char *path, int pos)\n {\n-\tint i = -pos - 1;\n+\t/*\n+\t * This function is only called when pos < 0, so -pos - 1 is\n+\t * greater than or equal to 0, so it can be safely be stored in\n+\t * an unsigned int.\n+\t */\n+\tunsigned int i = -pos - 1;\n \n \twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n \t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n@@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n \t\t\t      int *errs)\n {\n \tif (files_list->nr) {\n-\t\tint i;\n+\t\tunsigned int i;\n \t\tstruct strbuf err_msg = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&err_msg, main_msg);\n@@ -271,6 +275,7 @@ int cmd_rm(int argc,\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint i, ret = 0;\n+\tunsigned int j;\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n@@ -314,8 +319,8 @@ int cmd_rm(int argc,\n \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n \t\tensure_full_index(the_repository->index);\n \n-\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n-\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n+\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n+\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n \n \t\tif (!include_sparse &&\n \t\t    (ce_skip_worktree(ce) ||\n-- \n2.48.1\n"},{"id":"514164","messageId":"xmqqy0x9s8mg.fsf@gitster.g","threadId":"63103","inReplyTo":"38de63ce-6d4e-4f1f-95b1-049df78d9cfc@gmail.com","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T07:16:39Z","receivedAt":"2025-03-13T07:16:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n>  static int get_ours_cache_pos(const char *path, int pos)\n>  {\n> -\tint i = -pos - 1;\n> +\t/*\n> +\t * This function is only called when pos < 0, so -pos - 1 is\n> +\t * greater than or equal to 0, so it can be safely be stored in\n> +\t * an unsigned int.\n> +\t */\n> +\tunsigned int i = -pos - 1;\n\n\"Can be safely stored\", sure.\n\nBut so is \"int i\" perfectly adequate to hold such a value, no?\n\nThis is one of the many instances that demonstrate why the\n\"-Wsign-compare\" warning is of dubious value, and invites worse code\nthan necessary.\n\n> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n>  \t\t\t      int *errs)\n>  {\n>  \tif (files_list->nr) {\n> -\t\tint i;\n> +\t\tunsigned int i;\n>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>  \n>  \t\tstrbuf_addstr(&err_msg, main_msg);\n> @@ -271,6 +275,7 @@ int cmd_rm(int argc,\n>  {\n>  \tstruct lock_file lock_file = LOCK_INIT;\n>  \tint i, ret = 0;\n> +\tunsigned int j;\n>  \tstruct pathspec pathspec;\n>  \tchar *seen;\n>  \n> @@ -314,8 +319,8 @@ int cmd_rm(int argc,\n>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>  \t\tensure_full_index(the_repository->index);\n>  \n> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n> -\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n> +\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n> +\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n>  \n>  \t\tif (!include_sparse &&\n>  \t\t    (ce_skip_worktree(ce) ||\n"},{"id":"514172","messageId":"CAOLa=ZTia95Lib6bkz_nWi2BYEteAaOxsrrX9DqLTEz1t02ggA@mail.gmail.com","threadId":"63103","inReplyTo":"xmqqy0x9s8mg.fsf@gitster.g","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-13T11:25:07Z","receivedAt":"2025-03-13T11:25:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Arnav Bhate <bhatearnav@gmail.com> writes:\n>\n>>  static int get_ours_cache_pos(const char *path, int pos)\n>>  {\n>> -\tint i = -pos - 1;\n>> +\t/*\n>> +\t * This function is only called when pos < 0, so -pos - 1 is\n>> +\t * greater than or equal to 0, so it can be safely be stored in\n>> +\t * an unsigned int.\n>> +\t */\n>> +\tunsigned int i = -pos - 1;\n>\n> \"Can be safely stored\", sure.\n>\n> But so is \"int i\" perfectly adequate to hold such a value, no?\n>\n> This is one of the many instances that demonstrate why the\n> \"-Wsign-compare\" warning is of dubious value, and invites worse code\n> than necessary.\n\nI have to agree. I think it would a bit cleaner to actually change the\nfunctions argument type itself. Perhaps, something like:\n\n-- >8 --\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..79e47d6e9e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -40,10 +40,8 @@ static struct {\n \t} *entry;\n } list;\n\n-static int get_ours_cache_pos(const char *path, int pos)\n+static int get_ours_cache_pos(const char *path, unsigned int i)\n {\n-\tint i = -pos - 1;\n-\n \twhile ((i < the_repository->index->cache_nr) &&\n!strcmp(the_repository->index->cache[i]->name, path)) {\n \t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n \t\t\treturn i;\n@@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n\n \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n \t\tif (pos < 0) {\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \t\t}\n@@ -131,7 +129,7 @@ static int check_local_mod(struct object_id *head,\nint index_only)\n \t\t\t * Skip unmerged entries except for populated submodules\n \t\t\t * that could lose history when removed.\n \t\t\t */\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n\n>> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n>>  \t\t\t      int *errs)\n>>  {\n>>  \tif (files_list->nr) {\n>> -\t\tint i;\n>> +\t\tunsigned int i;\n>>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>>\n>>  \t\tstrbuf_addstr(&err_msg, main_msg);\n>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,\n>>  {\n>>  \tstruct lock_file lock_file = LOCK_INIT;\n>>  \tint i, ret = 0;\n>> +\tunsigned int j;\n>>  \tstruct pathspec pathspec;\n>>  \tchar *seen;\n>>\n>> @@ -314,8 +319,8 @@ int cmd_rm(int argc,\n>>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>>  \t\tensure_full_index(the_repository->index);\n>>\n>> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n>> -\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>> +\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n>> +\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n>>\n>>  \t\tif (!include_sparse &&\n>>  \t\t    (ce_skip_worktree(ce) ||\n"},{"id":"514190","messageId":"a4d33c35-a6c3-4f42-84c7-80a7ecdcbf96@gmail.com","threadId":"63103","inReplyTo":"xmqqy0x9s8mg.fsf@gitster.g","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-13T14:26:45Z","receivedAt":"2025-03-13T14:26:48Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Arnav Bhate <bhatearnav@gmail.com> writes:\n> \n>>  static int get_ours_cache_pos(const char *path, int pos)\n>>  {\n>> -\tint i = -pos - 1;\n>> +\t/*\n>> +\t * This function is only called when pos < 0, so -pos - 1 is\n>> +\t * greater than or equal to 0, so it can be safely be stored in\n>> +\t * an unsigned int.\n>> +\t */\n>> +\tunsigned int i = -pos - 1;\n> \n> \"Can be safely stored\", sure.\n> \n> But so is \"int i\" perfectly adequate to hold such a value, no?\n> \n> This is one of the many instances that demonstrate why the\n> \"-Wsign-compare\" warning is of dubious value, and invites worse code\n> than necessary.\n\nWhile \"int i\" may be adequate, this, in my opinion, emphasises the fact\nthat we do not want i to take negative values, so I do not think this is\nworse. Karthik's suggestion in another reply is also an alternative.\n \n>> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n>>  \t\t\t      int *errs)\n>>  {\n>>  \tif (files_list->nr) {\n>> -\t\tint i;\n>> +\t\tunsigned int i;\n>>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>>  \n>>  \t\tstrbuf_addstr(&err_msg, main_msg);\n>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,\n>>  {\n>>  \tstruct lock_file lock_file = LOCK_INIT;\n>>  \tint i, ret = 0;\n>> +\tunsigned int j;\n>>  \tstruct pathspec pathspec;\n>>  \tchar *seen;\n>>  \n>> @@ -314,8 +319,8 @@ int cmd_rm(int argc,\n>>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>>  \t\tensure_full_index(the_repository->index);\n>>  \n>> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n>> -\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>> +\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n>> +\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n>>  \n>>  \t\tif (!include_sparse &&\n>>  \t\t    (ce_skip_worktree(ce) ||\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"514191","messageId":"69d10d0a-35ac-4543-a699-b3237ae4ccfd@gmail.com","threadId":"63103","inReplyTo":"CAOLa=ZTia95Lib6bkz_nWi2BYEteAaOxsrrX9DqLTEz1t02ggA@mail.gmail.com","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-13T14:30:16Z","receivedAt":"2025-03-13T14:30:20Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Arnav Bhate <bhatearnav@gmail.com> writes:\n>>\n>>>  static int get_ours_cache_pos(const char *path, int pos)\n>>>  {\n>>> -\tint i = -pos - 1;\n>>> +\t/*\n>>> +\t * This function is only called when pos < 0, so -pos - 1 is\n>>> +\t * greater than or equal to 0, so it can be safely be stored in\n>>> +\t * an unsigned int.\n>>> +\t */\n>>> +\tunsigned int i = -pos - 1;\n>>\n>> \"Can be safely stored\", sure.\n>>\n>> But so is \"int i\" perfectly adequate to hold such a value, no?\n>>\n>> This is one of the many instances that demonstrate why the\n>> \"-Wsign-compare\" warning is of dubious value, and invites worse code\n>> than necessary.\n> \n> I have to agree. I think it would a bit cleaner to actually change the\n> functions argument type itself. Perhaps, something like:\n> \n> -- >8 --\n> \n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 12ae086a55..79e47d6e9e 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -40,10 +40,8 @@ static struct {\n>  \t} *entry;\n>  } list;\n> \n> -static int get_ours_cache_pos(const char *path, int pos)\n> +static int get_ours_cache_pos(const char *path, unsigned int i)\n>  {\n> -\tint i = -pos - 1;\n> -\n>  \twhile ((i < the_repository->index->cache_nr) &&\n> !strcmp(the_repository->index->cache[i]->name, path)) {\n>  \t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n>  \t\t\treturn i;\n> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n> \n>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>  \t\tif (pos < 0) {\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n>  \t\t}\n> @@ -131,7 +129,7 @@ static int check_local_mod(struct object_id *head,\n> int index_only)\n>  \t\t\t * Skip unmerged entries except for populated submodules\n>  \t\t\t * that could lose history when removed.\n>  \t\t\t */\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n\nThis is a good option, I think, but perhaps 'i' should be renamed to\nsomething more descriptive.\n\n>>> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n>>>  \t\t\t      int *errs)\n>>>  {\n>>>  \tif (files_list->nr) {\n>>> -\t\tint i;\n>>> +\t\tunsigned int i;\n>>>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>>>\n>>>  \t\tstrbuf_addstr(&err_msg, main_msg);\n>>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,\n>>>  {\n>>>  \tstruct lock_file lock_file = LOCK_INIT;\n>>>  \tint i, ret = 0;\n>>> +\tunsigned int j;\n>>>  \tstruct pathspec pathspec;\n>>>  \tchar *seen;\n>>>\n>>> @@ -314,8 +319,8 @@ int cmd_rm(int argc,\n>>>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>>>  \t\tensure_full_index(the_repository->index);\n>>>\n>>> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n>>> -\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>>> +\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n>>> +\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n>>>\n>>>  \t\tif (!include_sparse &&\n>>>  \t\t    (ce_skip_worktree(ce) ||\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"514194","messageId":"CAOLa=ZRmYP9DV268W=UFMmYvjuLczH2d9yU3q2P4sNMH50RLKQ@mail.gmail.com","threadId":"63103","inReplyTo":"69d10d0a-35ac-4543-a699-b3237ae4ccfd@gmail.com","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-13T14:45:58Z","receivedAt":"2025-03-13T14:46:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Arnav Bhate <bhatearnav@gmail.com> writes:\n>>>\n>>>>  static int get_ours_cache_pos(const char *path, int pos)\n>>>>  {\n>>>> -\tint i = -pos - 1;\n>>>> +\t/*\n>>>> +\t * This function is only called when pos < 0, so -pos - 1 is\n>>>> +\t * greater than or equal to 0, so it can be safely be stored in\n>>>> +\t * an unsigned int.\n>>>> +\t */\n>>>> +\tunsigned int i = -pos - 1;\n>>>\n>>> \"Can be safely stored\", sure.\n>>>\n>>> But so is \"int i\" perfectly adequate to hold such a value, no?\n>>>\n>>> This is one of the many instances that demonstrate why the\n>>> \"-Wsign-compare\" warning is of dubious value, and invites worse code\n>>> than necessary.\n>>\n>> I have to agree. I think it would a bit cleaner to actually change the\n>> functions argument type itself. Perhaps, something like:\n>>\n>> -- >8 --\n>>\n>> diff --git a/builtin/rm.c b/builtin/rm.c\n>> index 12ae086a55..79e47d6e9e 100644\n>> --- a/builtin/rm.c\n>> +++ b/builtin/rm.c\n>> @@ -40,10 +40,8 @@ static struct {\n>>  \t} *entry;\n>>  } list;\n>>\n>> -static int get_ours_cache_pos(const char *path, int pos)\n>> +static int get_ours_cache_pos(const char *path, unsigned int i)\n>>  {\n>> -\tint i = -pos - 1;\n>> -\n>>  \twhile ((i < the_repository->index->cache_nr) &&\n>> !strcmp(the_repository->index->cache[i]->name, path)) {\n>>  \t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n>>  \t\t\treturn i;\n>> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n>>\n>>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>>  \t\tif (pos < 0) {\n>> -\t\t\tpos = get_ours_cache_pos(name, pos);\n>> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>>  \t\t\tif (pos < 0)\n>>  \t\t\t\tcontinue;\n>>  \t\t}\n>> @@ -131,7 +129,7 @@ static int check_local_mod(struct object_id *head,\n>> int index_only)\n>>  \t\t\t * Skip unmerged entries except for populated submodules\n>>  \t\t\t * that could lose history when removed.\n>>  \t\t\t */\n>> -\t\t\tpos = get_ours_cache_pos(name, pos);\n>> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>>  \t\t\tif (pos < 0)\n>>  \t\t\t\tcontinue;\n>\n> This is a good option, I think, but perhaps 'i' should be renamed to\n> something more descriptive.\n>\n\nOf course, that's why I said 'something like' :) This is only a guidance,\nthe final changes are left to you.\n\n>>>> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,\n>>>>  \t\t\t      int *errs)\n>>>>  {\n>>>>  \tif (files_list->nr) {\n>>>> -\t\tint i;\n>>>> +\t\tunsigned int i;\n>>>>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>>>>\n>>>>  \t\tstrbuf_addstr(&err_msg, main_msg);\n>>>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,\n>>>>  {\n>>>>  \tstruct lock_file lock_file = LOCK_INIT;\n>>>>  \tint i, ret = 0;\n>>>> +\tunsigned int j;\n>>>>  \tstruct pathspec pathspec;\n>>>>  \tchar *seen;\n>>>>\n>>>> @@ -314,8 +319,8 @@ int cmd_rm(int argc,\n>>>>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>>>>  \t\tensure_full_index(the_repository->index);\n>>>>\n>>>> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n>>>> -\t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>>>> +\tfor (j = 0; j < the_repository->index->cache_nr; j++) {\n>>>> +\t\tconst struct cache_entry *ce = the_repository->index->cache[j];\n>>>>\n>>>>  \t\tif (!include_sparse &&\n>>>>  \t\t    (ce_skip_worktree(ce) ||\n>\n> --\n> Regards,\n> Arnav Bhate\n> (He/Him)\n"},{"id":"514198","messageId":"xmqqmsdpq7ff.fsf@gitster.g","threadId":"63103","inReplyTo":"CAOLa=ZTia95Lib6bkz_nWi2BYEteAaOxsrrX9DqLTEz1t02ggA@mail.gmail.com","subject":"Re: [GSoC PATCH] rm: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T15:25:24Z","receivedAt":"2025-03-13T15:25:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> I have to agree. I think it would a bit cleaner to actually change the\n> functions argument type itself. Perhaps, something like:\n>\n> -- >8 --\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 12ae086a55..79e47d6e9e 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -40,10 +40,8 @@ static struct {\n>  \t} *entry;\n>  } list;\n>\n> -static int get_ours_cache_pos(const char *path, int pos)\n> +static int get_ours_cache_pos(const char *path, unsigned int i)\n>  {\n> -\tint i = -pos - 1;\n> -\n>  \twhile ((i < the_repository->index->cache_nr) &&\n> !strcmp(the_repository->index->cache[i]->name, path)) {\n>  \t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n>  \t\t\treturn i;\n> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n>\n>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>  \t\tif (pos < 0) {\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n>  \t\t}\n>\n> @@ -131,7 +129,7 @@ static int check_local_mod(struct object_id *head,\n> int index_only)\n>  \t\t\t * Skip unmerged entries except for populated submodules\n>  \t\t\t * that could lose history when removed.\n>  \t\t\t */\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n\nIt makes the code far easier to read to turn what index_name_pos()\nreturns to the index where the conflicted \"our\" entry ought to\nappear like the above two hunks do.  Makes perfect sense, even if\nthere weren't any type mismatch warnings.\n\nThanks.\n"},{"id":"514391","messageId":"71098ea7-9136-4ab2-8e15-27017773e054@gmail.com","threadId":"63103","inReplyTo":"38de63ce-6d4e-4f1f-95b1-049df78d9cfc@gmail.com","subject":"[GSoC PATCH v2] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-16T10:13:03Z","receivedAt":"2025-03-16T10:13:07Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places in loops, where a signed and an\nunsigned data type are compared. Git uses a mix of signed and unsigned\ntypes to store lengths of arrays. This sometimes leads to using a signed\nindex for an array whose length is stored in an unsigned variable or\nvice versa.\n\nget_ours_cache_pos is a special case where i, though derived from a\nsigned variable is never negative. Move this part to the caller side\nand make i an unsigned argument of the function. Rename i to\ninverted_pos to make it more descriptive, now that it is a function\nargument.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both signed and unsigned data types have been\nused, define a new variable in the scope of the for loop for use as the\niterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n builtin/rm.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..a5c9fc644e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -5,7 +5,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"advice.h\"\n@@ -40,14 +39,12 @@ static struct {\n \t} *entry;\n } list;\n \n-static int get_ours_cache_pos(const char *path, int pos)\n+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n {\n-\tint i = -pos - 1;\n-\n-\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n-\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n-\t\t\treturn i;\n-\t\ti++;\n+\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n+\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n+\t\t\treturn inverted_pos;\n+\t\tinverted_pos++;\n \t}\n \treturn -1;\n }\n@@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n \t\t\t      int *errs)\n {\n \tif (files_list->nr) {\n-\t\tint i;\n+\t\tunsigned int i;\n \t\tstruct strbuf err_msg = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&err_msg, main_msg);\n@@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \n \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n \t\tif (pos < 0) {\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \t\t}\n@@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t\t * Skip unmerged entries except for populated submodules\n \t\t\t * that could lose history when removed.\n \t\t\t */\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \n@@ -314,7 +311,7 @@ int cmd_rm(int argc,\n \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n \t\tensure_full_index(the_repository->index);\n \n-\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n \n \t\tif (!include_sparse &&\n-- \n2.48.1\n"},{"id":"514413","messageId":"xmqq1puvbo3s.fsf@gitster.g","threadId":"63103","inReplyTo":"71098ea7-9136-4ab2-8e15-27017773e054@gmail.com","subject":"Re: [GSoC PATCH v2] rm: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-17T16:47:51Z","receivedAt":"2025-03-17T16:47:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n> -static int get_ours_cache_pos(const char *path, int pos)\n> +static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n\nThis renaming of parameter is not right.  \n\nAt this point when the value comes to this function, it *IS* the\nposition, there is nothing inverted about it.  It points at the\nposition in the .cache[] array where an cache_entry at a higher\nstage would appear.\n\nIt is perfectly fine to state that the value that is returned from\nindex_name_pos() is potentially inverted.  The function is given a\npath name (without any stage information) and\n\n - returns a non-negative number, the position in the .cache[] array,\n   where a cache_entry at stage #0 (i.e. an entry for a path that does\n   not require conflict resolution), or\n\n - returns a negative number, when there is no such cache_entry\n   exists.  The caller can \"invert\" the value to recover a position\n   in the .cache[] array, where a cache_entry for the path at stage\n   #0 _would_ _have_ been found, if existed.  Due to the way the\n   cache entries are sorted in the .cache[] array, when you are\n   interested in finding cache entries for a path at higher stages,\n   like this function is, you can start scanning at this point until\n   you see an entry for a different path.\n\nCalling the parameter \"pos\" is the right thing to do.  The value\nused to come here _could_ have been called \"inverted\", and the\nresult of (-inverted_pos-1) can be assigned to \"pos\".  But because\nthe patch moves the inversion to the caller, what the code in the\nwhile loop sees is no longer \"inverted\".\n\n>  {\n> -\tint i = -pos - 1;\n> -\n> -\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n> -\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n> -\t\t\treturn i;\n> -\t\ti++;\n> +\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n> +\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n> +\t\t\treturn inverted_pos;\n> +\t\tinverted_pos++;\n>  \t}\n>  \treturn -1;\n>  }\n> @@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n>  \t\t\t      int *errs)\n>  {\n>  \tif (files_list->nr) {\n> -\t\tint i;\n> +\t\tunsigned int i;\n>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>  \n>  \t\tstrbuf_addstr(&err_msg, main_msg);\n> @@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n>  \n>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>  \t\tif (pos < 0) {\n\nHere is where the caller notices that index_name_pos() did not see a\nstage #0 entry.  This caller wants to see \"ours\" entry at stage #2,\nso it \"inverts\" the returned value and asks the helper function if\nit sees such an entry in the .cache[] array.\n\nA handful of prerequisite pieces of knowledge to understand this\ncode are:\n\n - The index (i.e. the .cache[] array) is sorted by full path name\n   (down from the top level of the working tree).\n\n - The index can have at most one stage #0 entry for each path name.\n   When a stage #0 entry exists for a path name, there cannot be\n   higher stage entries (the path is called \"resolved\").\n\n - The cache entries in the .cache[] array for the same path name\n   are sorted by their stage number.\n\n - There can be at most one stage #2 entry for each path name, which\n   are called \"ours\".  Entries at stage #1 are from common ancestor,\n   entries at stage #3 are from \"their\" tree.  These higher (i.e.\n   more than zero) stage entries appear only for \"conflicting\"\n   paths in the .cache[] array.\n\nWith the understanding above, you can see why \"our\" position is\ncomputed only when index_name_pos() returns negative in this hunk.\n\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n>  \t\t}\n> @@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n>  \t\t\t * Skip unmerged entries except for populated submodules\n>  \t\t\t * that could lose history when removed.\n>  \t\t\t */\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n\nThe above hunks are perfectly fine.  \n\n> @@ -314,7 +311,7 @@ int cmd_rm(int argc,\n>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>  \t\tensure_full_index(the_repository->index);\n>  \n> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n> +\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n>  \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>  \n>  \t\tif (!include_sparse &&\n\nOK.\n\nThanks.\n"},{"id":"514417","messageId":"741cda93-5dd0-4e7c-9ecf-66af84603cca@gmail.com","threadId":"63103","inReplyTo":"xmqq1puvbo3s.fsf@gitster.g","subject":"Re: [GSoC PATCH v2] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-17T17:05:43Z","receivedAt":"2025-03-17T17:05:48Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Arnav Bhate <bhatearnav@gmail.com> writes:\n> \n>> -static int get_ours_cache_pos(const char *path, int pos)\n>> +static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n> \n> This renaming of parameter is not right.  \n> \n> At this point when the value comes to this function, it *IS* the\n> position, there is nothing inverted about it.  It points at the\n> position in the .cache[] array where an cache_entry at a higher\n> stage would appear.\n> \n> It is perfectly fine to state that the value that is returned from\n> index_name_pos() is potentially inverted.  The function is given a\n> path name (without any stage information) and\n> \n>  - returns a non-negative number, the position in the .cache[] array,\n>    where a cache_entry at stage #0 (i.e. an entry for a path that does\n>    not require conflict resolution), or\n> \n>  - returns a negative number, when there is no such cache_entry\n>    exists.  The caller can \"invert\" the value to recover a position\n>    in the .cache[] array, where a cache_entry for the path at stage\n>    #0 _would_ _have_ been found, if existed.  Due to the way the\n>    cache entries are sorted in the .cache[] array, when you are\n>    interested in finding cache entries for a path at higher stages,\n>    like this function is, you can start scanning at this point until\n>    you see an entry for a different path.\n> \n> Calling the parameter \"pos\" is the right thing to do.  The value\n> used to come here _could_ have been called \"inverted\", and the\n> result of (-inverted_pos-1) can be assigned to \"pos\".  But because\n> the patch moves the inversion to the caller, what the code in the\n> while loop sees is no longer \"inverted\".\n\nMy logic was that it was the inversion of the variable pos, but your\nlogic makes more sense. I'll make the change.\n \n>>  {\n>> -\tint i = -pos - 1;\n>> -\n>> -\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n>> -\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n>> -\t\t\treturn i;\n>> -\t\ti++;\n>> +\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n>> +\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n>> +\t\t\treturn inverted_pos;\n>> +\t\tinverted_pos++;\n>>  \t}\n>>  \treturn -1;\n>>  }\n>> @@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n>>  \t\t\t      int *errs)\n>>  {\n>>  \tif (files_list->nr) {\n>> -\t\tint i;\n>> +\t\tunsigned int i;\n>>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>>  \n>>  \t\tstrbuf_addstr(&err_msg, main_msg);\n>> @@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n>>  \n>>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>>  \t\tif (pos < 0) {\n> \n> Here is where the caller notices that index_name_pos() did not see a\n> stage #0 entry.  This caller wants to see \"ours\" entry at stage #2,\n> so it \"inverts\" the returned value and asks the helper function if\n> it sees such an entry in the .cache[] array.\n> \n> A handful of prerequisite pieces of knowledge to understand this\n> code are:\n> \n>  - The index (i.e. the .cache[] array) is sorted by full path name\n>    (down from the top level of the working tree).\n> \n>  - The index can have at most one stage #0 entry for each path name.\n>    When a stage #0 entry exists for a path name, there cannot be\n>    higher stage entries (the path is called \"resolved\").\n> \n>  - The cache entries in the .cache[] array for the same path name\n>    are sorted by their stage number.\n> \n>  - There can be at most one stage #2 entry for each path name, which\n>    are called \"ours\".  Entries at stage #1 are from common ancestor,\n>    entries at stage #3 are from \"their\" tree.  These higher (i.e.\n>    more than zero) stage entries appear only for \"conflicting\"\n>    paths in the .cache[] array.\n> \n> With the understanding above, you can see why \"our\" position is\n> computed only when index_name_pos() returns negative in this hunk.\n\nThanks for the explanation, I was not able to get this from the code.\n\n> \n>> -\t\t\tpos = get_ours_cache_pos(name, pos);\n>> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>>  \t\t\tif (pos < 0)\n>>  \t\t\t\tcontinue;\n>>  \t\t}\n>> @@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n>>  \t\t\t * Skip unmerged entries except for populated submodules\n>>  \t\t\t * that could lose history when removed.\n>>  \t\t\t */\n>> -\t\t\tpos = get_ours_cache_pos(name, pos);\n>> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>>  \t\t\tif (pos < 0)\n>>  \t\t\t\tcontinue;\n> \n> The above hunks are perfectly fine.  \n> \n>> @@ -314,7 +311,7 @@ int cmd_rm(int argc,\n>>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>>  \t\tensure_full_index(the_repository->index);\n>>  \n>> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n>> +\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n>>  \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>>  \n>>  \t\tif (!include_sparse &&\n> \n> OK.\n> \n> Thanks.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"514418","messageId":"306aa7d0-3621-40ba-93c3-77536e56b73a@gmail.com","threadId":"63103","inReplyTo":"71098ea7-9136-4ab2-8e15-27017773e054@gmail.com","subject":"[GSoC PATCH v3] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-17T17:07:25Z","receivedAt":"2025-03-17T17:07:28Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places in loops, where a signed and an\nunsigned data type are compared. Git uses a mix of signed and unsigned\ntypes to store lengths of arrays. This sometimes leads to using a signed\nindex for an array whose length is stored in an unsigned variable or\nvice versa.\n\nget_ours_cache_pos is a special case where i, though derived from a\nsigned variable is never negative. Move this part to the caller side\nand make i an unsigned argument of the function. Rename i to\npos to make it descriptive, now that it is a function argument.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both signed and unsigned data types have been\nused, define a new variable in the scope of the for loop for use as the\niterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n builtin/rm.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..a5c9fc644e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -5,7 +5,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"advice.h\"\n@@ -40,14 +39,12 @@ static struct {\n \t} *entry;\n } list;\n \n-static int get_ours_cache_pos(const char *path, int pos)\n+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n {\n-\tint i = -pos - 1;\n-\n-\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n-\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n-\t\t\treturn i;\n-\t\ti++;\n+\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n+\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n+\t\t\treturn inverted_pos;\n+\t\tinverted_pos++;\n \t}\n \treturn -1;\n }\n@@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n \t\t\t      int *errs)\n {\n \tif (files_list->nr) {\n-\t\tint i;\n+\t\tunsigned int i;\n \t\tstruct strbuf err_msg = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&err_msg, main_msg);\n@@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \n \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n \t\tif (pos < 0) {\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \t\t}\n@@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t\t * Skip unmerged entries except for populated submodules\n \t\t\t * that could lose history when removed.\n \t\t\t */\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \n@@ -314,7 +311,7 @@ int cmd_rm(int argc,\n \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n \t\tensure_full_index(the_repository->index);\n \n-\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n \n \t\tif (!include_sparse &&\n-- \n2.48.1\n"},{"id":"514419","messageId":"0286b10c-2f38-43de-b727-bbebc2f05108@gmail.com","threadId":"63103","inReplyTo":"71098ea7-9136-4ab2-8e15-27017773e054@gmail.com","subject":"[GSoC PATCH v3] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-17T17:10:11Z","receivedAt":"2025-03-17T17:10:15Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places in loops, where a signed and an\nunsigned data type are compared. Git uses a mix of signed and unsigned\ntypes to store lengths of arrays. This sometimes leads to using a signed\nindex for an array whose length is stored in an unsigned variable or\nvice versa.\n\nget_ours_cache_pos is a special case where i, though derived from a\nsigned variable is never negative. Move this part to the caller side\nand make i an unsigned argument of the function. Rename i to\npos to make it descriptive, now that it is a function argument.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both signed and unsigned data types have been\nused, define a new variable in the scope of the for loop for use as the\niterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n builtin/rm.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..a5c9fc644e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -5,7 +5,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"advice.h\"\n@@ -40,14 +39,12 @@ static struct {\n \t} *entry;\n } list;\n \n-static int get_ours_cache_pos(const char *path, int pos)\n+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n {\n-\tint i = -pos - 1;\n-\n-\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n-\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n-\t\t\treturn i;\n-\t\ti++;\n+\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n+\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n+\t\t\treturn inverted_pos;\n+\t\tinverted_pos++;\n \t}\n \treturn -1;\n }\n@@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n \t\t\t      int *errs)\n {\n \tif (files_list->nr) {\n-\t\tint i;\n+\t\tunsigned int i;\n \t\tstruct strbuf err_msg = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&err_msg, main_msg);\n@@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \n \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n \t\tif (pos < 0) {\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \t\t}\n@@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t\t * Skip unmerged entries except for populated submodules\n \t\t\t * that could lose history when removed.\n \t\t\t */\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \n@@ -314,7 +311,7 @@ int cmd_rm(int argc,\n \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n \t\tensure_full_index(the_repository->index);\n \n-\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n \n \t\tif (!include_sparse &&\n-- \n2.48.1\n"},{"id":"514420","messageId":"a6cd8557-6701-4aa8-af07-8fb90bf7f846@gmail.com","threadId":"63103","inReplyTo":"306aa7d0-3621-40ba-93c3-77536e56b73a@gmail.com","subject":"Re: [GSoC PATCH v3] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-17T17:12:20Z","receivedAt":"2025-03-17T17:12:22Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n> There are multiple places in loops, where a signed and an\n> unsigned data type are compared. Git uses a mix of signed and unsigned\n> types to store lengths of arrays. This sometimes leads to using a signed\n> index for an array whose length is stored in an unsigned variable or\n> vice versa.\n> \n> get_ours_cache_pos is a special case where i, though derived from a\n> signed variable is never negative. Move this part to the caller side\n> and make i an unsigned argument of the function. Rename i to\n> pos to make it descriptive, now that it is a function argument.\n> \n> Replace signed data types with unsigned data types and vice versa\n> wherever necessary. Where both signed and unsigned data types have been\n> used, define a new variable in the scope of the for loop for use as the\n> iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n> \n> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>\n> ---\n>  builtin/rm.c | 21 +++++++++------------\n>  1 file changed, 9 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 12ae086a55..a5c9fc644e 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -5,7 +5,6 @@\n>   */\n>  \n>  #define USE_THE_REPOSITORY_VARIABLE\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n>  \n>  #include \"builtin.h\"\n>  #include \"advice.h\"\n> @@ -40,14 +39,12 @@ static struct {\n>  \t} *entry;\n>  } list;\n>  \n> -static int get_ours_cache_pos(const char *path, int pos)\n> +static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)\n>  {\n> -\tint i = -pos - 1;\n> -\n> -\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n> -\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n> -\t\t\treturn i;\n> -\t\ti++;\n> +\twhile ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {\n> +\t\tif (ce_stage(the_repository->index->cache[inverted_pos]) == 2)\n> +\t\t\treturn inverted_pos;\n> +\t\tinverted_pos++;\n>  \t}\n>  \treturn -1;\n>  }\n> @@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n>  \t\t\t      int *errs)\n>  {\n>  \tif (files_list->nr) {\n> -\t\tint i;\n> +\t\tunsigned int i;\n>  \t\tstruct strbuf err_msg = STRBUF_INIT;\n>  \n>  \t\tstrbuf_addstr(&err_msg, main_msg);\n> @@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n>  \n>  \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n>  \t\tif (pos < 0) {\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n>  \t\t}\n> @@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n>  \t\t\t * Skip unmerged entries except for populated submodules\n>  \t\t\t * that could lose history when removed.\n>  \t\t\t */\n> -\t\t\tpos = get_ours_cache_pos(name, pos);\n> +\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tcontinue;\n>  \n> @@ -314,7 +311,7 @@ int cmd_rm(int argc,\n>  \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n>  \t\tensure_full_index(the_repository->index);\n>  \n> -\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n> +\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n>  \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n>  \n>  \t\tif (!include_sparse &&\n\n\nPlease ignore this one, I sent the old patch accidentally.\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"515259","messageId":"ecaaa968-f337-466b-b719-22ebb3587167@gmail.com","threadId":"63103","inReplyTo":"0286b10c-2f38-43de-b727-bbebc2f05108@gmail.com","subject":"[GSoC PATCH v4] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-29T06:03:14Z","receivedAt":"2025-03-29T06:03:16Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places in loops, where a signed and an\nunsigned data type are compared. Git uses a mix of signed and unsigned\ntypes to store lengths of arrays. This sometimes leads to using a signed\nindex for an array whose length is stored in an unsigned variable or\nvice versa.\n\nget_ours_cache_pos is a special case where i, though derived from a\nsigned variable is never negative. Move this part to the caller side\nand make i an unsigned argument of the function. Rename i to\npos to make it descriptive, now that it is a function argument.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both signed and unsigned data types have been\nused, define a new variable in the scope of the for loop for use as the\niterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n builtin/rm.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 12ae086a55..a6565a69cf 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -5,7 +5,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"advice.h\"\n@@ -40,14 +39,12 @@ static struct {\n \t} *entry;\n } list;\n \n-static int get_ours_cache_pos(const char *path, int pos)\n+static int get_ours_cache_pos(const char *path, unsigned int pos)\n {\n-\tint i = -pos - 1;\n-\n-\twhile ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {\n-\t\tif (ce_stage(the_repository->index->cache[i]) == 2)\n-\t\t\treturn i;\n-\t\ti++;\n+\twhile ((pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[pos]->name, path)) {\n+\t\tif (ce_stage(the_repository->index->cache[pos]) == 2)\n+\t\t\treturn pos;\n+\t\tpos++;\n \t}\n \treturn -1;\n }\n@@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,\n \t\t\t      int *errs)\n {\n \tif (files_list->nr) {\n-\t\tint i;\n+\t\tunsigned int i;\n \t\tstruct strbuf err_msg = STRBUF_INIT;\n \n \t\tstrbuf_addstr(&err_msg, main_msg);\n@@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \n \t\tpos = index_name_pos(the_repository->index, name, strlen(name));\n \t\tif (pos < 0) {\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \t\t}\n@@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t\t * Skip unmerged entries except for populated submodules\n \t\t\t * that could lose history when removed.\n \t\t\t */\n-\t\t\tpos = get_ours_cache_pos(name, pos);\n+\t\t\tpos = get_ours_cache_pos(name, -pos - 1);\n \t\t\tif (pos < 0)\n \t\t\t\tcontinue;\n \n@@ -314,7 +311,7 @@ int cmd_rm(int argc,\n \tif (pathspec_needs_expanded_index(the_repository->index, &pathspec))\n \t\tensure_full_index(the_repository->index);\n \n-\tfor (i = 0; i < the_repository->index->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = the_repository->index->cache[i];\n \n \t\tif (!include_sparse &&\n-- \n2.49.0\n"},{"id":"515260","messageId":"7c55145c-7a64-46b9-8501-3662bc5d1367@gmail.com","threadId":"63103","inReplyTo":"ecaaa968-f337-466b-b719-22ebb3587167@gmail.com","subject":"Re: [GSoC PATCH v4] rm: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-29T06:07:05Z","receivedAt":"2025-03-29T06:07:06Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"I did not notice that the patch I had sent previously had a mistake.\nNow, I don't remember how it happened, but I have fixed it. Probably a\nmistake in the arguments to git format-patch. This patch should have the\ncorrect name for the variable.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"}]}