* [PATCH] bulk-checkin: fix sign compare warnings
@ 2025-03-21 20:07 Tuomas Ahola
2025-03-21 21:08 ` Karthik Nayak
2025-03-24 21:47 ` [PATCH v3] " Tuomas Ahola
0 siblings, 2 replies; 9+ messages in thread
From: Tuomas Ahola @ 2025-03-21 20:07 UTC (permalink / raw)
To: git; +Cc: Tuomas Ahola
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(-)
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)
hsize = rsize;
if (hsize)
git_hash_update(ctx, ibuf, hsize);
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] bulk-checkin: fix sign compare warnings
2025-03-21 20:07 [PATCH] bulk-checkin: fix sign compare warnings Tuomas Ahola
@ 2025-03-21 21:08 ` Karthik Nayak
2025-03-21 22:14 ` [PATCH v2] " Tuomas Ahola
2025-03-24 2:53 ` [PATCH] " Jeff King
2025-03-24 21:47 ` [PATCH v3] " Tuomas Ahola
1 sibling, 2 replies; 9+ messages in thread
From: Karthik Nayak @ 2025-03-21 21:08 UTC (permalink / raw)
To: Tuomas Ahola, git
[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]
Tuomas Ahola <taahol@utu.fi> writes:
> 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.
> 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.
> hsize = rsize;
> if (hsize)
> git_hash_update(ctx, ibuf, hsize);
>
> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
> --
> 2.30.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] bulk-checkin: fix sign compare warnings
2025-03-21 21:08 ` Karthik Nayak
@ 2025-03-21 22:14 ` Tuomas Ahola
2025-03-23 22:08 ` Junio C Hamano
2025-03-24 2:53 ` [PATCH] " Jeff King
1 sibling, 1 reply; 9+ messages in thread
From: Tuomas Ahola @ 2025-03-21 22:14 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Tuomas Ahola
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(-)
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)
hsize = rsize;
if (hsize)
git_hash_update(ctx, ibuf, hsize);
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] bulk-checkin: fix sign compare warnings
2025-03-21 22:14 ` [PATCH v2] " Tuomas Ahola
@ 2025-03-23 22:08 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2025-03-23 22:08 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Karthik Nayak
Tuomas Ahola <taahol@utu.fi> writes:
> 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).
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bulk-checkin: fix sign compare warnings
2025-03-21 21:08 ` Karthik Nayak
2025-03-21 22:14 ` [PATCH v2] " Tuomas Ahola
@ 2025-03-24 2:53 ` Jeff King
2025-03-24 19:48 ` Karthik Nayak
1 sibling, 1 reply; 9+ messages in thread
From: Jeff King @ 2025-03-24 2:53 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Tuomas Ahola, git
On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:
> > @@ -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.
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:
diff --git a/bulk-checkin.c b/bulk-checkin.c
index f6f79cb9e2..fbffc7c8d6 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -178,9 +178,10 @@ 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);
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
- if (read_result < 0)
+ size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
+ size_t read_result;
+
+ if (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))
die_errno("failed to read from '%s'", path);
if (read_result != rsize)
die("failed to read %d bytes from '%s'",
Though it does kind of obscure the call to read_in_full(). You can use
two variables, like:
ssize_t read_result;
size_t bytes_read;
read_result = read_in_full(fd, ibuf, rsize);
if (!safe_scast(read_result, &bytes_read))
die_errno(...);
which is a bit more verbose but perhaps clearer.
This reminded me a bit of the issues we had with write_in_full() before,
where:
if (write_in_full(fd, buf, len) < len)
behaves unexpectedly because of integer conversions. There the solution
was to never check against "len", because write_in_full() either writes
everything or returns an error. So:
if (write_in_full(fd, buf, len) < 0)
is correct and sufficient.
But alas, we can't do the same here, because reading returns three
cases: error, a full read, or a partial read (maybe even EOF!). So we
really do need to record and compare the return value between what we
asked for and what we got.
-Peff
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] bulk-checkin: fix sign compare warnings
2025-03-24 2:53 ` [PATCH] " Jeff King
@ 2025-03-24 19:48 ` Karthik Nayak
2025-03-24 20:13 ` Jeff King
0 siblings, 1 reply; 9+ messages in thread
From: Karthik Nayak @ 2025-03-24 19:48 UTC (permalink / raw)
To: Jeff King; +Cc: Tuomas Ahola, git
[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]
Jeff King <peff@peff.net> writes:
> On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:
>
>> > @@ -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.
>
> 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.
Nice, thanks for exploring this thought out more. I did look at the
code, but was more cursory.
> 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.
>
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.
> 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:
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index f6f79cb9e2..fbffc7c8d6 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -178,9 +178,10 @@ 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);
> - ssize_t read_result = read_in_full(fd, ibuf, rsize);
> - if (read_result < 0)
> + size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
> + size_t read_result;
> +
> + if (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))
> die_errno("failed to read from '%s'", path);
> if (read_result != rsize)
> die("failed to read %d bytes from '%s'",
>
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.
> Though it does kind of obscure the call to read_in_full(). You can use
> two variables, like:
>
> ssize_t read_result;
> size_t bytes_read;
>
> read_result = read_in_full(fd, ibuf, rsize);
> if (!safe_scast(read_result, &bytes_read))
> die_errno(...);
>
> which is a bit more verbose but perhaps clearer.
Yeah this is much better too.
> This reminded me a bit of the issues we had with write_in_full() before,
> where:
>
> if (write_in_full(fd, buf, len) < len)
>
> behaves unexpectedly because of integer conversions. There the solution
> was to never check against "len", because write_in_full() either writes
> everything or returns an error. So:
>
> if (write_in_full(fd, buf, len) < 0)
>
> is correct and sufficient.
>
> But alas, we can't do the same here, because reading returns three
> cases: error, a full read, or a partial read (maybe even EOF!). So we
> really do need to record and compare the return value between what we
> asked for and what we got.
>
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.
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] bulk-checkin: fix sign compare warnings
2025-03-24 19:48 ` Karthik Nayak
@ 2025-03-24 20:13 ` Jeff King
0 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2025-03-24 20:13 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Tuomas Ahola, git
On Mon, Mar 24, 2025 at 07:48:59PM +0000, Karthik Nayak wrote:
> > 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.
>
> 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] bulk-checkin: fix sign compare warnings
2025-03-21 20:07 [PATCH] bulk-checkin: fix sign compare warnings Tuomas Ahola
2025-03-21 21:08 ` Karthik Nayak
@ 2025-03-24 21:47 ` Tuomas Ahola
2025-03-24 23:46 ` Jeff King
1 sibling, 1 reply; 9+ messages in thread
From: Tuomas Ahola @ 2025-03-24 21:47 UTC (permalink / raw)
To: git; +Cc: peff, karthik.188, gitster, taahol
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(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 20f2da67b9..a5a3395188 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;
@@ -182,13 +178,13 @@ 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;
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] bulk-checkin: fix sign compare warnings
2025-03-24 21:47 ` [PATCH v3] " Tuomas Ahola
@ 2025-03-24 23:46 ` Jeff King
0 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2025-03-24 23:46 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, karthik.188, gitster
On Mon, Mar 24, 2025 at 11:47:03PM +0200, Tuomas Ahola wrote:
> 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.
Thanks, this description (and the matching changes in the patch) look
good to me.
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-24 23:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-21 20:07 [PATCH] bulk-checkin: fix sign compare warnings Tuomas Ahola
2025-03-21 21:08 ` Karthik Nayak
2025-03-21 22:14 ` [PATCH v2] " Tuomas Ahola
2025-03-23 22:08 ` Junio C Hamano
2025-03-24 2:53 ` [PATCH] " Jeff King
2025-03-24 19:48 ` Karthik Nayak
2025-03-24 20:13 ` Jeff King
2025-03-24 21:47 ` [PATCH v3] " Tuomas Ahola
2025-03-24 23:46 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).