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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2025, 18:36 UTC
Message-ID: <xmqqr02fd6pu.fsf@gitster.g>
In-Reply-To: <aa7753f2-27f5-4a7a-830d-780bd21191f7@gmail.com>

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?