{"thread":{"id":"63104","subject":"[GSoC PATCH] pathspec: fix sign comparison warnings","startedAt":"2025-03-12T20:20:39Z","lastAt":"2025-03-30T17:45:11Z","messageCount":11,"participants":["Arnav Bhate","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514106","messageId":"a3aa5f99-63ce-4be5-8d64-fb6e226b3bf9@gmail.com","threadId":"63104","inReplyTo":null,"subject":"[GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-12T20:20:35Z","receivedAt":"2025-03-12T20:20:39Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places, especially 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\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. In cases where this is not possible, add\nappropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n pathspec.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 89663645e1..fd7dfdfd84 100644\n--- a/pathspec.c\n+++ b/pathspec.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 \"abspath.h\"\n@@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n {\n \tint num_unmatched = 0, i;\n+\tunsigned int j;\n \n \t/*\n \t * Since we are walking the index as if we were walking the directory,\n@@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\tfor (i = 0; i < istate->cache_nr; i++) {\n-\t\tconst struct cache_entry *ce = istate->cache[i];\n+\tfor (j = 0; j < istate->cache_nr; j++) {\n+\t\tconst struct cache_entry *ce = istate->cache[j];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n \t\t\tcontinue;\n@@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n {\n \tstruct index_state *istate = the_repository->index;\n \tchar *seen = xcalloc(pathspec->nr, 1);\n-\tint i;\n+\tunsigned int i;\n \n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n@@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n \tif (element[1] != '(') {\n \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n \t\tstrbuf_addstr(sb, \":(\");\n-\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif ((magic & pathspec_magic[i].bit) &&\n \t\t\t    pathspec_magic[i].mnemonic) {\n \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n@@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n \t\tsize_t len = strcspn_escaped(pos, \",)\");\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\tif (pos[len] == ',')\n \t\t\tnextat = pos + len + 1; /* handle ',' */\n@@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\tif (starts_with(pos, \"prefix:\")) {\n \t\t\tchar *endptr;\n \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n-\t\t\tif (endptr - pos != len)\n+\t\t\tif ((size_t)(endptr - pos) != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n@@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n \n \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n \t\tchar ch = *pos;\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\t/* Special case alias for '!' */\n \t\tif (ch == '^') {\n@@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n \n void pathspec_magic_names(unsigned magic, struct strbuf *out)\n {\n-\tint i;\n+\tunsigned int i;\n \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n \t\tif (!(magic & m->bit))\n@@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,\n int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t  const struct pathspec *pathspec)\n {\n-\tunsigned int i, pos;\n-\tint res = 0;\n+\tunsigned int pos;\n+\tint i, res = 0;\n \tchar *skip_worktree_seen = NULL;\n \n \t/*\n@@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t * - not-in-cone/bar*: may need expanded index\n \t\t\t * - **.c: may need expanded index\n \t\t\t */\n-\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n+\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n \t\t\t\tcontinue;\n \n@@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t * directory name and the sparse directory is the first\n \t\t\t\t * component of the pathspec, need to expand the index.\n \t\t\t\t */\n-\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n+\t\t\t\tif ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&\n \t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n \t\t\t\t\tres = 1;\n \t\t\t\t\tbreak;\n-- \n2.48.1\n"},{"id":"514177","messageId":"CAOLa=ZRNJD7NqceGQ4B8ox+50m2q1nU1t29x7O0roc=-_4ww0w@mail.gmail.com","threadId":"63104","inReplyTo":"a3aa5f99-63ce-4be5-8d64-fb6e226b3bf9@gmail.com","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-13T12:52:58Z","receivedAt":"2025-03-13T12:53:00Z","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> There are multiple places, especially 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> Replace signed data types with unsigned data types and vice versa\n> wherever necessary. In some cases, introduce a new variable, where both\n> signed and unsigned data types have been used to store lengths of arrays\n> in the same function, where previously only one variable was used to\n> iterate over both types. In cases where this is not possible, add\n> appropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n>\n> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>\n> ---\n>  pathspec.c | 26 +++++++++++++-------------\n>  1 file changed, 13 insertions(+), 13 deletions(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 89663645e1..fd7dfdfd84 100644\n> --- a/pathspec.c\n> +++ b/pathspec.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 \"abspath.h\"\n> @@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n>  \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n>  {\n>  \tint num_unmatched = 0, i;\n> +\tunsigned int j;\n>\n>  \t/*\n>  \t * Since we are walking the index as if we were walking the directory,\n> @@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n>  \t\t\tnum_unmatched++;\n>  \tif (!num_unmatched)\n>  \t\treturn;\n> -\tfor (i = 0; i < istate->cache_nr; i++) {\n> -\t\tconst struct cache_entry *ce = istate->cache[i];\n> +\tfor (j = 0; j < istate->cache_nr; j++) {\n> +\t\tconst struct cache_entry *ce = istate->cache[j];\n>  \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n>  \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n>  \t\t\tcontinue;\n\nWhile this is correct, now we have 'i' & 'j' as iteration variables,\ngenerally this is used in O(n^2) loops to define the outer and inner\nloops. Here, however, we use it to simply define two different types. I\nfind this deviation from convention a little confusing.\n\nPerhaps, we could simply utilize the option of intializing loop\nvariables in the loop itself?\n\n  diff --git a/pathspec.c b/pathspec.c\n  index 89663645e1..ff8854afb8 100644\n  --- a/pathspec.c\n  +++ b/pathspec.c\n  @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const\nstruct pathspec *pathspec,\n   \t\t\t\t\tchar *seen,\n   \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n   {\n  -\tint num_unmatched = 0, i;\n  +\tint num_unmatched = 0;\n\n   \t/*\n   \t * Since we are walking the index as if we were walking the directory,\n  @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const\nstruct pathspec *pathspec,\n   \t * mistakenly think that the user gave a pathspec that did not match\n   \t * anything.\n   \t */\n  -\tfor (i = 0; i < pathspec->nr; i++)\n  +\tfor (int i = 0; i < pathspec->nr; i++)\n   \t\tif (!seen[i])\n   \t\t\tnum_unmatched++;\n   \tif (!num_unmatched)\n   \t\treturn;\n  -\tfor (i = 0; i < istate->cache_nr; i++) {\n  +\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n   \t\tconst struct cache_entry *ce = istate->cache[i];\n   \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n   \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n\nThis would read much cleaner and also avoid two different loop\nvariables. WDYT?\n\nAlso a bigger question is, shouldn't the type of `pathspec.nr` and\n'istate.cache_nr' be the actual change required? Shouldn't they be set\nto 'size_t'?\n\n> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n>  {\n>  \tstruct index_state *istate = the_repository->index;\n>  \tchar *seen = xcalloc(pathspec->nr, 1);\n> -\tint i;\n> +\tunsigned int i;\n>\n\nNit: We could also drop this and move the initialization to the line\nbelow.\n\n>  \tfor (i = 0; i < istate->cache_nr; i++) {\n>  \t\tstruct cache_entry *ce = istate->cache[i];\n> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n>  \tif (element[1] != '(') {\n>  \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n>  \t\tstrbuf_addstr(sb, \":(\");\n> -\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n> +\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>  \t\t\tif ((magic & pathspec_magic[i].bit) &&\n>  \t\t\t    pathspec_magic[i].mnemonic) {\n>  \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n\nShouldn't we use 'size_t' for this, since we're iterating over the\nelements of an array?\n\n> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>\n>  \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n>  \t\tsize_t len = strcspn_escaped(pos, \",)\");\n> -\t\tint i;\n> +\t\tunsigned int i;\n>\n\nThis too should be 'size_t'.\n\n>  \t\tif (pos[len] == ',')\n>  \t\t\tnextat = pos + len + 1; /* handle ',' */\n> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>  \t\tif (starts_with(pos, \"prefix:\")) {\n>  \t\t\tchar *endptr;\n>  \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n> -\t\t\tif (endptr - pos != len)\n> +\t\t\tif ((size_t)(endptr - pos) != len)\n>  \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n>  \t\t\tcontinue;\n>  \t\t}\n\nThis makes sense. But is it guaranteed that `endptr - pos` is greater\nthan 0?\n\n> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n>\n>  \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n>  \t\tchar ch = *pos;\n> -\t\tint i;\n> +\t\tunsigned int i;\n>\n\nThis too, should be 'size_t'\n\n>  \t\t/* Special case alias for '!' */\n>  \t\tif (ch == '^') {\n> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n>\n>  void pathspec_magic_names(unsigned magic, struct strbuf *out)\n>  {\n> -\tint i;\n> +\tunsigned int i;\n\nThis can be inlined and made 'size_t'.\n\n>  \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>  \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n>  \t\tif (!(magic & m->bit))\n> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,\n>  int pathspec_needs_expanded_index(struct index_state *istate,\n>  \t\t\t\t  const struct pathspec *pathspec)\n>  {\n> -\tunsigned int i, pos;\n> -\tint res = 0;\n> +\tunsigned int pos;\n> +\tint i, res = 0;\n>  \tchar *skip_worktree_seen = NULL;\n>\n\nThis can be inlined, but this change is done to match 'pathspec.nr''s\ntype. This goes to my earlier question, I would say we first need to\nmodify 'pathspec.nr' itself to be 'size_t'.\n\n>  \t/*\n> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>  \t\t\t * - not-in-cone/bar*: may need expanded index\n>  \t\t\t * - **.c: may need expanded index\n>  \t\t\t */\n> -\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n> +\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n>  \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n>  \t\t\t\tcontinue;\n>\n\nSimilar here, I see the types of 'item.len' and 'item.nowwildcard_len'\nare 'int'. Do they need to be 'size_t'?\n\n> @@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>  \t\t\t\t * directory name and the sparse directory is the first\n>  \t\t\t\t * component of the pathspec, need to expand the index.\n>  \t\t\t\t */\n> -\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n> +\t\t\t\tif ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&\n>  \t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n>  \t\t\t\t\tres = 1;\n>  \t\t\t\t\tbreak;\n> --\n> 2.48.1\n\nSame question as above!\n"},{"id":"514187","messageId":"0946673c-72ce-4848-b7f6-edd7a1cc7ff4@gmail.com","threadId":"63104","inReplyTo":"CAOLa=ZRNJD7NqceGQ4B8ox+50m2q1nU1t29x7O0roc=-_4ww0w@mail.gmail.com","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-13T14:20:52Z","receivedAt":"2025-03-13T14:20:56Z","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> Arnav Bhate <bhatearnav@gmail.com> writes:\n> \n>> There are multiple places, especially 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>> Replace signed data types with unsigned data types and vice versa\n>> wherever necessary. In some cases, introduce a new variable, where both\n>> signed and unsigned data types have been used to store lengths of arrays\n>> in the same function, where previously only one variable was used to\n>> iterate over both types. In cases where this is not possible, add\n>> appropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.\n>>\n>> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>\n>> ---\n>>  pathspec.c | 26 +++++++++++++-------------\n>>  1 file changed, 13 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/pathspec.c b/pathspec.c\n>> index 89663645e1..fd7dfdfd84 100644\n>> --- a/pathspec.c\n>> +++ b/pathspec.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 \"abspath.h\"\n>> @@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n>>  \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n>>  {\n>>  \tint num_unmatched = 0, i;\n>> +\tunsigned int j;\n>>\n>>  \t/*\n>>  \t * Since we are walking the index as if we were walking the directory,\n>> @@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n>>  \t\t\tnum_unmatched++;\n>>  \tif (!num_unmatched)\n>>  \t\treturn;\n>> -\tfor (i = 0; i < istate->cache_nr; i++) {\n>> -\t\tconst struct cache_entry *ce = istate->cache[i];\n>> +\tfor (j = 0; j < istate->cache_nr; j++) {\n>> +\t\tconst struct cache_entry *ce = istate->cache[j];\n>>  \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n>>  \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n>>  \t\t\tcontinue;\n> \n> While this is correct, now we have 'i' & 'j' as iteration variables,\n> generally this is used in O(n^2) loops to define the outer and inner\n> loops. Here, however, we use it to simply define two different types. I\n> find this deviation from convention a little confusing.\n> \n> Perhaps, we could simply utilize the option of intializing loop\n> variables in the loop itself?\n> \n>   diff --git a/pathspec.c b/pathspec.c\n>   index 89663645e1..ff8854afb8 100644\n>   --- a/pathspec.c\n>   +++ b/pathspec.c\n>   @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const\n> struct pathspec *pathspec,\n>    \t\t\t\t\tchar *seen,\n>    \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n>    {\n>   -\tint num_unmatched = 0, i;\n>   +\tint num_unmatched = 0;\n> \n>    \t/*\n>    \t * Since we are walking the index as if we were walking the directory,\n>   @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const\n> struct pathspec *pathspec,\n>    \t * mistakenly think that the user gave a pathspec that did not match\n>    \t * anything.\n>    \t */\n>   -\tfor (i = 0; i < pathspec->nr; i++)\n>   +\tfor (int i = 0; i < pathspec->nr; i++)\n>    \t\tif (!seen[i])\n>    \t\t\tnum_unmatched++;\n>    \tif (!num_unmatched)\n>    \t\treturn;\n>   -\tfor (i = 0; i < istate->cache_nr; i++) {\n>   +\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n>    \t\tconst struct cache_entry *ce = istate->cache[i];\n>    \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n>    \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n> \n> This would read much cleaner and also avoid two different loop\n> variables. WDYT?\n\nWe could certainly do that. My impression was that the convention was\nnot to do so.\n\n> Also a bigger question is, shouldn't the type of `pathspec.nr` and\n> 'istate.cache_nr' be the actual change required? Shouldn't they be set\n> to 'size_t'?\n\nI tried that first and found that it required making a large number of\nchanges spread over many files. As noted in my commit message, both\nsigned and unsigned types are used at different places for this\npurpose.\n\n>> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n>>  {\n>>  \tstruct index_state *istate = the_repository->index;\n>>  \tchar *seen = xcalloc(pathspec->nr, 1);\n>> -\tint i;\n>> +\tunsigned int i;\n>>\n> \n> Nit: We could also drop this and move the initialization to the line\n> below.\n> \n>>  \tfor (i = 0; i < istate->cache_nr; i++) {\n>>  \t\tstruct cache_entry *ce = istate->cache[i];\n>> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n>>  \tif (element[1] != '(') {\n>>  \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n>>  \t\tstrbuf_addstr(sb, \":(\");\n>> -\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>> +\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>>  \t\t\tif ((magic & pathspec_magic[i].bit) &&\n>>  \t\t\t    pathspec_magic[i].mnemonic) {\n>>  \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n> \n> Shouldn't we use 'size_t' for this, since we're iterating over the\n> elements of an array?\n\nWe can use size_t there.\n\n> \n>> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>>\n>>  \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n>>  \t\tsize_t len = strcspn_escaped(pos, \",)\");\n>> -\t\tint i;\n>> +\t\tunsigned int i;\n>>\n> \n> This too should be 'size_t'.\n> \n>>  \t\tif (pos[len] == ',')\n>>  \t\t\tnextat = pos + len + 1; /* handle ',' */\n>> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>>  \t\tif (starts_with(pos, \"prefix:\")) {\n>>  \t\t\tchar *endptr;\n>>  \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n>> -\t\t\tif (endptr - pos != len)\n>> +\t\t\tif ((size_t)(endptr - pos) != len)\n>>  \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n>>  \t\t\tcontinue;\n>>  \t\t}\n> \n> This makes sense. But is it guaranteed that `endptr - pos` is greater\n> than 0?\n\nendptr - pos will be greater than or equal to zero, as endptr is set by\nstrtol\n\n>> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n>>\n>>  \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n>>  \t\tchar ch = *pos;\n>> -\t\tint i;\n>> +\t\tunsigned int i;\n>>\n> \n> This too, should be 'size_t'\n> \n>>  \t\t/* Special case alias for '!' */\n>>  \t\tif (ch == '^') {\n>> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n>>\n>>  void pathspec_magic_names(unsigned magic, struct strbuf *out)\n>>  {\n>> -\tint i;\n>> +\tunsigned int i;\n> \n> This can be inlined and made 'size_t'.\n> \n>>  \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>>  \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n>>  \t\tif (!(magic & m->bit))\n>> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,\n>>  int pathspec_needs_expanded_index(struct index_state *istate,\n>>  \t\t\t\t  const struct pathspec *pathspec)\n>>  {\n>> -\tunsigned int i, pos;\n>> -\tint res = 0;\n>> +\tunsigned int pos;\n>> +\tint i, res = 0;\n>>  \tchar *skip_worktree_seen = NULL;\n>>\n> \n> This can be inlined, but this change is done to match 'pathspec.nr''s\n> type. This goes to my earlier question, I would say we first need to\n> modify 'pathspec.nr' itself to be 'size_t'.\n> \n>>  \t/*\n>> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>>  \t\t\t * - not-in-cone/bar*: may need expanded index\n>>  \t\t\t * - **.c: may need expanded index\n>>  \t\t\t */\n>> -\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n>> +\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n>>  \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n>>  \t\t\t\tcontinue;\n>>\n> \n> Similar here, I see the types of 'item.len' and 'item.nowwildcard_len'\n> are 'int'. Do they need to be 'size_t'?\n\nSame as above, will require a large number of changes.\n \n>> @@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>>  \t\t\t\t * directory name and the sparse directory is the first\n>>  \t\t\t\t * component of the pathspec, need to expand the index.\n>>  \t\t\t\t */\n>> -\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n>> +\t\t\t\tif ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&\n>>  \t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n>>  \t\t\t\t\tres = 1;\n>>  \t\t\t\t\tbreak;\n>> --\n>> 2.48.1\n> \n> Same question as above!\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"514192","messageId":"CAOLa=ZTRA_7UpUamtc2NeJH75uEw16z73qWFqY8veV=mToeUhg@mail.gmail.com","threadId":"63104","inReplyTo":"0946673c-72ce-4848-b7f6-edd7a1cc7ff4@gmail.com","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-13T14:44:43Z","receivedAt":"2025-03-13T14:44:45Z","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[snip]\n\n>> While this is correct, now we have 'i' & 'j' as iteration variables,\n>> generally this is used in O(n^2) loops to define the outer and inner\n>> loops. Here, however, we use it to simply define two different types. I\n>> find this deviation from convention a little confusing.\n>>\n>> Perhaps, we could simply utilize the option of intializing loop\n>> variables in the loop itself?\n>>\n>>   diff --git a/pathspec.c b/pathspec.c\n>>   index 89663645e1..ff8854afb8 100644\n>>   --- a/pathspec.c\n>>   +++ b/pathspec.c\n>>   @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const\n>> struct pathspec *pathspec,\n>>    \t\t\t\t\tchar *seen,\n>>    \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n>>    {\n>>   -\tint num_unmatched = 0, i;\n>>   +\tint num_unmatched = 0;\n>>\n>>    \t/*\n>>    \t * Since we are walking the index as if we were walking the directory,\n>>   @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const\n>> struct pathspec *pathspec,\n>>    \t * mistakenly think that the user gave a pathspec that did not match\n>>    \t * anything.\n>>    \t */\n>>   -\tfor (i = 0; i < pathspec->nr; i++)\n>>   +\tfor (int i = 0; i < pathspec->nr; i++)\n>>    \t\tif (!seen[i])\n>>    \t\t\tnum_unmatched++;\n>>    \tif (!num_unmatched)\n>>    \t\treturn;\n>>   -\tfor (i = 0; i < istate->cache_nr; i++) {\n>>   +\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n>>    \t\tconst struct cache_entry *ce = istate->cache[i];\n>>    \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n>>    \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n>>\n>> This would read much cleaner and also avoid two different loop\n>> variables. WDYT?\n>\n> We could certainly do that. My impression was that the convention was\n> not to do so.\n>\n\nThis is specifically mentioned in 'Documentation/CodingGuidelines':\n\n  since late 2021 with 44ba10d6, we have had variables declared in the\n  for loop \"for (int i = 0; i < 10; i++)\".\n\nSo actually, the convention is to do so.\n\n>> Also a bigger question is, shouldn't the type of `pathspec.nr` and\n>> 'istate.cache_nr' be the actual change required? Shouldn't they be set\n>> to 'size_t'?\n>\n> I tried that first and found that it required making a large number of\n> changes spread over many files. As noted in my commit message, both\n> signed and unsigned types are used at different places for this\n> purpose.\n>\n\nI can see that, but that is the correct change, no? either ways, it\nshould be called out in the commit message why that was not the approach\ntaken.\n\nMy personal take is that this fix is more of a bandaid, it would be\nbetter to fix the issue at source. Adding these smaller local fixes is\ngoing in the wrong direction, because we're increasing touchpoints which\nhave to be changed when the actual fix is made.\n\n>>> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n>>>  {\n>>>  \tstruct index_state *istate = the_repository->index;\n>>>  \tchar *seen = xcalloc(pathspec->nr, 1);\n>>> -\tint i;\n>>> +\tunsigned int i;\n>>>\n>>\n>> Nit: We could also drop this and move the initialization to the line\n>> below.\n>>\n>>>  \tfor (i = 0; i < istate->cache_nr; i++) {\n>>>  \t\tstruct cache_entry *ce = istate->cache[i];\n>>> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n>>>  \tif (element[1] != '(') {\n>>>  \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n>>>  \t\tstrbuf_addstr(sb, \":(\");\n>>> -\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>>> +\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>>>  \t\t\tif ((magic & pathspec_magic[i].bit) &&\n>>>  \t\t\t    pathspec_magic[i].mnemonic) {\n>>>  \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n>>\n>> Shouldn't we use 'size_t' for this, since we're iterating over the\n>> elements of an array?\n>\n> We can use size_t there.\n>\n>>\n>>> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>>>\n>>>  \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n>>>  \t\tsize_t len = strcspn_escaped(pos, \",)\");\n>>> -\t\tint i;\n>>> +\t\tunsigned int i;\n>>>\n>>\n>> This too should be 'size_t'.\n>>\n>>>  \t\tif (pos[len] == ',')\n>>>  \t\t\tnextat = pos + len + 1; /* handle ',' */\n>>> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n>>>  \t\tif (starts_with(pos, \"prefix:\")) {\n>>>  \t\t\tchar *endptr;\n>>>  \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n>>> -\t\t\tif (endptr - pos != len)\n>>> +\t\t\tif ((size_t)(endptr - pos) != len)\n>>>  \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n>>>  \t\t\tcontinue;\n>>>  \t\t}\n>>\n>> This makes sense. But is it guaranteed that `endptr - pos` is greater\n>> than 0?\n>\n> endptr - pos will be greater than or equal to zero, as endptr is set by\n> strtol\n>\n>>> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n>>>\n>>>  \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n>>>  \t\tchar ch = *pos;\n>>> -\t\tint i;\n>>> +\t\tunsigned int i;\n>>>\n>>\n>> This too, should be 'size_t'\n>>\n>>>  \t\t/* Special case alias for '!' */\n>>>  \t\tif (ch == '^') {\n>>> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n>>>\n>>>  void pathspec_magic_names(unsigned magic, struct strbuf *out)\n>>>  {\n>>> -\tint i;\n>>> +\tunsigned int i;\n>>\n>> This can be inlined and made 'size_t'.\n>>\n>>>  \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>>>  \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n>>>  \t\tif (!(magic & m->bit))\n>>> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,\n>>>  int pathspec_needs_expanded_index(struct index_state *istate,\n>>>  \t\t\t\t  const struct pathspec *pathspec)\n>>>  {\n>>> -\tunsigned int i, pos;\n>>> -\tint res = 0;\n>>> +\tunsigned int pos;\n>>> +\tint i, res = 0;\n>>>  \tchar *skip_worktree_seen = NULL;\n>>>\n>>\n>> This can be inlined, but this change is done to match 'pathspec.nr''s\n>> type. This goes to my earlier question, I would say we first need to\n>> modify 'pathspec.nr' itself to be 'size_t'.\n>>\n>>>  \t/*\n>>> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>>>  \t\t\t * - not-in-cone/bar*: may need expanded index\n>>>  \t\t\t * - **.c: may need expanded index\n>>>  \t\t\t */\n>>> -\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n>>> +\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n>>>  \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n>>>  \t\t\t\tcontinue;\n>>>\n>>\n>> Similar here, I see the types of 'item.len' and 'item.nowwildcard_len'\n>> are 'int'. Do they need to be 'size_t'?\n>\n> Same as above, will require a large number of changes.\n>\n\nContinuing from top, I would say that the goal is not to simply remove\nall 'DISABLE_SIGN_COMPARE_WARNINGS' definitions in the easiest way\npossible. Specially when we end up adding more things to be fixed\neventually.\n\nI would say perhaps picking one of these structs and fixing their types\nmight be more suited. Let's see what others have to say here!\n"},{"id":"514200","messageId":"xmqqcyelq714.fsf@gitster.g","threadId":"63104","inReplyTo":"CAOLa=ZRNJD7NqceGQ4B8ox+50m2q1nU1t29x7O0roc=-_4ww0w@mail.gmail.com","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T15:33:59Z","receivedAt":"2025-03-13T15:34:02Z","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> Also a bigger question is, shouldn't the type of `pathspec.nr` and\n> 'istate.cache_nr' be the actual change required? Shouldn't they be set\n> to 'size_t'?\n\nPlease do not blindly advocate the idea that size_t is always the\nright type for any countables.  It is not.  \n\nPlatform natural way to count things is either \"unsigned int\", if\nyou are only counting, or \"int\", if you need to be able to signal an\nunusual state other than \"here is now many we have in the set\", like\nhow index related functions uses (-pos-1) to signal a location in\nthe same range with different meanings.\n"},{"id":"514202","messageId":"CAOLa=ZQWWh2952HDazcdYkDxBAeuCS+KWQk66zVGZyb+foE1WA@mail.gmail.com","threadId":"63104","inReplyTo":"xmqqcyelq714.fsf@gitster.g","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-13T15:50:39Z","receivedAt":"2025-03-13T15:50:41Z","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> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Also a bigger question is, shouldn't the type of `pathspec.nr` and\n>> 'istate.cache_nr' be the actual change required? Shouldn't they be set\n>> to 'size_t'?\n>\n> Please do not blindly advocate the idea that size_t is always the\n> right type for any countables.  It is not.\n>\n> Platform natural way to count things is either \"unsigned int\", if\n> you are only counting, or \"int\", if you need to be able to signal an\n> unusual state other than \"here is now many we have in the set\", like\n> how index related functions uses (-pos-1) to signal a location in\n> the same range with different meanings.\n\nI agree there is more nuance here, and that is what I was trying to\nclarify too.\n\nIn this particular case, since `istate.cache_nr` is used for capturing\nthe number of elements in `index_state.cache` and the fact that we also\nuse ALLOC_GROW on it, would suggest that the type should be 'size_t'.\nBut I don't know much about the usage of 'index_state' itself, so I'll\ntake what you say.\n"},{"id":"514392","messageId":"a720058a-f8cd-422c-a8c3-af7536bfc880@gmail.com","threadId":"63104","inReplyTo":"xmqqcyelq714.fsf@gitster.g","subject":"Re: [GSoC PATCH] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-16T10:14:54Z","receivedAt":"2025-03-16T10:14:58Z","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> Karthik Nayak <karthik.188@gmail.com> writes:\n> \n>> Also a bigger question is, shouldn't the type of `pathspec.nr` and\n>> 'istate.cache_nr' be the actual change required? Shouldn't they be set\n>> to 'size_t'?\n> \n> Please do not blindly advocate the idea that size_t is always the\n> right type for any countables.  It is not.  \n> \n> Platform natural way to count things is either \"unsigned int\", if\n> you are only counting, or \"int\", if you need to be able to signal an\n> unusual state other than \"here is now many we have in the set\", like\n> how index related functions uses (-pos-1) to signal a location in\n> the same range with different meanings.\n\nI did notice that negative states were sometimes used for such things,\nwhich why I said I didn't want to do the change to unsigned, it would\nbe too complicated to change such things, and I do not think any\nalternative would be better.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"515261","messageId":"aa7753f2-27f5-4a7a-830d-780bd21191f7@gmail.com","threadId":"63104","inReplyTo":"a3aa5f99-63ce-4be5-8d64-fb6e226b3bf9@gmail.com","subject":"[GSoC PATCH v2] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-29T06:17:36Z","receivedAt":"2025-03-29T06:17:38Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places, especially 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. In some cases, where both signed and unsigned data types\nhave been used to store lengths of arrays in the same function, only\none variable was used to iterate over both types.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both types of iterators are required, move\nthe declaration inside the for loop. In cases where this is not\npossible, add appropriate cast.\n\nRemove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n pathspec.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 89663645e1..c5b38278fc 100644\n--- a/pathspec.c\n+++ b/pathspec.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 \"abspath.h\"\n@@ -35,7 +34,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\t\t\tchar *seen,\n \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n {\n-\tint num_unmatched = 0, i;\n+\tint num_unmatched = 0;\n \n \t/*\n \t * Since we are walking the index as if we were walking the directory,\n@@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t * mistakenly think that the user gave a pathspec that did not match\n \t * anything.\n \t */\n-\tfor (i = 0; i < pathspec->nr; i++)\n+\tfor (int i = 0; i < pathspec->nr; i++)\n \t\tif (!seen[i])\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\tfor (i = 0; i < istate->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n@@ -78,7 +77,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n {\n \tstruct index_state *istate = the_repository->index;\n \tchar *seen = xcalloc(pathspec->nr, 1);\n-\tint i;\n+\tunsigned int i;\n \n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n@@ -130,7 +129,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n \tif (element[1] != '(') {\n \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n \t\tstrbuf_addstr(sb, \":(\");\n-\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif ((magic & pathspec_magic[i].bit) &&\n \t\t\t    pathspec_magic[i].mnemonic) {\n \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n@@ -341,7 +340,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n \t\tsize_t len = strcspn_escaped(pos, \",)\");\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\tif (pos[len] == ',')\n \t\t\tnextat = pos + len + 1; /* handle ',' */\n@@ -354,7 +353,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\tif (starts_with(pos, \"prefix:\")) {\n \t\t\tchar *endptr;\n \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n-\t\t\tif (endptr - pos != len)\n+\t\t\tif ((size_t)(endptr - pos) != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n@@ -400,7 +399,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n \n \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n \t\tchar ch = *pos;\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\t/* Special case alias for '!' */\n \t\tif (ch == '^') {\n@@ -564,7 +563,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n \n void pathspec_magic_names(unsigned magic, struct strbuf *out)\n {\n-\tint i;\n+\tunsigned int i;\n \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n \t\tif (!(magic & m->bit))\n@@ -803,8 +802,8 @@ int match_pathspec_attrs(struct index_state *istate,\n int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t  const struct pathspec *pathspec)\n {\n-\tunsigned int i, pos;\n-\tint res = 0;\n+\tunsigned int pos;\n+\tint i, res = 0;\n \tchar *skip_worktree_seen = NULL;\n \n \t/*\n@@ -845,7 +844,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t * - not-in-cone/bar*: may need expanded index\n \t\t\t * - **.c: may need expanded index\n \t\t\t */\n-\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n+\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n \t\t\t\tcontinue;\n \n@@ -860,7 +859,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t * directory name and the sparse directory is the first\n \t\t\t\t * component of the pathspec, need to expand the index.\n \t\t\t\t */\n-\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n+\t\t\t\tif ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&\n \t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n \t\t\t\t\tres = 1;\n \t\t\t\t\tbreak;\n-- \n2.49.0\n"},{"id":"515262","messageId":"e093e548-0955-4ced-8770-01cd6eb29967@gmail.com","threadId":"63104","inReplyTo":"aa7753f2-27f5-4a7a-830d-780bd21191f7@gmail.com","subject":"Re: [GSoC PATCH v2] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-29T06:18:43Z","receivedAt":"2025-03-29T06:18:45Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"This patch incorporates Karthik's suggestions.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"515282","messageId":"xmqqr02fd6pu.fsf@gitster.g","threadId":"63104","inReplyTo":"aa7753f2-27f5-4a7a-830d-780bd21191f7@gmail.com","subject":"Re: [GSoC PATCH v2] pathspec: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-29T18:36:45Z","receivedAt":"2025-03-29T18:36:48Z","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> @@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n>  \t * mistakenly think that the user gave a pathspec that did not match\n>  \t * anything.\n>  \t */\n> -\tfor (i = 0; i < pathspec->nr; i++)\n> +\tfor (int i = 0; i < pathspec->nr; i++)\n>  \t\tif (!seen[i])\n>  \t\t\tnum_unmatched++;\n>  \tif (!num_unmatched)\n>  \t\treturn;\n> -\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n>  \t\tconst struct cache_entry *ce = istate->cache[i];\n>  \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n>  \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n\nMakes me wonder if it is a nicer solution for longer term to count\nitems in \"struct pathspec\" with unsigned, not signed int.  A local\nvariable that needs to hold the number of items plus an extra bit\nthat signals an invalid state (typically denoted by a negative\nnumber) needs to be signed, but a member in a struct that records\nnumber of items in an array in the same struct has no reason to be\nof signed type, as the invalid state could just be represented by\nthe .item being NULL.\n\nBut let's leave it for later; it is not something we want to leave\nGSoC students to handle.\n\n> @@ -845,7 +844,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>  \t\t\t * - not-in-cone/bar*: may need expanded index\n>  \t\t\t * - **.c: may need expanded index\n>  \t\t\t */\n> -\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n> +\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == (unsigned int)(item.len - item.nowildcard_len) &&\n>  \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n>  \t\t\t\tcontinue;\n>  \n> @@ -860,7 +859,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n>  \t\t\t\t * directory name and the sparse directory is the first\n>  \t\t\t\t * component of the pathspec, need to expand the index.\n>  \t\t\t\t */\n> -\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n> +\t\t\t\tif ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&\n>  \t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n>  \t\t\t\t\tres = 1;\n>  \t\t\t\t\tbreak;\n\nThese lines in these two hunks are way overly long already in the\noriginal, and extra casts make the even worse.  Perhaps fold them\nin the middle appropriately?\n"},{"id":"515308","messageId":"127c3010-4b23-436e-a07d-e6031e746113@gmail.com","threadId":"63104","inReplyTo":"xmqqr02fd6pu.fsf@gitster.g","subject":"[GSoC PATCH v3] pathspec: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-30T17:45:06Z","receivedAt":"2025-03-30T17:45:11Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple places, especially 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. In some cases, where both signed and unsigned data types\nhave been used to store lengths of arrays in the same function, only\none variable was used to iterate over both types.\n\nReplace signed data types with unsigned data types and vice versa\nwherever necessary. Where both types of iterators are required, move\nthe declaration inside the for loop. In cases where this is not\npossible, add appropriate cast.\n\nRemove #define DISABLE_SIGN_COMPARE_WARNINGS.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n pathspec.c | 32 +++++++++++++++++---------------\n 1 file changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 89663645e1..2b4e434bc0 100644\n--- a/pathspec.c\n+++ b/pathspec.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 \"abspath.h\"\n@@ -35,7 +34,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\t\t\tchar *seen,\n \t\t\t\t\tenum ps_skip_worktree_action sw_action)\n {\n-\tint num_unmatched = 0, i;\n+\tint num_unmatched = 0;\n \n \t/*\n \t * Since we are walking the index as if we were walking the directory,\n@@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t * mistakenly think that the user gave a pathspec that did not match\n \t * anything.\n \t */\n-\tfor (i = 0; i < pathspec->nr; i++)\n+\tfor (int i = 0; i < pathspec->nr; i++)\n \t\tif (!seen[i])\n \t\t\tnum_unmatched++;\n \tif (!num_unmatched)\n \t\treturn;\n-\tfor (i = 0; i < istate->cache_nr; i++) {\n+\tfor (unsigned int i = 0; i < istate->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (sw_action == PS_IGNORE_SKIP_WORKTREE &&\n \t\t    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))\n@@ -78,7 +77,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)\n {\n \tstruct index_state *istate = the_repository->index;\n \tchar *seen = xcalloc(pathspec->nr, 1);\n-\tint i;\n+\tunsigned int i;\n \n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n@@ -130,7 +129,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,\n \tif (element[1] != '(') {\n \t\t/* Process an element in shorthand form (e.g. \":!/<match>\") */\n \t\tstrbuf_addstr(sb, \":(\");\n-\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\t\tif ((magic & pathspec_magic[i].bit) &&\n \t\t\t    pathspec_magic[i].mnemonic) {\n \t\t\t\tif (sb->buf[sb->len - 1] != '(')\n@@ -341,7 +340,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \n \tfor (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {\n \t\tsize_t len = strcspn_escaped(pos, \",)\");\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\tif (pos[len] == ',')\n \t\t\tnextat = pos + len + 1; /* handle ',' */\n@@ -354,7 +353,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,\n \t\tif (starts_with(pos, \"prefix:\")) {\n \t\t\tchar *endptr;\n \t\t\t*prefix_len = strtol(pos + 7, &endptr, 10);\n-\t\t\tif (endptr - pos != len)\n+\t\t\tif ((size_t)(endptr - pos) != len)\n \t\t\t\tdie(_(\"invalid parameter for pathspec magic 'prefix'\"));\n \t\t\tcontinue;\n \t\t}\n@@ -400,7 +399,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)\n \n \tfor (pos = elem + 1; *pos && *pos != ':'; pos++) {\n \t\tchar ch = *pos;\n-\t\tint i;\n+\t\tunsigned int i;\n \n \t\t/* Special case alias for '!' */\n \t\tif (ch == '^') {\n@@ -564,7 +563,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)\n \n void pathspec_magic_names(unsigned magic, struct strbuf *out)\n {\n-\tint i;\n+\tunsigned int i;\n \tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n \t\tconst struct pathspec_magic *m = pathspec_magic + i;\n \t\tif (!(magic & m->bit))\n@@ -803,8 +802,8 @@ int match_pathspec_attrs(struct index_state *istate,\n int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t  const struct pathspec *pathspec)\n {\n-\tunsigned int i, pos;\n-\tint res = 0;\n+\tunsigned int pos;\n+\tint i, res = 0;\n \tchar *skip_worktree_seen = NULL;\n \n \t/*\n@@ -845,7 +844,8 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t * - not-in-cone/bar*: may need expanded index\n \t\t\t * - **.c: may need expanded index\n \t\t\t */\n-\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") == item.len - item.nowildcard_len &&\n+\t\t\tif (strspn(item.original + item.nowildcard_len, \"*\") ==\n+\t\t\t\t    (unsigned int)(item.len - item.nowildcard_len) &&\n \t\t\t    path_in_cone_mode_sparse_checkout(item.original, istate))\n \t\t\t\tcontinue;\n \n@@ -860,8 +860,10 @@ int pathspec_needs_expanded_index(struct index_state *istate,\n \t\t\t\t * directory name and the sparse directory is the first\n \t\t\t\t * component of the pathspec, need to expand the index.\n \t\t\t\t */\n-\t\t\t\tif (item.nowildcard_len > ce_namelen(ce) &&\n-\t\t\t\t    !strncmp(item.original, ce->name, ce_namelen(ce))) {\n+\t\t\t\tif ((unsigned int)item.nowildcard_len >\n+\t\t\t\t\t    ce_namelen(ce) &&\n+\t\t\t\t    !strncmp(item.original, ce->name,\n+\t\t\t\t\t     ce_namelen(ce))) {\n \t\t\t\t\tres = 1;\n \t\t\t\t\tbreak;\n \t\t\t\t}\n-- \n2.49.0\n"}]}