[GSoC PATCH] rm: fix sign comparison warnings

[GSoC PATCH] rm: fix sign comparison warnings

[Arnav Bhate]

There are multiple places 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.

get_ours_cache_pos is a special case where the initial index
is derived from a signed variable, however, upon checking the usage of
the function, it is clear that it cannot be negative, and hence can be
replaced by an unsigned variable.

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. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 builtin/rm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..4ebfa7539d 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -5,7 +5,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "advice.h"
@@ -42,7 +41,12 @@ static struct {
 
 static int get_ours_cache_pos(const char *path, int pos)
 {
-	int i = -pos - 1;
+	/*
+	 * This function is only called when pos < 0, so -pos - 1 is
+	 * greater than or equal to 0, so it can be safely be stored in
+	 * an unsigned int.
+	 */
+	unsigned 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)
@@ -58,7 +62,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);
@@ -271,6 +275,7 @@ int cmd_rm(int argc,
 {
 	struct lock_file lock_file = LOCK_INIT;
 	int i, ret = 0;
+	unsigned int j;
 	struct pathspec pathspec;
 	char *seen;
 
@@ -314,8 +319,8 @@ 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++) {
-		const struct cache_entry *ce = the_repository->index->cache[i];
+	for (j = 0; j < the_repository->index->cache_nr; j++) {
+		const struct cache_entry *ce = the_repository->index->cache[j];
 
 		if (!include_sparse &&
 		    (ce_skip_worktree(ce) ||
-- 
2.48.1

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

[Junio C Hamano]

Arnav Bhate <bhatearnav@gmail.com> writes:

>  static int get_ours_cache_pos(const char *path, int pos)
>  {
> -	int i = -pos - 1;
> +	/*
> +	 * This function is only called when pos < 0, so -pos - 1 is
> +	 * greater than or equal to 0, so it can be safely be stored in
> +	 * an unsigned int.
> +	 */
> +	unsigned int i = -pos - 1;

"Can be safely stored", sure.

But so is "int i" perfectly adequate to hold such a value, no?

This is one of the many instances that demonstrate why the
"-Wsign-compare" warning is of dubious value, and invites worse code
than necessary.

> @@ -58,7 +62,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);
> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>  {
>  	struct lock_file lock_file = LOCK_INIT;
>  	int i, ret = 0;
> +	unsigned int j;
>  	struct pathspec pathspec;
>  	char *seen;
>  
> @@ -314,8 +319,8 @@ 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++) {
> -		const struct cache_entry *ce = the_repository->index->cache[i];
> +	for (j = 0; j < the_repository->index->cache_nr; j++) {
> +		const struct cache_entry *ce = the_repository->index->cache[j];
>  
>  		if (!include_sparse &&
>  		    (ce_skip_worktree(ce) ||

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Arnav Bhate <bhatearnav@gmail.com> writes:
>
>>  static int get_ours_cache_pos(const char *path, int pos)
>>  {
>> -	int i = -pos - 1;
>> +	/*
>> +	 * This function is only called when pos < 0, so -pos - 1 is
>> +	 * greater than or equal to 0, so it can be safely be stored in
>> +	 * an unsigned int.
>> +	 */
>> +	unsigned int i = -pos - 1;
>
> "Can be safely stored", sure.
>
> But so is "int i" perfectly adequate to hold such a value, no?
>
> This is one of the many instances that demonstrate why the
> "-Wsign-compare" warning is of dubious value, and invites worse code
> than necessary.

I have to agree. I think it would a bit cleaner to actually change the
functions argument type itself. Perhaps, something like:

-- >8 --

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..79e47d6e9e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -40,10 +40,8 @@ static struct {
 	} *entry;
 } list;

-static int get_ours_cache_pos(const char *path, int pos)
+static int get_ours_cache_pos(const char *path, unsigned int i)
 {
-	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;
@@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)

 		pos = index_name_pos(the_repository->index, name, strlen(name));
 		if (pos < 0) {
-			pos = get_ours_cache_pos(name, pos);
+			pos = get_ours_cache_pos(name, -pos - 1);
 			if (pos < 0)
 				continue;
 		}
@@ -131,7 +129,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;

>> @@ -58,7 +62,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);
>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>>  {
>>  	struct lock_file lock_file = LOCK_INIT;
>>  	int i, ret = 0;
>> +	unsigned int j;
>>  	struct pathspec pathspec;
>>  	char *seen;
>>
>> @@ -314,8 +319,8 @@ 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++) {
>> -		const struct cache_entry *ce = the_repository->index->cache[i];
>> +	for (j = 0; j < the_repository->index->cache_nr; j++) {
>> +		const struct cache_entry *ce = the_repository->index->cache[j];
>>
>>  		if (!include_sparse &&
>>  		    (ce_skip_worktree(ce) ||

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Arnav Bhate]

Junio C Hamano <gitster@pobox.com> writes:
> Arnav Bhate <bhatearnav@gmail.com> writes:
> 
>>  static int get_ours_cache_pos(const char *path, int pos)
>>  {
>> -	int i = -pos - 1;
>> +	/*
>> +	 * This function is only called when pos < 0, so -pos - 1 is
>> +	 * greater than or equal to 0, so it can be safely be stored in
>> +	 * an unsigned int.
>> +	 */
>> +	unsigned int i = -pos - 1;
> 
> "Can be safely stored", sure.
> 
> But so is "int i" perfectly adequate to hold such a value, no?
> 
> This is one of the many instances that demonstrate why the
> "-Wsign-compare" warning is of dubious value, and invites worse code
> than necessary.

While "int i" may be adequate, this, in my opinion, emphasises the fact
that we do not want i to take negative values, so I do not think this is
worse. Karthik's suggestion in another reply is also an alternative.
 
>> @@ -58,7 +62,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);
>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>>  {
>>  	struct lock_file lock_file = LOCK_INIT;
>>  	int i, ret = 0;
>> +	unsigned int j;
>>  	struct pathspec pathspec;
>>  	char *seen;
>>  
>> @@ -314,8 +319,8 @@ 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++) {
>> -		const struct cache_entry *ce = the_repository->index->cache[i];
>> +	for (j = 0; j < the_repository->index->cache_nr; j++) {
>> +		const struct cache_entry *ce = the_repository->index->cache[j];
>>  
>>  		if (!include_sparse &&
>>  		    (ce_skip_worktree(ce) ||

-- 
Regards,
Arnav Bhate
(He/Him)

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

[Arnav Bhate]

Karthik Nayak <karthik.188@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Arnav Bhate <bhatearnav@gmail.com> writes:
>>
>>>  static int get_ours_cache_pos(const char *path, int pos)
>>>  {
>>> -	int i = -pos - 1;
>>> +	/*
>>> +	 * This function is only called when pos < 0, so -pos - 1 is
>>> +	 * greater than or equal to 0, so it can be safely be stored in
>>> +	 * an unsigned int.
>>> +	 */
>>> +	unsigned int i = -pos - 1;
>>
>> "Can be safely stored", sure.
>>
>> But so is "int i" perfectly adequate to hold such a value, no?
>>
>> This is one of the many instances that demonstrate why the
>> "-Wsign-compare" warning is of dubious value, and invites worse code
>> than necessary.
> 
> I have to agree. I think it would a bit cleaner to actually change the
> functions argument type itself. Perhaps, something like:
> 
> -- >8 --
> 
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 12ae086a55..79e47d6e9e 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -40,10 +40,8 @@ static struct {
>  	} *entry;
>  } list;
> 
> -static int get_ours_cache_pos(const char *path, int pos)
> +static int get_ours_cache_pos(const char *path, unsigned int i)
>  {
> -	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;
> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)
> 
>  		pos = index_name_pos(the_repository->index, name, strlen(name));
>  		if (pos < 0) {
> -			pos = get_ours_cache_pos(name, pos);
> +			pos = get_ours_cache_pos(name, -pos - 1);
>  			if (pos < 0)
>  				continue;
>  		}
> @@ -131,7 +129,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;

This is a good option, I think, but perhaps 'i' should be renamed to
something more descriptive.

>>> @@ -58,7 +62,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);
>>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>>>  {
>>>  	struct lock_file lock_file = LOCK_INIT;
>>>  	int i, ret = 0;
>>> +	unsigned int j;
>>>  	struct pathspec pathspec;
>>>  	char *seen;
>>>
>>> @@ -314,8 +319,8 @@ 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++) {
>>> -		const struct cache_entry *ce = the_repository->index->cache[i];
>>> +	for (j = 0; j < the_repository->index->cache_nr; j++) {
>>> +		const struct cache_entry *ce = the_repository->index->cache[j];
>>>
>>>  		if (!include_sparse &&
>>>  		    (ce_skip_worktree(ce) ||

-- 
Regards,
Arnav Bhate
(He/Him)

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]

Arnav Bhate <bhatearnav@gmail.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Arnav Bhate <bhatearnav@gmail.com> writes:
>>>
>>>>  static int get_ours_cache_pos(const char *path, int pos)
>>>>  {
>>>> -	int i = -pos - 1;
>>>> +	/*
>>>> +	 * This function is only called when pos < 0, so -pos - 1 is
>>>> +	 * greater than or equal to 0, so it can be safely be stored in
>>>> +	 * an unsigned int.
>>>> +	 */
>>>> +	unsigned int i = -pos - 1;
>>>
>>> "Can be safely stored", sure.
>>>
>>> But so is "int i" perfectly adequate to hold such a value, no?
>>>
>>> This is one of the many instances that demonstrate why the
>>> "-Wsign-compare" warning is of dubious value, and invites worse code
>>> than necessary.
>>
>> I have to agree. I think it would a bit cleaner to actually change the
>> functions argument type itself. Perhaps, something like:
>>
>> -- >8 --
>>
>> diff --git a/builtin/rm.c b/builtin/rm.c
>> index 12ae086a55..79e47d6e9e 100644
>> --- a/builtin/rm.c
>> +++ b/builtin/rm.c
>> @@ -40,10 +40,8 @@ static struct {
>>  	} *entry;
>>  } list;
>>
>> -static int get_ours_cache_pos(const char *path, int pos)
>> +static int get_ours_cache_pos(const char *path, unsigned int i)
>>  {
>> -	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;
>> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)
>>
>>  		pos = index_name_pos(the_repository->index, name, strlen(name));
>>  		if (pos < 0) {
>> -			pos = get_ours_cache_pos(name, pos);
>> +			pos = get_ours_cache_pos(name, -pos - 1);
>>  			if (pos < 0)
>>  				continue;
>>  		}
>> @@ -131,7 +129,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;
>
> This is a good option, I think, but perhaps 'i' should be renamed to
> something more descriptive.
>

Of course, that's why I said 'something like' :) This is only a guidance,
the final changes are left to you.

>>>> @@ -58,7 +62,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);
>>>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>>>>  {
>>>>  	struct lock_file lock_file = LOCK_INIT;
>>>>  	int i, ret = 0;
>>>> +	unsigned int j;
>>>>  	struct pathspec pathspec;
>>>>  	char *seen;
>>>>
>>>> @@ -314,8 +319,8 @@ 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++) {
>>>> -		const struct cache_entry *ce = the_repository->index->cache[i];
>>>> +	for (j = 0; j < the_repository->index->cache_nr; j++) {
>>>> +		const struct cache_entry *ce = the_repository->index->cache[j];
>>>>
>>>>  		if (!include_sparse &&
>>>>  		    (ce_skip_worktree(ce) ||
>
> --
> Regards,
> Arnav Bhate
> (He/Him)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> I have to agree. I think it would a bit cleaner to actually change the
> functions argument type itself. Perhaps, something like:
>
> -- >8 --
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 12ae086a55..79e47d6e9e 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -40,10 +40,8 @@ static struct {
>  	} *entry;
>  } list;
>
> -static int get_ours_cache_pos(const char *path, int pos)
> +static int get_ours_cache_pos(const char *path, unsigned int i)
>  {
> -	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;
> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)
>
>  		pos = index_name_pos(the_repository->index, name, strlen(name));
>  		if (pos < 0) {
> -			pos = get_ours_cache_pos(name, pos);
> +			pos = get_ours_cache_pos(name, -pos - 1);
>  			if (pos < 0)
>  				continue;
>  		}
>
> @@ -131,7 +129,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;

It makes the code far easier to read to turn what index_name_pos()
returns to the index where the conflicted "our" entry ought to
appear like the above two hunks do.  Makes perfect sense, even if
there weren't any type mismatch warnings.

Thanks.

[GSoC PATCH v2] rm: fix sign comparison warnings

[Arnav Bhate]

There are multiple places 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.

get_ours_cache_pos is a special case where i, though derived from a
signed variable is never negative. Move this part to the caller side
and make i an unsigned argument of the function. Rename i to
inverted_pos to make it more descriptive, now that it is a function
argument.

Replace signed data types with unsigned data types and vice versa
wherever necessary. Where both signed and unsigned data types have been
used, define a new variable in the scope of the for loop for use as the
iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 builtin/rm.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..a5c9fc644e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -5,7 +5,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "advice.h"
@@ -40,14 +39,12 @@ static struct {
 	} *entry;
 } list;
 
-static int get_ours_cache_pos(const char *path, int pos)
+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)
 {
-	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) {
-			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;
 
@@ -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 &&
-- 
2.48.1

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

[Junio C Hamano]

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

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

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

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

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

[Arnav Bhate]

Junio C Hamano <gitster@pobox.com> writes:

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

My logic was that it was the inversion of the variable pos, but your
logic makes more sense. I'll make the change.
 
>>  {
>> -	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.

Thanks for the explanation, I was not able to get this from the code.

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

-- 
Regards,
Arnav Bhate
(He/Him)

[GSoC PATCH v3] rm: fix sign comparison warnings

[Arnav Bhate]

There are multiple places 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.

get_ours_cache_pos is a special case where i, though derived from a
signed variable is never negative. Move this part to the caller side
and make i an unsigned argument of the function. Rename i to
pos to make it descriptive, now that it is a function argument.

Replace signed data types with unsigned data types and vice versa
wherever necessary. Where both signed and unsigned data types have been
used, define a new variable in the scope of the for loop for use as the
iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 builtin/rm.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..a5c9fc644e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -5,7 +5,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "advice.h"
@@ -40,14 +39,12 @@ static struct {
 	} *entry;
 } list;
 
-static int get_ours_cache_pos(const char *path, int pos)
+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)
 {
-	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) {
-			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;
 
@@ -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 &&
-- 
2.48.1

[GSoC PATCH v3] rm: fix sign comparison warnings

[Arnav Bhate]

There are multiple places 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.

get_ours_cache_pos is a special case where i, though derived from a
signed variable is never negative. Move this part to the caller side
and make i an unsigned argument of the function. Rename i to
pos to make it descriptive, now that it is a function argument.

Replace signed data types with unsigned data types and vice versa
wherever necessary. Where both signed and unsigned data types have been
used, define a new variable in the scope of the for loop for use as the
iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 builtin/rm.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..a5c9fc644e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -5,7 +5,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "advice.h"
@@ -40,14 +39,12 @@ static struct {
 	} *entry;
 } list;
 
-static int get_ours_cache_pos(const char *path, int pos)
+static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)
 {
-	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) {
-			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;
 
@@ -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 &&
-- 
2.48.1

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

[Arnav Bhate]

Arnav Bhate <bhatearnav@gmail.com> writes:

> There are multiple places 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.
> 
> get_ours_cache_pos is a special case where i, though derived from a
> signed variable is never negative. Move this part to the caller side
> and make i an unsigned argument of the function. Rename i to
> pos to make it descriptive, now that it is a function argument.
> 
> Replace signed data types with unsigned data types and vice versa
> wherever necessary. Where both signed and unsigned data types have been
> used, define a new variable in the scope of the for loop for use as the
> iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.
> 
> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
> ---
>  builtin/rm.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 12ae086a55..a5c9fc644e 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -5,7 +5,6 @@
>   */
>  
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "builtin.h"
>  #include "advice.h"
> @@ -40,14 +39,12 @@ static struct {
>  	} *entry;
>  } list;
>  
> -static int get_ours_cache_pos(const char *path, int pos)
> +static int get_ours_cache_pos(const char *path, unsigned int inverted_pos)
>  {
> -	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) {
> -			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;
>  
> @@ -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 &&


Please ignore this one, I sent the old patch accidentally.
-- 
Regards,
Arnav Bhate
(He/Him)

[GSoC PATCH v4] rm: fix sign comparison warnings

[Arnav Bhate]

There are multiple places 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.

get_ours_cache_pos is a special case where i, though derived from a
signed variable is never negative. Move this part to the caller side
and make i an unsigned argument of the function. Rename i to
pos to make it descriptive, now that it is a function argument.

Replace signed data types with unsigned data types and vice versa
wherever necessary. Where both signed and unsigned data types have been
used, define a new variable in the scope of the for loop for use as the
iterator. Remove #define DISABLE_SIGN_COMPARE_WARNINGS.

Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
---
 builtin/rm.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 12ae086a55..a6565a69cf 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -5,7 +5,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "advice.h"
@@ -40,14 +39,12 @@ static struct {
 	} *entry;
 } list;
 
-static int get_ours_cache_pos(const char *path, int pos)
+static int get_ours_cache_pos(const char *path, unsigned int pos)
 {
-	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 ((pos < the_repository->index->cache_nr) && !strcmp(the_repository->index->cache[pos]->name, path)) {
+		if (ce_stage(the_repository->index->cache[pos]) == 2)
+			return pos;
+		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) {
-			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;
 
@@ -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 &&
-- 
2.49.0

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

[Arnav Bhate]

I did not notice that the patch I had sent previously had a mistake.
Now, I don't remember how it happened, but I have fixed it. Probably a
mistake in the arguments to git format-patch. This patch should have the
correct name for the variable.

-- 
Regards,
Arnav Bhate
(He/Him)