Re: [PATCH] bulk-checkin: fix sign compare warnings

From: Karthik Nayak <karthik.188@gmail.com>
Date: Mar 21, 2025, 21:08 UTC
Message-ID: <CAOLa=ZRN5m0bccMdabUYwNJLg4HX6jcOe3PN-aBTHXBOuM71hw@mail.gmail.com>
In-Reply-To: <20250321200715.3338-1-taahol@utu.fi>

Tuomas Ahola <taahol@utu.fi> writes:

Show 5 quoted lines

> In file bulk-checkin.c, three warnings are emitted by
> "-Wsign-compare", two of which are caused by trivial loop iterator
> type mismatches.  The third one is also an uncomplicated case for
> which a simple cast is a sufficient remedy.
>

Nit: it would be nice if you expanded on why 'a simple cast is a
sufficient remedy' and more importantly how that casting is safe.

Show 58 quoted lines

> Fix issues accordingly, and enable sign compare warnings for the file.
>
> Signed-off-by: Tuomas Ahola <taahol@utu.fi>
> ---
>  bulk-checkin.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 20f2da67b9..0133427132 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -3,7 +3,6 @@
>   */
>
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
>  #include "git-compat-util.h"
>  #include "bulk-checkin.h"
> @@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
>  {
>  	unsigned char hash[GIT_MAX_RAWSZ];
>  	struct strbuf packname = STRBUF_INIT;
> -	int i;
>
>  	if (!state->f)
>  		return;
> @@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
>  	finish_tmp_packfile(&packname, state->pack_tmp_name,
>  			    state->written, state->nr_written,
>  			    &state->pack_idx_opts, hash);
> -	for (i = 0; i < state->nr_written; i++)
> +	for (uint32_t i = 0; i < state->nr_written; i++)
>  		free(state->written[i]);
>
>  clear_exit:
> @@ -131,14 +129,12 @@ static void flush_batch_fsync(void)
>
>  static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
>  {
> -	int i;
> -
>  	/* The object may already exist in the repository */
>  	if (repo_has_object_file(the_repository, oid))
>  		return 1;
>
>  	/* Might want to keep the list sorted */
> -	for (i = 0; i < state->nr_written; i++)
> +	for (uint32_t i = 0; i < state->nr_written; i++)
>  		if (oideq(&state->written[i]->oid, oid))
>  			return 1;
>
> @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
>  			offset += rsize;
>  			if (*already_hashed_to < offset) {
>  				size_t hsize = offset - *already_hashed_to;
> -				if (rsize < hsize)
> +				if ((size_t)rsize < hsize)

Something I found peculiar here is that `rsize` is of type ssize_t'. But it only seems to store a positive value.

Show 7 quoted lines

>  					hsize = rsize;
>  				if (hsize)
>  					git_hash_update(ctx, ibuf, hsize);
>
> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
> --
> 2.30.2