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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2025, 16:47 UTC
Message-ID: <xmqq1puvbo3s.fsf@gitster.g>
In-Reply-To: <71098ea7-9136-4ab2-8e15-27017773e054@gmail.com>

Arnav Bhate <bhatearnav@gmail.com> writes:

> -static int get_ours_cache_pos(const char *path, int pos)
> +static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)

This renaming of parameter is not right.

At this point when the value comes to this function, it *IS* the position, there is nothing inverted about it. It points at the position in the .cache[] array where an cache_entry at a higher stage would appear.

It is perfectly fine to state that the value that is returned from index_name_pos() is potentially inverted. The function is given a path name (without any stage information) and

 - returns a non-negative number, the position in the .cache[] array,
   where a cache_entry at stage #0 (i.e. an entry for a path that does
   not require conflict resolution), or

 - returns a negative number, when there is no such cache_entry
   exists.  The caller can "invert" the value to recover a position
   in the .cache[] array, where a cache_entry for the path at stage
   #0 _would_ _have_ been found, if existed.  Due to the way the
   cache entries are sorted in the .cache[] array, when you are
   interested in finding cache entries for a path at higher stages,
   like this function is, you can start scanning at this point until
   you see an entry for a different path.

Calling the parameter "pos" is the right thing to do. The value used to come here _could_ have been called "inverted", and the result of (-inverted_pos-1) can be assigned to "pos". But because the patch moves the inversion to the caller, what the code in the while loop sees is no longer "inverted".

Show 27 quoted lines

>  {
> -	int i = -pos - 1;
> -
> -	while ((i < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[i]->name, path)) {
> -		if (ce_stage(the_repository->index->cache[i]) == 2)
> -			return i;
> -		i++;
> +	while ((inverted_pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[inverted_pos]->name, path)) {
> +		if (ce_stage(the_repository->index->cache[inverted_pos]) == 2)
> +			return inverted_pos;
> +		inverted_pos++;
>  	}
>  	return -1;
>  }
> @@ -58,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
>  			      int *errs)
>  {
>  	if (files_list->nr) {
> -		int i;
> +		unsigned int i;
>  		struct strbuf err_msg = STRBUF_INIT;
>  
>  		strbuf_addstr(&err_msg, main_msg);
> @@ -83,7 +80,7 @@ static void submodules_absorb_gitdir_if_needed(void)
>  
>  		pos = index_name_pos(the_repository->index, name, strlen(name));
>  		if (pos < 0) {

Here is where the caller notices that index_name_pos() did not see a stage #0 entry. This caller wants to see "ours" entry at stage #2, so it "inverts" the returned value and asks the helper function if it sees such an entry in the .cache[] array.

A handful of prerequisite pieces of knowledge to understand this code are:

 - The index (i.e. the .cache[] array) is sorted by full path name
   (down from the top level of the working tree).

 - The index can have at most one stage #0 entry for each path name.
   When a stage #0 entry exists for a path name, there cannot be
   higher stage entries (the path is called "resolved").

 - The cache entries in the .cache[] array for the same path name
   are sorted by their stage number.

 - There can be at most one stage #2 entry for each path name, which
   are called "ours".  Entries at stage #1 are from common ancestor,
   entries at stage #3 are from "their" tree.  These higher (i.e.
   more than zero) stage entries appear only for "conflicting"
   paths in the .cache[] array.

With the understanding above, you can see why "our" position is computed only when index_name_pos() returns negative in this hunk.

Show 13 quoted lines

> -			pos = get_ours_cache_pos(name, pos);
> +			pos = get_ours_cache_pos(name, -pos - 1);
>  			if (pos < 0)
>  				continue;
>  		}
> @@ -131,7 +128,7 @@ static int check_local_mod(struct object_id *head, int index_only)
>  			 * Skip unmerged entries except for populated submodules
>  			 * that could lose history when removed.
>  			 */
> -			pos = get_ours_cache_pos(name, pos);
> +			pos = get_ours_cache_pos(name, -pos - 1);
>  			if (pos < 0)
>  				continue;

The above hunks are perfectly fine.

Show 9 quoted lines

> @@ -314,7 +311,7 @@ int cmd_rm(int argc,
>  	if (pathspec_needs_expanded_index(the_repository->index, &pathspec))
>  		ensure_full_index(the_repository->index);
>  
> -	for (i = 0; i < the_repository->index->cache_nr; i++) {
> +	for (unsigned int i = 0; i < the_repository->index->cache_nr; i++) {
>  		const struct cache_entry *ce = the_repository->index->cache[i];
>  
>  		if (!include_sparse &&

OK.

Thanks.