patchbulk-checkin: fix sign compare warnings

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.

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(-)

Tuomas Ahola <taahol@utu.fi> writes:

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.

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

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 safe and sufficient action as the variable in question only holds positive values (from sizeof() expression).

Fix issues accordingly, and enable sign compare warnings for the file.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
Intervall-diff mot v1:
1:  25b56dae76 ! 1:  289f3a0278 bulk-checkin: fix sign compare warnings
    @@ Commit message
         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.
    +    which a simple cast is a safe and sufficient action as the variable in
    +    question only holds positive values (from sizeof() expression).
     
         Fix issues accordingly, and enable sign compare warnings for the file.
     

 bulk-checkin.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Tuomas Ahola <taahol@utu.fi> writes:

The point of the sign-compare is that a positive value that is assigned to a signed variable may wrap around to become negative, causing a comparison with an unsigned type with the same size to fail.

So "only holds positive" is not a good enough explanation for the reason why this workaround for the "-Wsign-compare" false-positive [*] does not make things too bad. The key thing is that the value assigned to this "ssize_t rsize" variable is a small non-negative value that can fit both size_t and ssize_t.

[Footnote]

 * If we take -Wsign-compare too literally, it is warning every time
   a signed quantity and an unsigned quantity is being compared, so
   we could argue that there is no false-positive.  But that is an
   obviously pretty useless warning, when we can trivially tell that
   the value in a signed variable cannot have wrapped around.

On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:

I assumed it was ssize_t because it would hold the result of a read call. But it doesn't! We put that into the "read_result" variable.

So it could just be a size_t in the first place. And indeed it is better as one, because we assign from "size", which is itself a size_t. We do not yet warn about type mismatches outside of comparisons, but really it is equally bad.

However, if you switch it, then we get a different -Wsign-compare problem: we compare "rsize" and "read_result". So you still have to cast, but at a different spot.

If we are doing this a lot (and really this conversion is necessary any time you look at the outcome of a read call), I do still wonder if we should have a helper like:

static inline int safe_scast(ssize_t ret, size_t *out)
{
	if (ret < 0)
		return 0;
	/* cast is safe because of check above */
	*out = (size_t)ret;
	return 1;
}

(yes, I know the name is lousy). That would allow something like this:

Jeff King <peff@peff.net> writes:

Nice, thanks for exploring this thought out more. I did look at the code, but was more cursory.

True. But this would be better in my regards, since this would directly follow the

  if (read_result < 0)
     die_errno("failed to read from '%s'", path);

code, so a `if ((size_t)read_result != rsize)` here makes logical sense since we can clearly see that this section is only reached when `read_result` has a positive value.

This does look nice, but I'm worried something like `safe_scast` would just not be used througout the codebase, causing inconsistencies. But I think we can drive that through reviews.

Yeah this is much better too.

This is to some extent a flaw in the way errors are generally structured where the error indication (-1 here) and a potential result (bytes read) are combined into a single return.

It is unfortunate indeed.

On Mon, Mar 24, 2025 at 07:48:59PM +0000, Karthik Nayak wrote:

Yeah, agreed. And after doing that, it's probably the right stopping point for this patch.

I do think the safe_scast() thing could be a good general tool for this kind of case, but I'd rather not hold up an immediate fix for it.

-Peff

In file bulk-checkin.c, three warnings are emitted by "-Wsign-compare", two of which are caused by trivial loop iterator type mismatches. For the third case, the type of `rsize` from

			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);

can be changed to size_t as both options of the ternary expression are unsigned and the signedness of the variable isn't really needed anywhere.

To prevent `read_result != rsize` making a clash, it is to be noted that `read_result` is checked not to hold negative values. Therefore casting the variable to size_t is a safe operation and enough to remove the sign-compare warning.

Fix issues accordingly, and remove `DISABLE_SIGN_COMPARE_WARNINGS` to enable "-Wsign-compare" for the file.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---

Notes:
    Okay, I think I got it know.  Thanks for bearing with me.
    
    Range-diff against v2:
    
          ## bulk-checkin.c ##
         @@
           */
        @@ bulk-checkin.c: static void flush_batch_fsync(void)
          			return 1;
    
         @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
        +
        + 	while (status != Z_STREAM_END) {
        + 		if (size && !s.avail_in) {
        +-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
        ++			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
        + 			ssize_t read_result = read_in_full(fd, ibuf, rsize);
        + 			if (read_result < 0)
        + 				die_errno("failed to read from '%s'", path);
        +-			if (read_result != rsize)
        +-				die("failed to read %d bytes from '%s'",
        +-				    (int)rsize, path);
        ++			if ((size_t)read_result != rsize)
        ++				die("failed to read %u bytes from '%s'",
        ++				    (unsigned)rsize, path);
          			offset += rsize;
          			if (*already_hashed_to < offset) {
          				size_t hsize = offset - *already_hashed_to;
        --				if (rsize < hsize)
        -+				if ((size_t)rsize < hsize)
        - 					hsize = rsize;
        - 				if (hsize)
        - 					git_hash_update(ctx, ibuf, hsize);

 bulk-checkin.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

On Mon, Mar 24, 2025 at 11:47:03PM +0200, Tuomas Ahola wrote:

Thanks, this description (and the matching changes in the patch) look good to me.

-Peff