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

>> 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.
>

>>> @@ -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.
>