{"thread":{"id":"63174","subject":"[PATCH] bulk-checkin: fix sign compare warnings","startedAt":"2025-03-21T20:07:27Z","lastAt":"2025-03-24T23:46:37Z","messageCount":9,"participants":["Tuomas Ahola","Karthik Nayak","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514841","messageId":"20250321200715.3338-1-taahol@utu.fi","threadId":"63174","inReplyTo":null,"subject":"[PATCH] bulk-checkin: fix sign compare warnings","fromName":"Tuomas Ahola","fromEmail":"taahol@utu.fi","sentAt":"2025-03-21T20:07:15Z","receivedAt":"2025-03-21T20:07:27Z","isPatch":true,"sender":{"key":"taahol@utu.fi","avatar":"https://avatars.githubusercontent.com/u/114303477?v=4"},"body":"In file bulk-checkin.c, three warnings are emitted by\n\"-Wsign-compare\", two of which are caused by trivial loop iterator\ntype mismatches. The third one is also an uncomplicated case for\nwhich a simple cast is a sufficient remedy.\n\nFix issues accordingly, and enable sign compare warnings for the file.\n\nSigned-off-by: Tuomas Ahola \n---\n bulk-checkin.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 20f2da67b9..0133427132 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -3,7 +3,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"bulk-checkin.h\"\n@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n-\tint i;\n \n \tif (!state->f)\n \t\treturn;\n@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, hash);\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n clear_exit:\n@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n {\n-\tint i;\n-\n \t/* The object may already exist in the repository */\n \tif (repo_has_object_file(the_repository, oid))\n \t\treturn 1;\n \n \t/* Might want to keep the list sorted */\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tif (oideq(&state->written[i]->oid, oid))\n \t\t\treturn 1;\n \n@@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n-\t\t\t\tif (rsize < hsize)\n+\t\t\t\tif ((size_t)rsize < hsize)\n \t\t\t\t\thsize = rsize;\n \t\t\t\tif (hsize)\n \t\t\t\t\tgit_hash_update(ctx, ibuf, hsize);\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.30.2\n\n"},{"id":"514843","messageId":"CAOLa=ZRN5m0bccMdabUYwNJLg4HX6jcOe3PN-aBTHXBOuM71hw@mail.gmail.com","threadId":"63174","inReplyTo":"20250321200715.3338-1-taahol@utu.fi","subject":"Re: [PATCH] bulk-checkin: fix sign compare warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-21T21:08:06Z","receivedAt":"2025-03-21T21:08:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Tuomas Ahola writes:\n\n> In file bulk-checkin.c, three warnings are emitted by\n> \"-Wsign-compare\", two of which are caused by trivial loop iterator\n> type mismatches. The third one is also an uncomplicated case for\n> which a simple cast is a sufficient remedy.\n>\n\nNit: it would be nice if you expanded on why 'a simple cast is a\nsufficient remedy' and more importantly how that casting is safe.\n\n> Fix issues accordingly, and enable sign compare warnings for the file.\n>\n> Signed-off-by: Tuomas Ahola \n> ---\n> bulk-checkin.c | 10 +++-------\n> 1 file changed, 3 insertions(+), 7 deletions(-)\n>\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 20f2da67b9..0133427132 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -3,7 +3,6 @@\n> */\n>\n> #define USE_THE_REPOSITORY_VARIABLE\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n>\n> #include \"git-compat-util.h\"\n> #include \"bulk-checkin.h\"\n> @@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n> {\n> \tunsigned char hash[GIT_MAX_RAWSZ];\n> \tstruct strbuf packname = STRBUF_INIT;\n> -\tint i;\n>\n> \tif (!state->f)\n> \t\treturn;\n> @@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n> \tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n> \t\t\t state->written, state->nr_written,\n> \t\t\t &state->pack_idx_opts, hash);\n> -\tfor (i = 0; i < state->nr_written; i++)\n> +\tfor (uint32_t i = 0; i < state->nr_written; i++)\n> \t\tfree(state->written[i]);\n>\n> clear_exit:\n> @@ -131,14 +129,12 @@ static void flush_batch_fsync(void)\n>\n> static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n> {\n> -\tint i;\n> -\n> \t/* The object may already exist in the repository */\n> \tif (repo_has_object_file(the_repository, oid))\n> \t\treturn 1;\n>\n> \t/* Might want to keep the list sorted */\n> -\tfor (i = 0; i < state->nr_written; i++)\n> +\tfor (uint32_t i = 0; i < state->nr_written; i++)\n> \t\tif (oideq(&state->written[i]->oid, oid))\n> \t\t\treturn 1;\n>\n> @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n> \t\t\toffset += rsize;\n> \t\t\tif (*already_hashed_to < offset) {\n> \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n> -\t\t\t\tif (rsize < hsize)\n> +\t\t\t\tif ((size_t)rsize < hsize)\n\nSomething I found peculiar here is that `rsize` is of type ssize_t'.\nBut it only seems to store a positive value.\n\n> \t\t\t\t\thsize = rsize;\n> \t\t\t\tif (hsize)\n> \t\t\t\t\tgit_hash_update(ctx, ibuf, hsize);\n>\n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n> --\n> 2.30.2\n"},{"id":"514846","messageId":"20250321221404.10727-1-taahol@utu.fi","threadId":"63174","inReplyTo":"CAOLa=ZRN5m0bccMdabUYwNJLg4HX6jcOe3PN-aBTHXBOuM71hw@mail.gmail.com","subject":"[PATCH v2] bulk-checkin: fix sign compare warnings","fromName":"Tuomas Ahola","fromEmail":"taahol@utu.fi","sentAt":"2025-03-21T22:14:04Z","receivedAt":"2025-03-21T22:14:13Z","isPatch":true,"sender":{"key":"taahol@utu.fi","avatar":"https://avatars.githubusercontent.com/u/114303477?v=4"},"body":"In file bulk-checkin.c, three warnings are emitted by\n\"-Wsign-compare\", two of which are caused by trivial loop iterator\ntype mismatches. The third one is also an uncomplicated case for\nwhich a simple cast is a safe and sufficient action as the variable in\nquestion only holds positive values (from sizeof() expression).\n\nFix issues accordingly, and enable sign compare warnings for the file.\n\nSigned-off-by: Tuomas Ahola \n---\nIntervall-diff mot v1:\n1: 25b56dae76 ! 1: 289f3a0278 bulk-checkin: fix sign compare warnings\n @@ Commit message\n In file bulk-checkin.c, three warnings are emitted by\n \"-Wsign-compare\", two of which are caused by trivial loop iterator\n type mismatches. The third one is also an uncomplicated case for\n - which a simple cast is a sufficient remedy.\n + which a simple cast is a safe and sufficient action as the variable in\n + question only holds positive values (from sizeof() expression).\n \n Fix issues accordingly, and enable sign compare warnings for the file.\n \n\n bulk-checkin.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 20f2da67b9..0133427132 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -3,7 +3,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"bulk-checkin.h\"\n@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n-\tint i;\n \n \tif (!state->f)\n \t\treturn;\n@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, hash);\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n clear_exit:\n@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n {\n-\tint i;\n-\n \t/* The object may already exist in the repository */\n \tif (repo_has_object_file(the_repository, oid))\n \t\treturn 1;\n \n \t/* Might want to keep the list sorted */\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tif (oideq(&state->written[i]->oid, oid))\n \t\t\treturn 1;\n \n@@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n-\t\t\t\tif (rsize < hsize)\n+\t\t\t\tif ((size_t)rsize < hsize)\n \t\t\t\t\thsize = rsize;\n \t\t\t\tif (hsize)\n \t\t\t\t\tgit_hash_update(ctx, ibuf, hsize);\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.30.2\n\n"},{"id":"514886","messageId":"xmqqo6xrqu2k.fsf@gitster.g","threadId":"63174","inReplyTo":"20250321221404.10727-1-taahol@utu.fi","subject":"Re: [PATCH v2] bulk-checkin: fix sign compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-23T22:08:03Z","receivedAt":"2025-03-23T22:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tuomas Ahola writes:\n\n> In file bulk-checkin.c, three warnings are emitted by\n> \"-Wsign-compare\", two of which are caused by trivial loop iterator\n> type mismatches. The third one is also an uncomplicated case for\n> which a simple cast is a safe and sufficient action as the variable in\n> question only holds positive values (from sizeof() expression).\n\nThe point of the sign-compare is that a positive value that is\nassigned to a signed variable may wrap around to become negative,\ncausing a comparison with an unsigned type with the same size to\nfail.\n\nSo \"only holds positive\" is not a good enough explanation for the\nreason why this workaround for the \"-Wsign-compare\" false-positive [*]\ndoes not make things too bad. The key thing is that the value\nassigned to this \"ssize_t rsize\" variable is a small non-negative\nvalue that can fit both size_t and ssize_t.\n\n\n[Footnote]\n\n * If we take -Wsign-compare too literally, it is warning every time\n a signed quantity and an unsigned quantity is being compared, so\n we could argue that there is no false-positive. But that is an\n obviously pretty useless warning, when we can trivially tell that\n the value in a signed variable cannot have wrapped around.\n"},{"id":"514894","messageId":"20250324025300.GA690113@coredump.intra.peff.net","threadId":"63174","inReplyTo":"CAOLa=ZRN5m0bccMdabUYwNJLg4HX6jcOe3PN-aBTHXBOuM71hw@mail.gmail.com","subject":"Re: [PATCH] bulk-checkin: fix sign compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T02:53:00Z","receivedAt":"2025-03-24T02:59:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:\n\n> > @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n> > \t\t\toffset += rsize;\n> > \t\t\tif (*already_hashed_to < offset) {\n> > \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n> > -\t\t\t\tif (rsize < hsize)\n> > +\t\t\t\tif ((size_t)rsize < hsize)\n> \n> Something I found peculiar here is that `rsize` is of type ssize_t'.\n> But it only seems to store a positive value.\n\nI assumed it was ssize_t because it would hold the result of a read\ncall. But it doesn't! We put that into the \"read_result\" variable.\n\nSo it could just be a size_t in the first place. And indeed it is better\nas one, because we assign from \"size\", which is itself a size_t. We do\nnot yet warn about type mismatches outside of comparisons, but really it\nis equally bad.\n\nHowever, if you switch it, then we get a different -Wsign-compare\nproblem: we compare \"rsize\" and \"read_result\". So you still have to\ncast, but at a different spot.\n\nIf we are doing this a lot (and really this conversion is necessary any\ntime you look at the outcome of a read call), I do still wonder if we\nshould have a helper like:\n\nstatic inline int safe_scast(ssize_t ret, size_t *out)\n{\n\tif (ret < 0)\n\t\treturn 0;\n\t/* cast is safe because of check above */\n\t*out = (size_t)ret;\n\treturn 1;\n}\n\n(yes, I know the name is lousy). That would allow something like this:\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex f6f79cb9e2..fbffc7c8d6 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -178,9 +178,10 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \twhile (status != Z_STREAM_END) {\n \t\tif (size && !s.avail_in) {\n-\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n-\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n-\t\t\tif (read_result < 0)\n+\t\t\tsize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n+\t\t\tsize_t read_result;\n+\n+\t\t\tif (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))\n \t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n \t\t\tif (read_result != rsize)\n \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n\nThough it does kind of obscure the call to read_in_full(). You can use\ntwo variables, like:\n\n ssize_t read_result;\n size_t bytes_read;\n\n read_result = read_in_full(fd, ibuf, rsize);\n if (!safe_scast(read_result, &bytes_read))\n\tdie_errno(...);\n\nwhich is a bit more verbose but perhaps clearer.\n\nThis reminded me a bit of the issues we had with write_in_full() before,\nwhere:\n\n if (write_in_full(fd, buf, len) < len)\n\nbehaves unexpectedly because of integer conversions. There the solution\nwas to never check against \"len\", because write_in_full() either writes\neverything or returns an error. So:\n\n if (write_in_full(fd, buf, len) < 0)\n\nis correct and sufficient.\n\nBut alas, we can't do the same here, because reading returns three\ncases: error, a full read, or a partial read (maybe even EOF!). So we\nreally do need to record and compare the return value between what we\nasked for and what we got.\n\n-Peff\n"},{"id":"514962","messageId":"CAOLa=ZRkzp6A+S-bqbUMnkovazrczFi=B8tG06xqTzsNQB2enA@mail.gmail.com","threadId":"63174","inReplyTo":"20250324025300.GA690113@coredump.intra.peff.net","subject":"Re: [PATCH] bulk-checkin: fix sign compare warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-24T19:48:59Z","receivedAt":"2025-03-24T19:49:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:\n>\n>> > @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n>> > \t\t\toffset += rsize;\n>> > \t\t\tif (*already_hashed_to < offset) {\n>> > \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n>> > -\t\t\t\tif (rsize < hsize)\n>> > +\t\t\t\tif ((size_t)rsize < hsize)\n>>\n>> Something I found peculiar here is that `rsize` is of type ssize_t'.\n>> But it only seems to store a positive value.\n>\n> I assumed it was ssize_t because it would hold the result of a read\n> call. But it doesn't! We put that into the \"read_result\" variable.\n>\n> So it could just be a size_t in the first place. And indeed it is better\n> as one, because we assign from \"size\", which is itself a size_t. We do\n> not yet warn about type mismatches outside of comparisons, but really it\n> is equally bad.\n\nNice, thanks for exploring this thought out more. I did look at the\ncode, but was more cursory.\n\n> However, if you switch it, then we get a different -Wsign-compare\n> problem: we compare \"rsize\" and \"read_result\". So you still have to\n> cast, but at a different spot.\n>\n\nTrue. But this would be better in my regards, since this would directly\nfollow the\n\n if (read_result < 0)\n die_errno(\"failed to read from '%s'\", path);\n\ncode, so a `if ((size_t)read_result != rsize)` here makes logical sense\nsince we can clearly see that this section is only reached when\n`read_result` has a positive value.\n\n> If we are doing this a lot (and really this conversion is necessary any\n> time you look at the outcome of a read call), I do still wonder if we\n> should have a helper like:\n>\n> static inline int safe_scast(ssize_t ret, size_t *out)\n> {\n> \tif (ret < 0)\n> \t\treturn 0;\n> \t/* cast is safe because of check above */\n> \t*out = (size_t)ret;\n> \treturn 1;\n> }\n>\n> (yes, I know the name is lousy). That would allow something like this:\n>\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index f6f79cb9e2..fbffc7c8d6 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -178,9 +178,10 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n>\n> \twhile (status != Z_STREAM_END) {\n> \t\tif (size && !s.avail_in) {\n> -\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n> -\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n> -\t\t\tif (read_result < 0)\n> +\t\t\tsize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n> +\t\t\tsize_t read_result;\n> +\n> +\t\t\tif (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))\n> \t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n> \t\t\tif (read_result != rsize)\n> \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n>\n\nThis does look nice, but I'm worried something like `safe_scast` would\njust not be used througout the codebase, causing inconsistencies. But I\nthink we can drive that through reviews.\n\n> Though it does kind of obscure the call to read_in_full(). You can use\n> two variables, like:\n>\n> ssize_t read_result;\n> size_t bytes_read;\n>\n> read_result = read_in_full(fd, ibuf, rsize);\n> if (!safe_scast(read_result, &bytes_read))\n> \tdie_errno(...);\n>\n> which is a bit more verbose but perhaps clearer.\n\nYeah this is much better too.\n\n> This reminded me a bit of the issues we had with write_in_full() before,\n> where:\n>\n> if (write_in_full(fd, buf, len) < len)\n>\n> behaves unexpectedly because of integer conversions. There the solution\n> was to never check against \"len\", because write_in_full() either writes\n> everything or returns an error. So:\n>\n> if (write_in_full(fd, buf, len) < 0)\n>\n> is correct and sufficient.\n>\n> But alas, we can't do the same here, because reading returns three\n> cases: error, a full read, or a partial read (maybe even EOF!). So we\n> really do need to record and compare the return value between what we\n> asked for and what we got.\n>\n\nThis is to some extent a flaw in the way errors are generally\nstructured where the error indication (-1 here) and a potential result\n(bytes read) are combined into a single return.\n\nIt is unfortunate indeed.\n\n> -Peff\n"},{"id":"514966","messageId":"20250324201343.GA777700@coredump.intra.peff.net","threadId":"63174","inReplyTo":"CAOLa=ZRkzp6A+S-bqbUMnkovazrczFi=B8tG06xqTzsNQB2enA@mail.gmail.com","subject":"Re: [PATCH] bulk-checkin: fix sign compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T20:13:43Z","receivedAt":"2025-03-24T20:13:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 24, 2025 at 07:48:59PM +0000, Karthik Nayak wrote:\n\n> > However, if you switch it, then we get a different -Wsign-compare\n> > problem: we compare \"rsize\" and \"read_result\". So you still have to\n> > cast, but at a different spot.\n> \n> True. But this would be better in my regards, since this would directly\n> follow the\n> \n> if (read_result < 0)\n> die_errno(\"failed to read from '%s'\", path);\n> \n> code, so a `if ((size_t)read_result != rsize)` here makes logical sense\n> since we can clearly see that this section is only reached when\n> `read_result` has a positive value.\n\nYeah, agreed. And after doing that, it's probably the right stopping\npoint for this patch.\n\nI do think the safe_scast() thing could be a good general tool for this\nkind of case, but I'd rather not hold up an immediate fix for it.\n\n-Peff\n"},{"id":"514974","messageId":"20250324214703.7547-1-taahol@utu.fi","threadId":"63174","inReplyTo":"20250321200715.3338-1-taahol@utu.fi","subject":"[PATCH v3] bulk-checkin: fix sign compare warnings","fromName":"Tuomas Ahola","fromEmail":"taahol@utu.fi","sentAt":"2025-03-24T21:47:03Z","receivedAt":"2025-03-24T21:47:36Z","isPatch":true,"sender":{"key":"taahol@utu.fi","avatar":"https://avatars.githubusercontent.com/u/114303477?v=4"},"body":"In file bulk-checkin.c, three warnings are emitted by\n\"-Wsign-compare\", two of which are caused by trivial loop iterator\ntype mismatches. For the third case, the type of `rsize` from\n\n\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n\ncan be changed to size_t as both options of the ternary expression are\nunsigned and the signedness of the variable isn't really needed\nanywhere.\n\nTo prevent `read_result != rsize` making a clash, it is to be noted\nthat `read_result` is checked not to hold negative values. Therefore\ncasting the variable to size_t is a safe operation and enough to\nremove the sign-compare warning.\n\nFix issues accordingly, and remove `DISABLE_SIGN_COMPARE_WARNINGS` to\nenable \"-Wsign-compare\" for the file.\n\nSigned-off-by: Tuomas Ahola \n---\n\nNotes:\n Okay, I think I got it know. Thanks for bearing with me.\n \n Range-diff against v2:\n \n ## bulk-checkin.c ##\n @@\n */\n @@ bulk-checkin.c: static void flush_batch_fsync(void)\n \t\t\treturn 1;\n \n @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n +\n + \twhile (status != Z_STREAM_END) {\n + \t\tif (size && !s.avail_in) {\n +-\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n ++\t\t\tsize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n + \t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n + \t\t\tif (read_result < 0)\n + \t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n +-\t\t\tif (read_result != rsize)\n +-\t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n +-\t\t\t\t (int)rsize, path);\n ++\t\t\tif ((size_t)read_result != rsize)\n ++\t\t\t\tdie(\"failed to read %u bytes from '%s'\",\n ++\t\t\t\t (unsigned)rsize, path);\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n --\t\t\t\tif (rsize < hsize)\n -+\t\t\t\tif ((size_t)rsize < hsize)\n - \t\t\t\t\thsize = rsize;\n - \t\t\t\tif (hsize)\n - \t\t\t\t\tgit_hash_update(ctx, ibuf, hsize);\n\n bulk-checkin.c | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 20f2da67b9..a5a3395188 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -3,7 +3,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"bulk-checkin.h\"\n@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct strbuf packname = STRBUF_INIT;\n-\tint i;\n \n \tif (!state->f)\n \t\treturn;\n@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)\n \tfinish_tmp_packfile(&packname, state->pack_tmp_name,\n \t\t\t state->written, state->nr_written,\n \t\t\t &state->pack_idx_opts, hash);\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tfree(state->written[i]);\n \n clear_exit:\n@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)\n \n static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)\n {\n-\tint i;\n-\n \t/* The object may already exist in the repository */\n \tif (repo_has_object_file(the_repository, oid))\n \t\treturn 1;\n \n \t/* Might want to keep the list sorted */\n-\tfor (i = 0; i < state->nr_written; i++)\n+\tfor (uint32_t i = 0; i < state->nr_written; i++)\n \t\tif (oideq(&state->written[i]->oid, oid))\n \t\t\treturn 1;\n \n@@ -182,13 +178,13 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n \n \twhile (status != Z_STREAM_END) {\n \t\tif (size && !s.avail_in) {\n-\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n+\t\t\tsize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n \t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n \t\t\tif (read_result < 0)\n \t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n-\t\t\tif (read_result != rsize)\n-\t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n-\t\t\t\t (int)rsize, path);\n+\t\t\tif ((size_t)read_result != rsize)\n+\t\t\t\tdie(\"failed to read %u bytes from '%s'\",\n+\t\t\t\t (unsigned)rsize, path);\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.30.2\n\n"},{"id":"514977","messageId":"20250324234635.GA789136@coredump.intra.peff.net","threadId":"63174","inReplyTo":"20250324214703.7547-1-taahol@utu.fi","subject":"Re: [PATCH v3] bulk-checkin: fix sign compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T23:46:35Z","receivedAt":"2025-03-24T23:46:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 24, 2025 at 11:47:03PM +0200, Tuomas Ahola wrote:\n\n> In file bulk-checkin.c, three warnings are emitted by\n> \"-Wsign-compare\", two of which are caused by trivial loop iterator\n> type mismatches. For the third case, the type of `rsize` from\n> \n> \t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n> \n> can be changed to size_t as both options of the ternary expression are\n> unsigned and the signedness of the variable isn't really needed\n> anywhere.\n> \n> To prevent `read_result != rsize` making a clash, it is to be noted\n> that `read_result` is checked not to hold negative values. Therefore\n> casting the variable to size_t is a safe operation and enough to\n> remove the sign-compare warning.\n\nThanks, this description (and the matching changes in the patch) look\ngood to me.\n\n-Peff\n"}]}