git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Karthik Nayak <karthik.188@gmail.com>
Date
Mar 13, 2025, 12:52 UTC
Message-ID
<CAOLa=ZRNJD7NqceGQ4B8ox+50m2q1nU1t29x7O0roc=-_4ww0w@mail.gmail.com>
In-Reply-To
<a3aa5f99-63ce-4be5-8d64-fb6e226b3bf9@gmail.com>
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!
Previous: Arnav BhateNext: Arnav Bhate
Message 2 of 11 in “pathspec: fix sign comparison warnings”
  1. pathspec: fix sign comparison warningsArnav Bhate, Mar 12, 2025
  2. Karthik NayakMar 13, 2025
  3. Arnav BhateMar 13, 2025
  4. Karthik NayakMar 13, 2025
  5. Junio C HamanoMar 13, 2025
  6. Karthik NayakMar 13, 2025
  7. Arnav BhateMar 16, 2025
  8. pathspec: fix sign comparison warningsArnav Bhate, Mar 29, 2025
  9. Arnav BhateMar 29, 2025
  10. Junio C HamanoMar 29, 2025
  11. pathspec: fix sign comparison warningsArnav Bhate, Mar 30, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.