threads / patch / 63104

patchpathspec: fix sign comparison warnings

Subject: [GSoC PATCH] pathspec: fix sign comparison warnings

## tl;dr

11 messages between Mar 12, 2025 and Mar 30, 2025. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Arnav Bhate· Mar 12, 2025, 20:20 UTC · lore

There are multiple places, especially in loops, where a signed and an unsigned data type are compared. Git uses a mix of signed and unsigned types to store lengths of arrays. This sometimes leads to using a signed index for an array whose length is stored in an unsigned variable or vice versa.

Replace signed data types with unsigned data types and vice versa wherever necessary. In some cases, introduce a new variable, where both signed and unsigned data types have been used to store lengths of arrays in the same function, where previously only one variable was used to iterate over both types. In cases where this is not possible, add appropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 pathspec.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
Show changes to pathspec.c +13 −13
diff --git a/pathspec.c b/pathspec.c
index 89663645e1..fd7dfdfd84 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,5 +1,4 @@
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "abspath.h"
@@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 					enum ps_skip_worktree_action sw_action)
 {
 	int num_unmatched = 0, i;
+	unsigned int j;
 
 	/*
 	 * Since we are walking the index as if we were walking the directory,
@@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	for (i = 0; i < istate->cache_nr; i++) {
-		const struct cache_entry *ce = istate->cache[i];
+	for (j = 0; j < istate->cache_nr; j++) {
+		const struct cache_entry *ce = istate->cache[j];
 		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
 		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
 			continue;
@@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
 {
 	struct index_state *istate = the_repository->index;
 	char *seen = xcalloc(pathspec->nr, 1);
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
@@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
 	if (element[1] != '(') {
 		/* Process an element in shorthand form (e.g. ":!/<match>") */
 		strbuf_addstr(sb, ":(");
-		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 			if ((magic & pathspec_magic[i].bit) &&
 			    pathspec_magic[i].mnemonic) {
 				if (sb->buf[sb->len - 1] != '(')
@@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 
 	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
 		size_t len = strcspn_escaped(pos, ",)");
-		int i;
+		unsigned int i;
 
 		if (pos[len] == ',')
 			nextat = pos + len + 1; /* handle ',' */
@@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 		if (starts_with(pos, "prefix:")) {
 			char *endptr;
 			*prefix_len = strtol(pos + 7, &endptr, 10);
-			if (endptr - pos != len)
+			if ((size_t)(endptr - pos) != len)
 				die(_("invalid parameter for pathspec magic 'prefix'"));
 			continue;
 		}
@@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 
 	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
 		char ch = *pos;
-		int i;
+		unsigned int i;
 
 		/* Special case alias for '!' */
 		if (ch == '^') {
@@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 
 void pathspec_magic_names(unsigned magic, struct strbuf *out)
 {
-	int i;
+	unsigned int i;
 	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 		const struct pathspec_magic *m = pathspec_magic + i;
 		if (!(magic & m->bit))
@@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,
 int pathspec_needs_expanded_index(struct index_state *istate,
 				  const struct pathspec *pathspec)
 {
-	unsigned int i, pos;
-	int res = 0;
+	unsigned int pos;
+	int i, res = 0;
 	char *skip_worktree_seen = NULL;
 
 	/*
@@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 			 * - not-in-cone/bar*: may need expanded index
 			 * - **.c: may need expanded index
 			 */
-			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
+			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
 			    path_in_cone_mode_sparse_checkout(item.original, istate))
 				continue;
 
@@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 				 * directory name and the sparse directory is the first
 				 * component of the pathspec, need to expand the index.
 				 */
-				if (item.nowildcard_len > ce_namelen(ce) &&
+				if ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&
 				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
 					res = 1;
 					break;
-- 
2.48.1
Karthik Nayak· Mar 13, 2025, 12:52 UTC · re: Arnav Bhate · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Arnav Bhate <bhatearnav@gmail.com> writes:
Show 47 quoted lines
> There are multiple places, especially in loops, where a signed and an
> unsigned data type are compared. Git uses a mix of signed and unsigned
> types to store lengths of arrays. This sometimes leads to using a signed
> index for an array whose length is stored in an unsigned variable or
> vice versa.
>
> Replace signed data types with unsigned data types and vice versa
> wherever necessary. In some cases, introduce a new variable, where both
> signed and unsigned data types have been used to store lengths of arrays
> in the same function, where previously only one variable was used to
> iterate over both types. In cases where this is not possible, add
> appropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.
>
> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
> ---
>  pathspec.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 89663645e1..fd7dfdfd84 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -1,5 +1,4 @@
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
>  #include "git-compat-util.h"
>  #include "abspath.h"
> @@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>  					enum ps_skip_worktree_action sw_action)
>  {
>  	int num_unmatched = 0, i;
> +	unsigned int j;
>
>  	/*
>  	 * Since we are walking the index as if we were walking the directory,
> @@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>  			num_unmatched++;
>  	if (!num_unmatched)
>  		return;
> -	for (i = 0; i < istate->cache_nr; i++) {
> -		const struct cache_entry *ce = istate->cache[i];
> +	for (j = 0; j < istate->cache_nr; j++) {
> +		const struct cache_entry *ce = istate->cache[j];
>  		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>  		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
>  			continue;

While this is correct, now we have 'i' & 'j' as iteration variables, generally this is used in O(n^2) loops to define the outer and inner loops. Here, however, we use it to simply define two different types. I find this deviation from convention a little confusing.

Perhaps, we could simply utilize the option of intializing loop variables in the loop itself?

  diff --git a/pathspec.c b/pathspec.c
  index 89663645e1..ff8854afb8 100644
  --- a/pathspec.c
  +++ b/pathspec.c
  @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const
struct pathspec *pathspec,
   					char *seen,
   					enum ps_skip_worktree_action sw_action)
   {
  -	int num_unmatched = 0, i;
  +	int num_unmatched = 0;
   	/*
   	 * Since we are walking the index as if we were walking the directory,
  @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const
struct pathspec *pathspec,
   	 * mistakenly think that the user gave a pathspec that did not match
   	 * anything.
   	 */
  -	for (i = 0; i < pathspec->nr; i++)
  +	for (int i = 0; i < pathspec->nr; i++)
   		if (!seen[i])
   			num_unmatched++;
   	if (!num_unmatched)
   		return;
  -	for (i = 0; i < istate->cache_nr; i++) {
  +	for (unsigned int i = 0; i < istate->cache_nr; i++) {
   		const struct cache_entry *ce = istate->cache[i];
   		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
   		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))

This would read much cleaner and also avoid two different loop variables. WDYT?

Also a bigger question is, shouldn't the type of `pathspec.nr` and 'istate.cache_nr' be the actual change required? Shouldn't they be set to 'size_t'?

Show 7 quoted lines
> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
>  {
>  	struct index_state *istate = the_repository->index;
>  	char *seen = xcalloc(pathspec->nr, 1);
> -	int i;
> +	unsigned int i;
>
Nit: We could also drop this and move the initialization to the line
below.
Show 11 quoted lines
>  	for (i = 0; i < istate->cache_nr; i++) {
>  		struct cache_entry *ce = istate->cache[i];
> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
>  	if (element[1] != '(') {
>  		/* Process an element in shorthand form (e.g. ":!/<match>") */
>  		strbuf_addstr(sb, ":(");
> -		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>  			if ((magic & pathspec_magic[i].bit) &&
>  			    pathspec_magic[i].mnemonic) {
>  				if (sb->buf[sb->len - 1] != '(')

Shouldn't we use 'size_t' for this, since we're iterating over the elements of an array?

Show 7 quoted lines
> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>
>  	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
>  		size_t len = strcspn_escaped(pos, ",)");
> -		int i;
> +		unsigned int i;
>
This too should be 'size_t'.
Show 11 quoted lines
>  		if (pos[len] == ',')
>  			nextat = pos + len + 1; /* handle ',' */
> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>  		if (starts_with(pos, "prefix:")) {
>  			char *endptr;
>  			*prefix_len = strtol(pos + 7, &endptr, 10);
> -			if (endptr - pos != len)
> +			if ((size_t)(endptr - pos) != len)
>  				die(_("invalid parameter for pathspec magic 'prefix'"));
>  			continue;
>  		}

This makes sense. But is it guaranteed that `endptr - pos` is greater than 0?

Show 7 quoted lines
> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>
>  	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
>  		char ch = *pos;
> -		int i;
> +		unsigned int i;
>
This too, should be 'size_t'
Show 8 quoted lines
>  		/* Special case alias for '!' */
>  		if (ch == '^') {
> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
>
>  void pathspec_magic_names(unsigned magic, struct strbuf *out)
>  {
> -	int i;
> +	unsigned int i;
This can be inlined and made 'size_t'.
Show 13 quoted lines
>  	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>  		const struct pathspec_magic *m = pathspec_magic + i;
>  		if (!(magic & m->bit))
> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,
>  int pathspec_needs_expanded_index(struct index_state *istate,
>  				  const struct pathspec *pathspec)
>  {
> -	unsigned int i, pos;
> -	int res = 0;
> +	unsigned int pos;
> +	int i, res = 0;
>  	char *skip_worktree_seen = NULL;
>

This can be inlined, but this change is done to match 'pathspec.nr''s type. This goes to my earlier question, I would say we first need to modify 'pathspec.nr' itself to be 'size_t'.

Show 10 quoted lines
>  	/*
> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>  			 * - not-in-cone/bar*: may need expanded index
>  			 * - **.c: may need expanded index
>  			 */
> -			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
> +			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
>  			    path_in_cone_mode_sparse_checkout(item.original, istate))
>  				continue;
>

Similar here, I see the types of 'item.len' and 'item.nowwildcard_len' are 'int'. Do they need to be 'size_t'?

Show 11 quoted lines
> @@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>  				 * directory name and the sparse directory is the first
>  				 * component of the pathspec, need to expand the index.
>  				 */
> -				if (item.nowildcard_len > ce_namelen(ce) &&
> +				if ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&
>  				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
>  					res = 1;
>  					break;
> --
> 2.48.1
Same question as above!
Arnav Bhate· Mar 13, 2025, 14:20 UTC · re: Karthik Nayak · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Karthik Nayak <karthik.188@gmail.com> writes:
Show 91 quoted lines
> Arnav Bhate <bhatearnav@gmail.com> writes:
> 
>> There are multiple places, especially in loops, where a signed and an
>> unsigned data type are compared. Git uses a mix of signed and unsigned
>> types to store lengths of arrays. This sometimes leads to using a signed
>> index for an array whose length is stored in an unsigned variable or
>> vice versa.
>>
>> Replace signed data types with unsigned data types and vice versa
>> wherever necessary. In some cases, introduce a new variable, where both
>> signed and unsigned data types have been used to store lengths of arrays
>> in the same function, where previously only one variable was used to
>> iterate over both types. In cases where this is not possible, add
>> appropriate cast. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.
>>
>> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
>> ---
>>  pathspec.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/pathspec.c b/pathspec.c
>> index 89663645e1..fd7dfdfd84 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -1,5 +1,4 @@
>>  #define USE_THE_REPOSITORY_VARIABLE
>> -#define DISABLE_SIGN_COMPARE_WARNINGS
>>
>>  #include "git-compat-util.h"
>>  #include "abspath.h"
>> @@ -36,6 +35,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>>  					enum ps_skip_worktree_action sw_action)
>>  {
>>  	int num_unmatched = 0, i;
>> +	unsigned int j;
>>
>>  	/*
>>  	 * Since we are walking the index as if we were walking the directory,
>> @@ -48,8 +48,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>>  			num_unmatched++;
>>  	if (!num_unmatched)
>>  		return;
>> -	for (i = 0; i < istate->cache_nr; i++) {
>> -		const struct cache_entry *ce = istate->cache[i];
>> +	for (j = 0; j < istate->cache_nr; j++) {
>> +		const struct cache_entry *ce = istate->cache[j];
>>  		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>>  		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
>>  			continue;
> 
> While this is correct, now we have 'i' & 'j' as iteration variables,
> generally this is used in O(n^2) loops to define the outer and inner
> loops. Here, however, we use it to simply define two different types. I
> find this deviation from convention a little confusing.
> 
> Perhaps, we could simply utilize the option of intializing loop
> variables in the loop itself?
> 
>   diff --git a/pathspec.c b/pathspec.c
>   index 89663645e1..ff8854afb8 100644
>   --- a/pathspec.c
>   +++ b/pathspec.c
>   @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const
> struct pathspec *pathspec,
>    					char *seen,
>    					enum ps_skip_worktree_action sw_action)
>    {
>   -	int num_unmatched = 0, i;
>   +	int num_unmatched = 0;
> 
>    	/*
>    	 * Since we are walking the index as if we were walking the directory,
>   @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const
> struct pathspec *pathspec,
>    	 * mistakenly think that the user gave a pathspec that did not match
>    	 * anything.
>    	 */
>   -	for (i = 0; i < pathspec->nr; i++)
>   +	for (int i = 0; i < pathspec->nr; i++)
>    		if (!seen[i])
>    			num_unmatched++;
>    	if (!num_unmatched)
>    		return;
>   -	for (i = 0; i < istate->cache_nr; i++) {
>   +	for (unsigned int i = 0; i < istate->cache_nr; i++) {
>    		const struct cache_entry *ce = istate->cache[i];
>    		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>    		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
> 
> This would read much cleaner and also avoid two different loop
> variables. WDYT?

We could certainly do that. My impression was that the convention was not to do so.

> Also a bigger question is, shouldn't the type of `pathspec.nr` and
> 'istate.cache_nr' be the actual change required? Shouldn't they be set
> to 'size_t'?

I tried that first and found that it required making a large number of changes spread over many files. As noted in my commit message, both signed and unsigned types are used at different places for this purpose.

Show 25 quoted lines
>> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
>>  {
>>  	struct index_state *istate = the_repository->index;
>>  	char *seen = xcalloc(pathspec->nr, 1);
>> -	int i;
>> +	unsigned int i;
>>
> 
> Nit: We could also drop this and move the initialization to the line
> below.
> 
>>  	for (i = 0; i < istate->cache_nr; i++) {
>>  		struct cache_entry *ce = istate->cache[i];
>> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
>>  	if (element[1] != '(') {
>>  		/* Process an element in shorthand form (e.g. ":!/<match>") */
>>  		strbuf_addstr(sb, ":(");
>> -		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>> +		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>  			if ((magic & pathspec_magic[i].bit) &&
>>  			    pathspec_magic[i].mnemonic) {
>>  				if (sb->buf[sb->len - 1] != '(')
> 
> Shouldn't we use 'size_t' for this, since we're iterating over the
> elements of an array?
We can use size_t there.
Show 25 quoted lines
> 
>> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>
>>  	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
>>  		size_t len = strcspn_escaped(pos, ",)");
>> -		int i;
>> +		unsigned int i;
>>
> 
> This too should be 'size_t'.
> 
>>  		if (pos[len] == ',')
>>  			nextat = pos + len + 1; /* handle ',' */
>> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>  		if (starts_with(pos, "prefix:")) {
>>  			char *endptr;
>>  			*prefix_len = strtol(pos + 7, &endptr, 10);
>> -			if (endptr - pos != len)
>> +			if ((size_t)(endptr - pos) != len)
>>  				die(_("invalid parameter for pathspec magic 'prefix'"));
>>  			continue;
>>  		}
> 
> This makes sense. But is it guaranteed that `endptr - pos` is greater
> than 0?

endptr - pos will be greater than or equal to zero, as endptr is set by strtol

Show 52 quoted lines
>> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>>
>>  	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
>>  		char ch = *pos;
>> -		int i;
>> +		unsigned int i;
>>
> 
> This too, should be 'size_t'
> 
>>  		/* Special case alias for '!' */
>>  		if (ch == '^') {
>> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
>>
>>  void pathspec_magic_names(unsigned magic, struct strbuf *out)
>>  {
>> -	int i;
>> +	unsigned int i;
> 
> This can be inlined and made 'size_t'.
> 
>>  	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>  		const struct pathspec_magic *m = pathspec_magic + i;
>>  		if (!(magic & m->bit))
>> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,
>>  int pathspec_needs_expanded_index(struct index_state *istate,
>>  				  const struct pathspec *pathspec)
>>  {
>> -	unsigned int i, pos;
>> -	int res = 0;
>> +	unsigned int pos;
>> +	int i, res = 0;
>>  	char *skip_worktree_seen = NULL;
>>
> 
> This can be inlined, but this change is done to match 'pathspec.nr''s
> type. This goes to my earlier question, I would say we first need to
> modify 'pathspec.nr' itself to be 'size_t'.
> 
>>  	/*
>> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>>  			 * - not-in-cone/bar*: may need expanded index
>>  			 * - **.c: may need expanded index
>>  			 */
>> -			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
>> +			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
>>  			    path_in_cone_mode_sparse_checkout(item.original, istate))
>>  				continue;
>>
> 
> Similar here, I see the types of 'item.len' and 'item.nowwildcard_len'
> are 'int'. Do they need to be 'size_t'?
Same as above, will require a large number of changes.
 
Show 13 quoted lines
>> @@ -860,7 +860,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>>  				 * directory name and the sparse directory is the first
>>  				 * component of the pathspec, need to expand the index.
>>  				 */
>> -				if (item.nowildcard_len > ce_namelen(ce) &&
>> +				if ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&
>>  				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
>>  					res = 1;
>>  					break;
>> --
>> 2.48.1
> 
> Same question as above!
-- 
Regards,
Arnav Bhate
(He/Him)
Karthik Nayak· Mar 13, 2025, 14:44 UTC · re: Arnav Bhate · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Arnav Bhate <bhatearnav@gmail.com> writes:
[snip]
Show 45 quoted lines
>> While this is correct, now we have 'i' & 'j' as iteration variables,
>> generally this is used in O(n^2) loops to define the outer and inner
>> loops. Here, however, we use it to simply define two different types. I
>> find this deviation from convention a little confusing.
>>
>> Perhaps, we could simply utilize the option of intializing loop
>> variables in the loop itself?
>>
>>   diff --git a/pathspec.c b/pathspec.c
>>   index 89663645e1..ff8854afb8 100644
>>   --- a/pathspec.c
>>   +++ b/pathspec.c
>>   @@ -35,7 +35,7 @@ void add_pathspec_matches_against_index(const
>> struct pathspec *pathspec,
>>    					char *seen,
>>    					enum ps_skip_worktree_action sw_action)
>>    {
>>   -	int num_unmatched = 0, i;
>>   +	int num_unmatched = 0;
>>
>>    	/*
>>    	 * Since we are walking the index as if we were walking the directory,
>>   @@ -43,12 +43,12 @@ void add_pathspec_matches_against_index(const
>> struct pathspec *pathspec,
>>    	 * mistakenly think that the user gave a pathspec that did not match
>>    	 * anything.
>>    	 */
>>   -	for (i = 0; i < pathspec->nr; i++)
>>   +	for (int i = 0; i < pathspec->nr; i++)
>>    		if (!seen[i])
>>    			num_unmatched++;
>>    	if (!num_unmatched)
>>    		return;
>>   -	for (i = 0; i < istate->cache_nr; i++) {
>>   +	for (unsigned int i = 0; i < istate->cache_nr; i++) {
>>    		const struct cache_entry *ce = istate->cache[i];
>>    		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>>    		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
>>
>> This would read much cleaner and also avoid two different loop
>> variables. WDYT?
>
> We could certainly do that. My impression was that the convention was
> not to do so.
>
This is specifically mentioned in 'Documentation/CodingGuidelines':
  since late 2021 with 44ba10d6, we have had variables declared in the
  for loop "for (int i = 0; i < 10; i++)".
So actually, the convention is to do so.
Show 9 quoted lines
>> Also a bigger question is, shouldn't the type of `pathspec.nr` and
>> 'istate.cache_nr' be the actual change required? Shouldn't they be set
>> to 'size_t'?
>
> I tried that first and found that it required making a large number of
> changes spread over many files. As noted in my commit message, both
> signed and unsigned types are used at different places for this
> purpose.
>

I can see that, but that is the correct change, no? either ways, it should be called out in the commit message why that was not the approach taken.

My personal take is that this fix is more of a bandaid, it would be better to fix the issue at source. Adding these smaller local fixes is going in the wrong direction, because we're increasing touchpoints which have to be changed when the actual fix is made.

Show 112 quoted lines
>>> @@ -78,7 +78,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
>>>  {
>>>  	struct index_state *istate = the_repository->index;
>>>  	char *seen = xcalloc(pathspec->nr, 1);
>>> -	int i;
>>> +	unsigned int i;
>>>
>>
>> Nit: We could also drop this and move the initialization to the line
>> below.
>>
>>>  	for (i = 0; i < istate->cache_nr; i++) {
>>>  		struct cache_entry *ce = istate->cache[i];
>>> @@ -130,7 +130,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
>>>  	if (element[1] != '(') {
>>>  		/* Process an element in shorthand form (e.g. ":!/<match>") */
>>>  		strbuf_addstr(sb, ":(");
>>> -		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>> +		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>>  			if ((magic & pathspec_magic[i].bit) &&
>>>  			    pathspec_magic[i].mnemonic) {
>>>  				if (sb->buf[sb->len - 1] != '(')
>>
>> Shouldn't we use 'size_t' for this, since we're iterating over the
>> elements of an array?
>
> We can use size_t there.
>
>>
>>> @@ -341,7 +341,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>>
>>>  	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
>>>  		size_t len = strcspn_escaped(pos, ",)");
>>> -		int i;
>>> +		unsigned int i;
>>>
>>
>> This too should be 'size_t'.
>>
>>>  		if (pos[len] == ',')
>>>  			nextat = pos + len + 1; /* handle ',' */
>>> @@ -354,7 +354,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
>>>  		if (starts_with(pos, "prefix:")) {
>>>  			char *endptr;
>>>  			*prefix_len = strtol(pos + 7, &endptr, 10);
>>> -			if (endptr - pos != len)
>>> +			if ((size_t)(endptr - pos) != len)
>>>  				die(_("invalid parameter for pathspec magic 'prefix'"));
>>>  			continue;
>>>  		}
>>
>> This makes sense. But is it guaranteed that `endptr - pos` is greater
>> than 0?
>
> endptr - pos will be greater than or equal to zero, as endptr is set by
> strtol
>
>>> @@ -400,7 +400,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>>>
>>>  	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
>>>  		char ch = *pos;
>>> -		int i;
>>> +		unsigned int i;
>>>
>>
>> This too, should be 'size_t'
>>
>>>  		/* Special case alias for '!' */
>>>  		if (ch == '^') {
>>> @@ -564,7 +564,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
>>>
>>>  void pathspec_magic_names(unsigned magic, struct strbuf *out)
>>>  {
>>> -	int i;
>>> +	unsigned int i;
>>
>> This can be inlined and made 'size_t'.
>>
>>>  	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>>>  		const struct pathspec_magic *m = pathspec_magic + i;
>>>  		if (!(magic & m->bit))
>>> @@ -803,8 +803,8 @@ int match_pathspec_attrs(struct index_state *istate,
>>>  int pathspec_needs_expanded_index(struct index_state *istate,
>>>  				  const struct pathspec *pathspec)
>>>  {
>>> -	unsigned int i, pos;
>>> -	int res = 0;
>>> +	unsigned int pos;
>>> +	int i, res = 0;
>>>  	char *skip_worktree_seen = NULL;
>>>
>>
>> This can be inlined, but this change is done to match 'pathspec.nr''s
>> type. This goes to my earlier question, I would say we first need to
>> modify 'pathspec.nr' itself to be 'size_t'.
>>
>>>  	/*
>>> @@ -845,7 +845,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>>>  			 * - not-in-cone/bar*: may need expanded index
>>>  			 * - **.c: may need expanded index
>>>  			 */
>>> -			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
>>> +			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
>>>  			    path_in_cone_mode_sparse_checkout(item.original, istate))
>>>  				continue;
>>>
>>
>> Similar here, I see the types of 'item.len' and 'item.nowwildcard_len'
>> are 'int'. Do they need to be 'size_t'?
>
> Same as above, will require a large number of changes.
>

Continuing from top, I would say that the goal is not to simply remove all 'DISABLE_SIGN_COMPARE_WARNINGS' definitions in the easiest way possible. Specially when we end up adding more things to be fixed eventually.

I would say perhaps picking one of these structs and fixing their types might be more suited. Let's see what others have to say here!

Junio C Hamano· Mar 13, 2025, 15:33 UTC · re: Karthik Nayak · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Karthik Nayak <karthik.188@gmail.com> writes:
> Also a bigger question is, shouldn't the type of `pathspec.nr` and
> 'istate.cache_nr' be the actual change required? Shouldn't they be set
> to 'size_t'?

Please do not blindly advocate the idea that size_t is always the right type for any countables. It is not.

Platform natural way to count things is either "unsigned int", if you are only counting, or "int", if you need to be able to signal an unusual state other than "here is now many we have in the set", like how index related functions uses (-pos-1) to signal a location in the same range with different meanings.

Karthik Nayak· Mar 13, 2025, 15:50 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Also a bigger question is, shouldn't the type of `pathspec.nr` and
>> 'istate.cache_nr' be the actual change required? Shouldn't they be set
>> to 'size_t'?
>
> Please do not blindly advocate the idea that size_t is always the
> right type for any countables.  It is not.
>
> Platform natural way to count things is either "unsigned int", if
> you are only counting, or "int", if you need to be able to signal an
> unusual state other than "here is now many we have in the set", like
> how index related functions uses (-pos-1) to signal a location in
> the same range with different meanings.

I agree there is more nuance here, and that is what I was trying to clarify too.

In this particular case, since `istate.cache_nr` is used for capturing the number of elements in `index_state.cache` and the fact that we also use ALLOC_GROW on it, would suggest that the type should be 'size_t'. But I don't know much about the usage of 'index_state' itself, so I'll take what you say.

Arnav Bhate· Mar 16, 2025, 10:14 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH] pathspec: fix sign comparison warnings

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
>> Also a bigger question is, shouldn't the type of `pathspec.nr` and
>> 'istate.cache_nr' be the actual change required? Shouldn't they be set
>> to 'size_t'?
> 
> Please do not blindly advocate the idea that size_t is always the
> right type for any countables.  It is not.  
> 
> Platform natural way to count things is either "unsigned int", if
> you are only counting, or "int", if you need to be able to signal an
> unusual state other than "here is now many we have in the set", like
> how index related functions uses (-pos-1) to signal a location in
> the same range with different meanings.

I did notice that negative states were sometimes used for such things, which why I said I didn't want to do the change to unsigned, it would be too complicated to change such things, and I do not think any alternative would be better.

-- 
Regards,
Arnav Bhate
(He/Him)
Arnav Bhate· Mar 29, 2025, 06:17 UTC · re: Arnav Bhate · lore

[GSoC PATCH v2] pathspec: fix sign comparison warnings

There are multiple places, especially in loops, where a signed and an unsigned data type are compared. Git uses a mix of signed and unsigned types to store lengths of arrays. This sometimes leads to using a signed index for an array whose length is stored in an unsigned variable or vice versa. In some cases, where both signed and unsigned data types have been used to store lengths of arrays in the same function, only one variable was used to iterate over both types.

Replace signed data types with unsigned data types and vice versa wherever necessary. Where both types of iterators are required, move the declaration inside the for loop. In cases where this is not possible, add appropriate cast.

Remove #define DISABLE_SIGN_COMPARE_WARNINGS.
Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 pathspec.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
Show changes to pathspec.c +13 −14
diff --git a/pathspec.c b/pathspec.c
index 89663645e1..c5b38278fc 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,5 +1,4 @@
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "abspath.h"
@@ -35,7 +34,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 					char *seen,
 					enum ps_skip_worktree_action sw_action)
 {
-	int num_unmatched = 0, i;
+	int num_unmatched = 0;
 
 	/*
 	 * Since we are walking the index as if we were walking the directory,
@@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 	 * mistakenly think that the user gave a pathspec that did not match
 	 * anything.
 	 */
-	for (i = 0; i < pathspec->nr; i++)
+	for (int i = 0; i < pathspec->nr; i++)
 		if (!seen[i])
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	for (i = 0; i < istate->cache_nr; i++) {
+	for (unsigned int i = 0; i < istate->cache_nr; i++) {
 		const struct cache_entry *ce = istate->cache[i];
 		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
 		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
@@ -78,7 +77,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
 {
 	struct index_state *istate = the_repository->index;
 	char *seen = xcalloc(pathspec->nr, 1);
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
@@ -130,7 +129,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
 	if (element[1] != '(') {
 		/* Process an element in shorthand form (e.g. ":!/<match>") */
 		strbuf_addstr(sb, ":(");
-		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 			if ((magic & pathspec_magic[i].bit) &&
 			    pathspec_magic[i].mnemonic) {
 				if (sb->buf[sb->len - 1] != '(')
@@ -341,7 +340,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 
 	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
 		size_t len = strcspn_escaped(pos, ",)");
-		int i;
+		unsigned int i;
 
 		if (pos[len] == ',')
 			nextat = pos + len + 1; /* handle ',' */
@@ -354,7 +353,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 		if (starts_with(pos, "prefix:")) {
 			char *endptr;
 			*prefix_len = strtol(pos + 7, &endptr, 10);
-			if (endptr - pos != len)
+			if ((size_t)(endptr - pos) != len)
 				die(_("invalid parameter for pathspec magic 'prefix'"));
 			continue;
 		}
@@ -400,7 +399,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 
 	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
 		char ch = *pos;
-		int i;
+		unsigned int i;
 
 		/* Special case alias for '!' */
 		if (ch == '^') {
@@ -564,7 +563,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 
 void pathspec_magic_names(unsigned magic, struct strbuf *out)
 {
-	int i;
+	unsigned int i;
 	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 		const struct pathspec_magic *m = pathspec_magic + i;
 		if (!(magic & m->bit))
@@ -803,8 +802,8 @@ int match_pathspec_attrs(struct index_state *istate,
 int pathspec_needs_expanded_index(struct index_state *istate,
 				  const struct pathspec *pathspec)
 {
-	unsigned int i, pos;
-	int res = 0;
+	unsigned int pos;
+	int i, res = 0;
 	char *skip_worktree_seen = NULL;
 
 	/*
@@ -845,7 +844,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 			 * - not-in-cone/bar*: may need expanded index
 			 * - **.c: may need expanded index
 			 */
-			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
+			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
 			    path_in_cone_mode_sparse_checkout(item.original, istate))
 				continue;
 
@@ -860,7 +859,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 				 * directory name and the sparse directory is the first
 				 * component of the pathspec, need to expand the index.
 				 */
-				if (item.nowildcard_len > ce_namelen(ce) &&
+				if ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&
 				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
 					res = 1;
 					break;
-- 
2.49.0
Junio C Hamano· Mar 29, 2025, 18:36 UTC · re: Arnav Bhate · lore

Re: [GSoC PATCH v2] pathspec: fix sign comparison warnings

Arnav Bhate <bhatearnav@gmail.com> writes:
Show 15 quoted lines
> @@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>  	 * mistakenly think that the user gave a pathspec that did not match
>  	 * anything.
>  	 */
> -	for (i = 0; i < pathspec->nr; i++)
> +	for (int i = 0; i < pathspec->nr; i++)
>  		if (!seen[i])
>  			num_unmatched++;
>  	if (!num_unmatched)
>  		return;
> -	for (i = 0; i < istate->cache_nr; i++) {
> +	for (unsigned int i = 0; i < istate->cache_nr; i++) {
>  		const struct cache_entry *ce = istate->cache[i];
>  		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
>  		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))

Makes me wonder if it is a nicer solution for longer term to count items in "struct pathspec" with unsigned, not signed int. A local variable that needs to hold the number of items plus an extra bit that signals an invalid state (typically denoted by a negative number) needs to be signed, but a member in a struct that records number of items in an array in the same struct has no reason to be of signed type, as the invalid state could just be represented by the .item being NULL.

But let's leave it for later; it is not something we want to leave GSoC students to handle.

Show 18 quoted lines
> @@ -845,7 +844,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>  			 * - not-in-cone/bar*: may need expanded index
>  			 * - **.c: may need expanded index
>  			 */
> -			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
> +			if (strspn(item.original + item.nowildcard_len, "*") == (unsigned int)(item.len - item.nowildcard_len) &&
>  			    path_in_cone_mode_sparse_checkout(item.original, istate))
>  				continue;
>  
> @@ -860,7 +859,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>  				 * directory name and the sparse directory is the first
>  				 * component of the pathspec, need to expand the index.
>  				 */
> -				if (item.nowildcard_len > ce_namelen(ce) &&
> +				if ((unsigned int)item.nowildcard_len > ce_namelen(ce) &&
>  				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
>  					res = 1;
>  					break;

These lines in these two hunks are way overly long already in the original, and extra casts make the even worse. Perhaps fold them in the middle appropriately?

Arnav Bhate· Mar 30, 2025, 17:45 UTC · re: Junio C Hamano · lore

[GSoC PATCH v3] pathspec: fix sign comparison warnings

There are multiple places, especially in loops, where a signed and an unsigned data type are compared. Git uses a mix of signed and unsigned types to store lengths of arrays. This sometimes leads to using a signed index for an array whose length is stored in an unsigned variable or vice versa. In some cases, where both signed and unsigned data types have been used to store lengths of arrays in the same function, only one variable was used to iterate over both types.

Replace signed data types with unsigned data types and vice versa wherever necessary. Where both types of iterators are required, move the declaration inside the for loop. In cases where this is not possible, add appropriate cast.

Remove #define DISABLE_SIGN_COMPARE_WARNINGS.
Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 pathspec.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
Show changes to pathspec.c +17 −15
diff --git a/pathspec.c b/pathspec.c
index 89663645e1..2b4e434bc0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,5 +1,4 @@
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "abspath.h"
@@ -35,7 +34,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 					char *seen,
 					enum ps_skip_worktree_action sw_action)
 {
-	int num_unmatched = 0, i;
+	int num_unmatched = 0;
 
 	/*
 	 * Since we are walking the index as if we were walking the directory,
@@ -43,12 +42,12 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 	 * mistakenly think that the user gave a pathspec that did not match
 	 * anything.
 	 */
-	for (i = 0; i < pathspec->nr; i++)
+	for (int i = 0; i < pathspec->nr; i++)
 		if (!seen[i])
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	for (i = 0; i < istate->cache_nr; i++) {
+	for (unsigned int i = 0; i < istate->cache_nr; i++) {
 		const struct cache_entry *ce = istate->cache[i];
 		if (sw_action == PS_IGNORE_SKIP_WORKTREE &&
 		    (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate)))
@@ -78,7 +77,7 @@ char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
 {
 	struct index_state *istate = the_repository->index;
 	char *seen = xcalloc(pathspec->nr, 1);
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
@@ -130,7 +129,7 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
 	if (element[1] != '(') {
 		/* Process an element in shorthand form (e.g. ":!/<match>") */
 		strbuf_addstr(sb, ":(");
-		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 			if ((magic & pathspec_magic[i].bit) &&
 			    pathspec_magic[i].mnemonic) {
 				if (sb->buf[sb->len - 1] != '(')
@@ -341,7 +340,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 
 	for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
 		size_t len = strcspn_escaped(pos, ",)");
-		int i;
+		unsigned int i;
 
 		if (pos[len] == ',')
 			nextat = pos + len + 1; /* handle ',' */
@@ -354,7 +353,7 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 		if (starts_with(pos, "prefix:")) {
 			char *endptr;
 			*prefix_len = strtol(pos + 7, &endptr, 10);
-			if (endptr - pos != len)
+			if ((size_t)(endptr - pos) != len)
 				die(_("invalid parameter for pathspec magic 'prefix'"));
 			continue;
 		}
@@ -400,7 +399,7 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 
 	for (pos = elem + 1; *pos && *pos != ':'; pos++) {
 		char ch = *pos;
-		int i;
+		unsigned int i;
 
 		/* Special case alias for '!' */
 		if (ch == '^') {
@@ -564,7 +563,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 
 void pathspec_magic_names(unsigned magic, struct strbuf *out)
 {
-	int i;
+	unsigned int i;
 	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
 		const struct pathspec_magic *m = pathspec_magic + i;
 		if (!(magic & m->bit))
@@ -803,8 +802,8 @@ int match_pathspec_attrs(struct index_state *istate,
 int pathspec_needs_expanded_index(struct index_state *istate,
 				  const struct pathspec *pathspec)
 {
-	unsigned int i, pos;
-	int res = 0;
+	unsigned int pos;
+	int i, res = 0;
 	char *skip_worktree_seen = NULL;
 
 	/*
@@ -845,7 +844,8 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 			 * - not-in-cone/bar*: may need expanded index
 			 * - **.c: may need expanded index
 			 */
-			if (strspn(item.original + item.nowildcard_len, "*") == item.len - item.nowildcard_len &&
+			if (strspn(item.original + item.nowildcard_len, "*") ==
+				    (unsigned int)(item.len - item.nowildcard_len) &&
 			    path_in_cone_mode_sparse_checkout(item.original, istate))
 				continue;
 
@@ -860,8 +860,10 @@ int pathspec_needs_expanded_index(struct index_state *istate,
 				 * directory name and the sparse directory is the first
 				 * component of the pathspec, need to expand the index.
 				 */
-				if (item.nowildcard_len > ce_namelen(ce) &&
-				    !strncmp(item.original, ce->name, ce_namelen(ce))) {
+				if ((unsigned int)item.nowildcard_len >
+					    ce_namelen(ce) &&
+				    !strncmp(item.original, ce->name,
+					     ce_namelen(ce))) {
 					res = 1;
 					break;
 				}
-- 
2.49.0

← back to recent threads