{"thread":{"id":"56486","subject":"[PATCH] strvec: use size_t to store nr and alloc","startedAt":"2021-09-11T15:01:19Z","lastAt":"2021-09-13T17:20:26Z","messageCount":23,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Philip Oakley","Carlo Arenas","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"435557","messageId":"YTzEvLHWcfuD20x4@coredump.intra.peff.net","threadId":"56486","inReplyTo":null,"subject":"[PATCH] strvec: use size_t to store nr and alloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T15:01:16Z","receivedAt":"2021-09-11T15:01:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We converted argv_array (which later became strvec) to use size_t in\n819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\norder to avoid the possibility of integer overflow. But later, commit\nd70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\nconverted these back to ints!\n\nThose two commits were part of the same patch series. I'm pretty sure\nwhat happened is that they were originally written in the opposite order\nand then cleaned up and re-ordered during an interactive rebase. And\nwhen resolving the inevitable conflict, I mistakenly took the \"rename\"\npatch completely, accidentally dropping the type change.\n\nWe can correct it now; better late than never.\n\nSigned-off-by: Jeff King \n---\nThis was posted previously in the midst of another thread, but I don't\nthink was picked up. There was some positive reaction, but one \"do we\nreally need this?\" to which I responded in detail:\n\n https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n\nI don't really think any of that needs to go into the commit message,\nbut if that's a hold-up, I can try to summarize it (though I think\nreferring to the commit which _already_ did this and was accidentally\nreverted would be sufficient).\n\n strvec.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/strvec.h b/strvec.h\nindex fdcad75b45..6b3cbd6758 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -29,8 +29,8 @@ extern const char *empty_strvec[];\n */\n struct strvec {\n \tconst char **v;\n-\tint nr;\n-\tint alloc;\n+\tsize_t nr;\n+\tsize_t alloc;\n };\n \n #define STRVEC_INIT { empty_strvec, 0, 0 }\n-- \n2.33.0.811.g40f7f3a89c\n"},{"id":"435568","messageId":"87o88z82pc.fsf@evledraar.gmail.com","threadId":"56486","inReplyTo":"YTzEvLHWcfuD20x4@coredump.intra.peff.net","subject":"Re: [PATCH] strvec: use size_t to store nr and alloc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T16:13:18Z","receivedAt":"2021-09-11T17:03:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 11 2021, Jeff King wrote:\n\n> We converted argv_array (which later became strvec) to use size_t in\n> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n> order to avoid the possibility of integer overflow. But later, commit\n> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n> converted these back to ints!\n>\n> Those two commits were part of the same patch series. I'm pretty sure\n> what happened is that they were originally written in the opposite order\n> and then cleaned up and re-ordered during an interactive rebase. And\n> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n> patch completely, accidentally dropping the type change.\n>\n> We can correct it now; better late than never.\n>\n> Signed-off-by: Jeff King \n> ---\n> This was posted previously in the midst of another thread, but I don't\n> think was picked up. There was some positive reaction, but one \"do we\n> really need this?\" to which I responded in detail:\n>\n> https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n>\n> I don't really think any of that needs to go into the commit message,\n> but if that's a hold-up, I can try to summarize it (though I think\n> referring to the commit which _already_ did this and was accidentally\n> reverted would be sufficient).\n\nThanks, I have a WIP version of this outstanding starting with this\npatch that I was planning to submit sometime, but I'm happy to have you\npursue it, especially with the ~100 outstanding patches I have in\nmaster..seen.\n\nIt does feel somewhere between iffy and a landmine waiting to be stepped\non to only convert the member itself, and not any of the corresponding\n\"int\" variables that track it to \"size_t\".\n\nIf you do the change I suggested in\nhttps://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll\nfind that there's at least one first-order reference to this that now\nuses \"int\" that if converted to \"size_t\" will result in a wrap-around\nerror, we're lucky that one has a test failure.\n\nI can tell you what that bug is, but maybe it's better if you find it\nyourself :) I.e. I found *that* one, but I'm not sure I found them\nall. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler\nerrors & the code context (note: pointer, obviously broken, but makes\nthe compiler yell).\n\nThat particular bug will be caught by the compiler as it involves a >= 0\ncomparison against unsigned, but we may not not have that everywhere...\n"},{"id":"435603","messageId":"5e5e7fd9-83d7-87f7-b1ef-1292912b6c00@iee.email","threadId":"56486","inReplyTo":"87o88z82pc.fsf@evledraar.gmail.com","subject":"Re: [PATCH] strvec: use size_t to store nr and alloc","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-09-11T22:48:38Z","receivedAt":"2021-09-11T22:48:43Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 11/09/2021 17:13, Ævar Arnfjörð Bjarmason wrote:\n> On Sat, Sep 11 2021, Jeff King wrote:\n>\n>> We converted argv_array (which later became strvec) to use size_t in\n>> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n>> order to avoid the possibility of integer overflow. But later, commit\n>> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n>> converted these back to ints!\n>>\n>> Those two commits were part of the same patch series. I'm pretty sure\n>> what happened is that they were originally written in the opposite order\n>> and then cleaned up and re-ordered during an interactive rebase. And\n>> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n>> patch completely, accidentally dropping the type change.\n>>\n>> We can correct it now; better late than never.\n>>\n>> Signed-off-by: Jeff King \n>> ---\n>> This was posted previously in the midst of another thread, but I don't\n>> think was picked up. There was some positive reaction, but one \"do we\n>> really need this?\" to which I responded in detail:\n>>\n>> https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n>>\n>> I don't really think any of that needs to go into the commit message,\n>> but if that's a hold-up, I can try to summarize it (though I think\n>> referring to the commit which _already_ did this and was accidentally\n>> reverted would be sufficient).\n> Thanks, I have a WIP version of this outstanding starting with this\n> patch that I was planning to submit sometime, but I'm happy to have you\n> pursue it, especially with the ~100 outstanding patches I have in\n> master..seen.\n>\n> It does feel somewhere between iffy and a landmine waiting to be stepped\n> on to only convert the member itself, and not any of the corresponding\n> \"int\" variables that track it to \"size_t\".\n>\n> If you do the change I suggested in\n> https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll\n> find that there's at least one first-order reference to this that now\n> uses \"int\" that if converted to \"size_t\" will result in a wrap-around\n> error, we're lucky that one has a test failure.\n>\n> I can tell you what that bug is, but maybe it's better if you find it\n> yourself :) I.e. I found *that* one, but I'm not sure I found them\n> all. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler\n> errors & the code context (note: pointer, obviously broken, but makes\n> the compiler yell).\n>\n> That particular bug will be caught by the compiler as it involves a >= 0\n> comparison against unsigned, but we may not not have that everywhere...\n\nI'm particularly interested in the int -> size_t change problem as part\nof the wider 4GB limitations for the LLP64 systems [0] such as the\nRaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big\nproblem.\n\n\nPhilip\n\n\n[0]\nhttp://nickdesaulniers.github.io/blog/2016/05/30/data-models-and-word-size/\n[1] https://github.com/git-lfs/git-lfs/issues/2434  Git on Windows\nclient corrupts files > 4Gb\n[2] https://github.com/git-for-windows/git/pull/2179  [DRAFT] for\ntesting : Fix 4Gb limit for large files on Git for Windows\n"},{"id":"435606","messageId":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"5e5e7fd9-83d7-87f7-b1ef-1292912b6c00@iee.email","subject":"[PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:48Z","receivedAt":"2021-09-12T00:16:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is a proposed v2 of Jeff King's one-patch change to change\nstrvec's nr/alloc from \"int\" to \"size_t\". As noted below I think it's\nworthwhile to not only change that in the struct, but also in code\nthat directly references the \"nr\" member.\n\nOn Sat, Sep 11 2021, Philip Oakley wrote:\n\n> On 11/09/2021 17:13, Ævar Arnfjörð Bjarmason wrote:\n>> On Sat, Sep 11 2021, Jeff King wrote:\n>>\n>>> We converted argv_array (which later became strvec) to use size_t in\n>>> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n>>> order to avoid the possibility of integer overflow. But later, commit\n>>> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n>>> converted these back to ints!\n>>>\n>>> Those two commits were part of the same patch series. I'm pretty sure\n>>> what happened is that they were originally written in the opposite order\n>>> and then cleaned up and re-ordered during an interactive rebase. And\n>>> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n>>> patch completely, accidentally dropping the type change.\n>>>\n>>> We can correct it now; better late than never.\n>>>\n>>> Signed-off-by: Jeff King \n>>> ---\n>>> This was posted previously in the midst of another thread, but I don't\n>>> think was picked up. There was some positive reaction, but one \"do we\n>>> really need this?\" to which I responded in detail:\n>>>\n>>> https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n>>>\n>>> I don't really think any of that needs to go into the commit message,\n>>> but if that's a hold-up, I can try to summarize it (though I think\n>>> referring to the commit which _already_ did this and was accidentally\n>>> reverted would be sufficient).\n>> Thanks, I have a WIP version of this outstanding starting with this\n>> patch that I was planning to submit sometime, but I'm happy to have you\n>> pursue it, especially with the ~100 outstanding patches I have in\n>> master..seen.\n>>\n>> It does feel somewhere between iffy and a landmine waiting to be stepped\n>> on to only convert the member itself, and not any of the corresponding\n>> \"int\" variables that track it to \"size_t\".\n>>\n>> If you do the change I suggested in\n>> https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll\n>> find that there's at least one first-order reference to this that now\n>> uses \"int\" that if converted to \"size_t\" will result in a wrap-around\n>> error, we're lucky that one has a test failure.\n>>\n>> I can tell you what that bug is, but maybe it's better if you find it\n>> yourself :) I.e. I found *that* one, but I'm not sure I found them\n>> all. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler\n>> errors & the code context (note: pointer, obviously broken, but makes\n>> the compiler yell).\n>>\n>> That particular bug will be caught by the compiler as it involves a >= 0\n>> comparison against unsigned, but we may not not have that everywhere...\n>\n> I'm particularly interested in the int -> size_t change problem as part\n> of the wider 4GB limitations for the LLP64 systems [0] such as the\n> RaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big\n> problem.\n\nOkey, fine, no fun excercise for the reader then ;)\n\nThis is what I'd been sitting on locally since that recent thread, I\npolished it up a bit since Jeff King posted his version.\n\nThe potential overflow bug I mentioned is in rebase.c. See\n5/7. \"Potential\" because it's not a bug now, but that code\nintentionally considers a strvec, and then iterates it from nr-1 to 0,\nand if it reaches 0 intentionally counts down one more to -1 to\nindicate that it's visited all elements.\n\nWe then check that with i >= 0, except of course if it becomes\nunsigned that doesn't become -1, but rather it wraps around.\n\nThe rest of this is all changes to have that s/int/size_t/ radiate\noutwards, i.e. when we assign that value to a variable somewhere its\nnow a \"size_t\" instead of an \"int\" etc.\n\n> [0]\n> http://nickdesaulniers.github.io/blog/2016/05/30/data-models-and-word-size/\n> [1] https://github.com/git-lfs/git-lfs/issues/2434  Git on Windows\n> client corrupts files > 4Gb\n> [2] https://github.com/git-for-windows/git/pull/2179  [DRAFT] for\n> testing : Fix 4Gb limit for large files on Git for Windows\n\nJeff King (1):\n strvec: use size_t to store nr and alloc\n\nÆvar Arnfjörð Bjarmason (6):\n remote-curl: pass \"struct strvec *\" instead of int/char ** pair\n pack-objects: pass \"struct strvec *\" instead of int/char ** pair\n sequencer.[ch]: pass \"struct strvec *\" instead of int/char ** pair\n upload-pack.c: pass \"struct strvec *\" instead of int/char ** pair\n rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"\n strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"\n\n builtin/pack-objects.c | 6 +++---\n builtin/rebase.c | 26 ++++++++++++--------------\n connect.c | 8 ++++----\n fetch-pack.c | 4 ++--\n ls-refs.c | 2 +-\n remote-curl.c | 23 +++++++++++------------\n sequencer.c | 8 ++++----\n sequencer.h | 4 ++--\n serve.c | 2 +-\n shallow.c | 5 +++--\n shallow.h | 6 ++++--\n strvec.h | 4 ++--\n submodule.c | 2 +-\n upload-pack.c | 7 +++----\n 14 files changed, 53 insertions(+), 54 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 2ef48d734e8 remote-curl: pass \"struct strvec *\" instead of int/char ** pair\n-: ----------- > 2: 7f59a58ed97 pack-objects: pass \"struct strvec *\" instead of int/char ** pair\n-: ----------- > 3: c35cfb9c9c5 sequencer.[ch]: pass \"struct strvec *\" instead of int/char ** pair\n-: ----------- > 4: 2e0b82d4316 upload-pack.c: pass \"struct strvec *\" instead of int/char ** pair\n-: ----------- > 5: be85a0565ef rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"\n1: 498f5ed80dc ! 6: ba17290852c strvec: use size_t to store nr and alloc\n @@ Commit message\n We can correct it now; better late than never.\n \n Signed-off-by: Jeff King \n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## strvec.h ##\n @@ strvec.h: extern const char *empty_strvec[];\n-: ----------- > 7: 2edd9708888 strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435607","messageId":"patch-v2-1.7-2ef48d734e8-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 1/7] remote-curl: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:49Z","receivedAt":"2021-09-12T00:16:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We'll be changing the \"int nr\" in the \"struct strvec\" to \"int size_t\"\nsoon, i.e. from signed to unsigned. It will then make sense to use the\n\"size_t\" type for things that mirror that \"nr\" member.\n\nThis sets up that change in remote-curl.c, we'll change the \"int i\"\nhere to a \"size_t i\" in a later commit (let's also split and \"err\" up\nto make that change smaller).\n\nIn this case the push() function can just pass the \"struct strvec\" it\nhas down to push_dav() and push_git(), in addition make use of\nstrvec_pushv() in push_dav() instead of looping over the the specs\nourselves. So we can get rid of much of the need for tracking the\n\"nr\", which in the case of push_dav() was already a \"size_t\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n remote-curl.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 598cff7cde6..1f177e86f11 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1266,10 +1266,9 @@ static void parse_fetch(struct strbuf *buf)\n \tstrbuf_reset(buf);\n }\n \n-static int push_dav(int nr_spec, const char **specs)\n+static int push_dav(struct strvec *specs)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tsize_t i;\n \n \tchild.git_cmd = 1;\n \tstrvec_push(&child.args, \"http-push\");\n@@ -1279,18 +1278,18 @@ static int push_dav(int nr_spec, const char **specs)\n \tif (options.verbosity > 1)\n \t\tstrvec_push(&child.args, \"--verbose\");\n \tstrvec_push(&child.args, url.buf);\n-\tfor (i = 0; i < nr_spec; i++)\n-\t\tstrvec_push(&child.args, specs[i]);\n+\tstrvec_pushv(&child.args, specs->v);\n \n \tif (run_command(&child))\n \t\tdie(_(\"git-http-push failed\"));\n \treturn 0;\n }\n \n-static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n+static int push_git(struct discovery *heads, struct strvec *specs)\n {\n \tstruct rpc_state rpc;\n-\tint i, err;\n+\tint i;\n+\tint err;\n \tstruct strvec args;\n \tstruct string_list_item *cas_option;\n \tstruct strbuf preamble = STRBUF_INIT;\n@@ -1326,8 +1325,8 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \t\tstrvec_push(&args, \"--force-if-includes\");\n \n \tstrvec_push(&args, \"--stdin\");\n-\tfor (i = 0; i < nr_spec; i++)\n-\t\tpacket_buf_write(&preamble, \"%s\\n\", specs[i]);\n+\tfor (i = 0; i < specs->nr; i++)\n+\t\tpacket_buf_write(&preamble, \"%s\\n\", specs->v[i]);\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n@@ -1342,15 +1341,15 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \treturn err;\n }\n \n-static int push(int nr_spec, const char **specs)\n+static int push(struct strvec *specs)\n {\n \tstruct discovery *heads = discover_refs(\"git-receive-pack\", 1);\n \tint ret;\n \n \tif (heads->proto_git)\n-\t\tret = push_git(heads, nr_spec, specs);\n+\t\tret = push_git(heads, specs);\n \telse\n-\t\tret = push_dav(nr_spec, specs);\n+\t\tret = push_dav(specs);\n \tfree_discovery(heads);\n \treturn ret;\n }\n@@ -1374,7 +1373,7 @@ static void parse_push(struct strbuf *buf)\n \t\t\tbreak;\n \t} while (1);\n \n-\tret = push(specs.nr, specs.v);\n+\tret = push(&specs);\n \tprintf(\"\\n\");\n \tfflush(stdout);\n \n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435608","messageId":"patch-v2-2.7-7f59a58ed97-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 2/7] pack-objects: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:50Z","receivedAt":"2021-09-12T00:16:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As in the preceding commit, prepare for the \"nr\" member of \"struct\nstrvec\" changing from an \"int\" to a \"size_t\".\n\nIn this case we end up passing the \"nr\" to setup_revisions(), which\nexpects to take an argc/argv pair passed to main(), so we can't change\nits type.\n\nBut we can defer the conversion until that point. Let's continue to\npass the \"struct strvec *\" down into instead of \"unwrapping\" it in\ncases where the only source of the \"int/char **\" pair is a \"struct\nstrvec\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ec8503563a6..1902c0b6776 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3701,7 +3701,7 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n-static void get_object_list(int ac, const char **av)\n+static void get_object_list(struct strvec *rp)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n@@ -3713,7 +3713,7 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\tsetup_revisions(rp->nr, rp->v, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n@@ -4138,7 +4138,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t} else if (!use_internal_rev_list)\n \t\tread_object_list_from_stdin();\n \telse {\n-\t\tget_object_list(rp.nr, rp.v);\n+\t\tget_object_list(&rp);\n \t\tstrvec_clear(&rp);\n \t}\n \tcleanup_preferred_base();\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435609","messageId":"patch-v2-3.7-c35cfb9c9c5-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 3/7] sequencer.[ch]: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:51Z","receivedAt":"2021-09-12T00:16:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As in the preceding commit, prepare for the \"nr\" member of \"struct\nstrvec\" changing from an \"int\" to a \"size_t\". These are the same sorts\nof changes to pass a \"struct strvec *\" further down instead of passing\nargs->nr and args->v.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rebase.c | 3 +--\n sequencer.c | 6 +++---\n sequencer.h | 4 ++--\n 3 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex eb01f4d790b..27669880918 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -363,8 +363,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n \n \tret = sequencer_make_script(the_repository, &todo_list.buf,\n-\t\t\t\t make_script_args.nr, make_script_args.v,\n-\t\t\t\t flags);\n+\t\t\t\t &make_script_args, flags);\n \n \tif (ret)\n \t\terror(_(\"could not generate todo list\"));\ndiff --git a/sequencer.c b/sequencer.c\nindex 68316636921..a4ba434f173 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5355,8 +5355,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \treturn 0;\n }\n \n-int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n-\t\t\t const char **argv, unsigned flags)\n+int sequencer_make_script(struct repository *r, struct strbuf *out,\n+\t\t\t struct strvec *args, unsigned flags)\n {\n \tchar *format = NULL;\n \tstruct pretty_print_context pp = {0};\n@@ -5390,7 +5390,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tpp.fmt = revs.commit_format;\n \tpp.output_encoding = get_log_output_encoding();\n \n-\tif (setup_revisions(argc, argv, &revs, NULL) > 1)\n+\tif (setup_revisions(args->nr, args->v, &revs, NULL) > 1)\n \t\treturn error(_(\"make_script: unhandled options\"));\n \n \tif (prepare_revision_walk(&revs) < 0)\ndiff --git a/sequencer.h b/sequencer.h\nindex 2088344cb37..36f67164f86 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -158,8 +158,8 @@ int sequencer_remove_state(struct replay_opts *opts);\n #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)\n #define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)\n \n-int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n-\t\t\t const char **argv, unsigned flags);\n+int sequencer_make_script(struct repository *r, struct strbuf *out,\n+\t\t\t struct strvec *args, unsigned flags);\n \n void todo_list_add_exec_commands(struct todo_list *todo_list,\n \t\t\t\t struct string_list *commands);\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435610","messageId":"patch-v2-4.7-2e0b82d4316-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 4/7] upload-pack.c: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:52Z","receivedAt":"2021-09-12T00:16:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As in the preceding commit, prepare for the \"nr\" member of \"struct\nstrvec\" changing from an \"int\" to a \"size_t\". These are the same sorts\nof changes to pass a \"struct strvec *\" further down instead of passing\nargs->nr and args->v.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n shallow.c | 5 +++--\n shallow.h | 6 ++++--\n upload-pack.c | 7 +++----\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex 9ed18eb8849..6928db36669 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -15,6 +15,7 @@\n #include \"list-objects.h\"\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n+#include \"strvec.h\"\n \n void set_alternate_shallow_file(struct repository *r, const char *path, int override)\n {\n@@ -196,7 +197,7 @@ static void show_commit(struct commit *commit, void *data)\n * are marked with shallow_flag. The list of border/shallow commits\n * are also returned.\n */\n-struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n+struct commit_list *get_shallow_commits_by_rev_list(struct strvec *args,\n \t\t\t\t\t\t int shallow_flag,\n \t\t\t\t\t\t int not_shallow_flag)\n {\n@@ -215,7 +216,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, NULL);\n+\tsetup_revisions(args->nr, args->v, &revs, NULL);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\ndiff --git a/shallow.h b/shallow.h\nindex 5b4a96dcd69..206405ec8ca 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -5,6 +5,7 @@\n #include \"object.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"strvec.h\"\n \n void set_alternate_shallow_file(struct repository *r, const char *path, int override);\n int register_shallow(struct repository *r, const struct object_id *oid);\n@@ -32,8 +33,9 @@ void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);\n \n struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n-struct commit_list *get_shallow_commits_by_rev_list(\n-\t\tint ac, const char **av, int shallow_flag, int not_shallow_flag);\n+struct commit_list *get_shallow_commits_by_rev_list(struct strvec *args,\n+\t\t\t\t\t\t int shallow_flag,\n+\t\t\t\t\t\t int not_shallow_flag);\n int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \t\t\t const struct oid_array *extra);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6ce07231d3d..5928973bcc3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -859,13 +859,12 @@ static void deepen(struct upload_pack_data *data, int depth)\n }\n \n static void deepen_by_rev_list(struct upload_pack_data *data,\n-\t\t\t int ac,\n-\t\t\t const char **av)\n+\t\t\t struct strvec *args)\n {\n \tstruct commit_list *result;\n \n \tdisable_commit_graph(the_repository);\n-\tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n+\tresult = get_shallow_commits_by_rev_list(args, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(data, result);\n \tfree_commit_list(result);\n \tsend_unshallow(data);\n@@ -900,7 +899,7 @@ static int send_shallow_list(struct upload_pack_data *data)\n \t\t\tstruct object *o = data->want_obj.objects[i].item;\n \t\t\tstrvec_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(data, av.nr, av.v);\n+\t\tdeepen_by_rev_list(data, &av);\n \t\tstrvec_clear(&av);\n \t\tret = 1;\n \t} else {\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435611","messageId":"patch-v2-5.7-be85a0565ef-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 5/7] rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:53Z","receivedAt":"2021-09-12T00:16:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As in the preceding commit, prepare for the \"nr\" member of \"struct\nstrvec\" changing from an \"int\" to a \"size_t\".\n\nLet's change this code added in f57696802c3 (rebase: really just\npassthru the `git am` options, 2018-11-14) so that it won't cause a\nbug if the \"i\" variable here is updated to be a \"size_t\" instead of an\n\"int\".\n\nThe reason it would be buggy is because this for-loop relied on\n\"counting down\" from nr-1 to 0, we'll then decrement the variable one\nlast time, so a value of -1 indicates that we've visited all\nelements. Since we're looking for a needle in the haystack having\nlooked at all the hay means that the needle isn't there.\n\nLet's replace this with simpler code that loops overthe \"struct\nstrvec\" and checks the current needle is \"-q\", if it isn't and we're\ndoing a merge we die, otherwise we do a \"REBASE_APPLY\".\n\nWe've been able to simplify this code since 8295ed690bf (rebase: make\nthe backend configurable via config setting, 2020-02-15) when we\nstopped using this for anything except this one check, so let's do\nthat and move the check into the loop itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rebase.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 27669880918..dcd897fda9c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1750,16 +1750,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.git_am_opts.nr || options.type == REBASE_APPLY) {\n \t\t/* all am options except -q are compatible only with --apply */\n-\t\tfor (i = options.git_am_opts.nr - 1; i >= 0; i--)\n-\t\t\tif (strcmp(options.git_am_opts.v[i], \"-q\"))\n-\t\t\t\tbreak;\n-\n-\t\tif (i >= 0) {\n-\t\t\tif (is_merge(&options))\n-\t\t\t\tdie(_(\"cannot combine apply options with \"\n-\t\t\t\t \"merge options\"));\n-\t\t\telse\n+\t\tfor (i = 0; i < options.git_am_opts.nr; i++) {\n+\t\t\tif (strcmp(options.git_am_opts.v[i], \"-q\")) {\n+\t\t\t\tif (is_merge(&options))\n+\t\t\t\t\tdie(_(\"cannot combine apply options with \"\n+\t\t\t\t\t \"merge options\"));\n \t\t\t\toptions.type = REBASE_APPLY;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435612","messageId":"patch-v2-6.7-ba17290852c-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 6/7] strvec: use size_t to store nr and alloc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:54Z","receivedAt":"2021-09-12T00:16:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nWe converted argv_array (which later became strvec) to use size_t in\n819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\norder to avoid the possibility of integer overflow. But later, commit\nd70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\nconverted these back to ints!\n\nThose two commits were part of the same patch series. I'm pretty sure\nwhat happened is that they were originally written in the opposite order\nand then cleaned up and re-ordered during an interactive rebase. And\nwhen resolving the inevitable conflict, I mistakenly took the \"rename\"\npatch completely, accidentally dropping the type change.\n\nWe can correct it now; better late than never.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n strvec.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/strvec.h b/strvec.h\nindex fdcad75b45b..6b3cbd67589 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -29,8 +29,8 @@ extern const char *empty_strvec[];\n */\n struct strvec {\n \tconst char **v;\n-\tint nr;\n-\tint alloc;\n+\tsize_t nr;\n+\tsize_t alloc;\n };\n \n #define STRVEC_INIT { empty_strvec, 0, 0 }\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435613","messageId":"patch-v2-7.7-2edd9708888-20210912T001420Z-avarab@gmail.com","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"[PATCH v2 7/7] strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-12T00:15:55Z","receivedAt":"2021-09-12T00:16:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Now that the strvec.h \"int nr\" has been changed to \"size_t nr\" in the\npreceding commit change various users of the API so that their local\ntracking (usually for-loop iteration) of the number of items in the\narray uses \"size_t\" as well.\n\nThese were found by changing the \"nr\" member to a pointer temporarily,\nand manually looking at the codepaths that the compiler complained\nabout.\n\nAs argued in [1] these\nchanges are not strictly necessary as a follow-up, but let's do them\nanyway so we don't need to wonder about the \"size_t\" v.s. \"int\"\ninconsistency going forward.\n\nAs noted in <87v93i8svd.fsf@evledraar.gmail.com> in that thread we\nhave various things that interact with the \"int argc\" passed from\nmain() (e.g. setup_revisions()) and \"struct strvec\". Those things\ncould consistently use \"int\" before, but will now use some mixture of\n\"int\" and \"size_t\". That's OK, but is the reason we're not converting\nall API users to use \"size_t\" here for their own copies of \"nr\", or\nwhen they pass that \"nr\" to other functions.\n\n1. https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n2. https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rebase.c | 7 ++++---\n connect.c | 8 ++++----\n fetch-pack.c | 4 ++--\n ls-refs.c | 2 +-\n remote-curl.c | 2 +-\n sequencer.c | 2 +-\n serve.c | 2 +-\n submodule.c | 2 +-\n 8 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex dcd897fda9c..b96faaa7fea 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1424,7 +1424,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"apply all changes, even those already present upstream\")),\n \t\tOPT_END(),\n \t};\n-\tint i;\n+\tsize_t i;\n+\tunsigned int j;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_rebase_usage,\n@@ -1654,8 +1655,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tfor (i = 0; i < exec.nr; i++)\n-\t\tif (check_exec_cmd(exec.items[i].string))\n+\tfor (j = 0; j < exec.nr; j++)\n+\t\tif (check_exec_cmd(exec.items[j].string))\n \t\t\texit(1);\n \n \tif (!(options.flags & REBASE_NO_QUIET))\ndiff --git a/connect.c b/connect.c\nindex aff13a270e6..a54d4aa4d90 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -68,7 +68,7 @@ static NORETURN void die_initial_contact(int unexpected)\n /* Checks if the server supports the capability 'c' */\n int server_supports_v2(const char *c, int die_on_error)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -85,7 +85,7 @@ int server_supports_v2(const char *c, int die_on_error)\n \n int server_feature_v2(const char *c, const char **v)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -101,7 +101,7 @@ int server_feature_v2(const char *c, const char **v)\n int server_supports_feature(const char *c, const char *feature,\n \t\t\t int die_on_error)\n {\n-\tint i;\n+\tsize_t i;\n \n \tfor (i = 0; i < server_capabilities_v2.nr; i++) {\n \t\tconst char *out;\n@@ -479,7 +479,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t\t const struct string_list *server_options,\n \t\t\t int stateless_rpc)\n {\n-\tint i;\n+\tsize_t i;\n \tconst char *hash_name;\n \tstruct strvec *ref_prefixes = transport_options ?\n \t\t&transport_options->ref_prefixes : NULL;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0bf7ed7e477..d20f9a046f7 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -945,7 +945,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t}\n \n \tif (index_pack_args) {\n-\t\tint i;\n+\t\tsize_t i;\n \n \t\tfor (i = 0; i < cmd.args.nr; i++)\n \t\t\tstrvec_push(index_pack_args, cmd.args.v[i]);\n@@ -1674,7 +1674,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t}\n \n \tfor (i = 0; i < packfile_uris.nr; i++) {\n-\t\tint j;\n+\t\tsize_t j;\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \t\tchar packname[GIT_MAX_HEXSZ + 1];\n \t\tconst char *uri = packfile_uris.items[i].string +\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 84021416ca5..9622bc46072 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -46,7 +46,7 @@ static void ensure_config_read(void)\n */\n static int ref_match(const struct strvec *prefixes, const char *refname)\n {\n-\tint i;\n+\tsize_t i;\n \n \tif (!prefixes->nr)\n \t\treturn 1; /* no restriction */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1f177e86f11..682643bb842 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1288,7 +1288,7 @@ static int push_dav(struct strvec *specs)\n static int push_git(struct discovery *heads, struct strvec *specs)\n {\n \tstruct rpc_state rpc;\n-\tint i;\n+\tsize_t i;\n \tint err;\n \tstruct strvec args;\n \tstruct string_list_item *cas_option;\ndiff --git a/sequencer.c b/sequencer.c\nindex a4ba434f173..ca3ee2e4167 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -919,7 +919,7 @@ static char *get_author(const char *message)\n \n static const char *author_date_from_env_array(const struct strvec *env)\n {\n-\tint i;\n+\tsize_t i;\n \tconst char *date;\n \n \tfor (i = 0; i < env->nr; i++)\ndiff --git a/serve.c b/serve.c\nindex f11c0e07c45..0271bf946cc 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -159,7 +159,7 @@ static int is_command(const char *key, struct protocol_capability **command)\n int has_capability(const struct strvec *keys, const char *capability,\n \t\t const char **value)\n {\n-\tint i;\n+\tsize_t i;\n \tfor (i = 0; i < keys->nr; i++) {\n \t\tconst char *out;\n \t\tif (skip_prefix(keys->v[i], capability, &out) &&\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1dbf..38ee56782c7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1606,7 +1606,7 @@ int fetch_populated_submodules(struct repository *r,\n \t\t\t int default_option,\n \t\t\t int quiet, int max_parallel_jobs)\n {\n-\tint i;\n+\tsize_t i;\n \tstruct submodule_parallel_fetch spf = SPF_INIT;\n \n \tspf.r = r;\n-- \n2.33.0.998.ga4d44345d43\n\n"},{"id":"435615","messageId":"CAPUEspizWWigu4yb_fK1kmzJDx7G0JUOFmRsAfaKeuvTPrk4bA@mail.gmail.com","threadId":"56486","inReplyTo":"patch-v2-1.7-2ef48d734e8-20210912T001420Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/7] remote-curl: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-12T00:36:19Z","receivedAt":"2021-09-12T00:36:36Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Sep 11, 2021 at 5:16 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> We'll be changing the \"int nr\" in the \"struct strvec\" to \"int size_t\"\n\nDid you mean \"size_t nr\"?\n\n> -static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n> +static int push_git(struct discovery *heads, struct strvec *specs)\n> {\n> struct rpc_state rpc;\n> - int i, err;\n> + int i;\n\nyou are already splitting them, why not then change 'i' also to be a size_t?\n\nCarlo\n"},{"id":"435621","messageId":"CAPig+cTWjqCiW=M37sf41F7-M0U=fJdTgtM_YqdPTqS53rFHHg@mail.gmail.com","threadId":"56486","inReplyTo":"patch-v2-5.7-be85a0565ef-20210912T001420Z-avarab@gmail.com","subject":"Re: [PATCH v2 5/7] rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-12T02:57:36Z","receivedAt":"2021-09-12T02:57:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 11, 2021 at 8:16 PM Ævar Arnfjörð Bjarmason\n wrote:\n> As in the preceding commit, prepare for the \"nr\" member of \"struct\n> strvec\" changing from an \"int\" to a \"size_t\".\n>\n> Let's change this code added in f57696802c3 (rebase: really just\n> passthru the `git am` options, 2018-11-14) so that it won't cause a\n> bug if the \"i\" variable here is updated to be a \"size_t\" instead of an\n> \"int\".\n>\n> The reason it would be buggy is because this for-loop relied on\n> \"counting down\" from nr-1 to 0, we'll then decrement the variable one\n> last time, so a value of -1 indicates that we've visited all\n> elements. Since we're looking for a needle in the haystack having\n> looked at all the hay means that the needle isn't there.\n\ns/haystack/&,/\n\n> Let's replace this with simpler code that loops overthe \"struct\n> strvec\" and checks the current needle is \"-q\", if it isn't and we're\n> doing a merge we die, otherwise we do a \"REBASE_APPLY\".\n\ns/overthe/over the/\n\nNit: comma-splice[1] at `\"-q\",`; replace comma with semicolon or period.\n\n[1]: https://lore.kernel.org/git/CAPx1GvfFPWvJsj+uJV7RZrv1rgEpio=pk6rKF2UrjHebVY=LPA@mail.gmail.com/\n\n> We've been able to simplify this code since 8295ed690bf (rebase: make\n> the backend configurable via config setting, 2020-02-15) when we\n> stopped using this for anything except this one check, so let's do\n> that and move the check into the loop itself.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"435622","messageId":"CAPig+cTdisiKU-8i3kRuDqtwy6eQOA+jNbz8WGm627ojUwkgbA@mail.gmail.com","threadId":"56486","inReplyTo":"patch-v2-7.7-2edd9708888-20210912T001420Z-avarab@gmail.com","subject":"Re: [PATCH v2 7/7] strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-12T03:00:36Z","receivedAt":"2021-09-12T03:00:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 11, 2021 at 8:16 PM Ævar Arnfjörð Bjarmason\n wrote:\n> Now that the strvec.h \"int nr\" has been changed to \"size_t nr\" in the\n> preceding commit change various users of the API so that their local\n> tracking (usually for-loop iteration) of the number of items in the\n> array uses \"size_t\" as well.\n\ns/change/changed/\n\n> These were found by changing the \"nr\" member to a pointer temporarily,\n> and manually looking at the codepaths that the compiler complained\n> about.\n>\n> As argued in [1] these\n> changes are not strictly necessary as a follow-up, but let's do them\n> anyway so we don't need to wonder about the \"size_t\" v.s. \"int\"\n> inconsistency going forward.\n>\n> As noted in <87v93i8svd.fsf@evledraar.gmail.com> in that thread we\n\ns/>/>[2]/\n\n> have various things that interact with the \"int argc\" passed from\n> main() (e.g. setup_revisions()) and \"struct strvec\". Those things\n> could consistently use \"int\" before, but will now use some mixture of\n> \"int\" and \"size_t\". That's OK, but is the reason we're not converting\n> all API users to use \"size_t\" here for their own copies of \"nr\", or\n> when they pass that \"nr\" to other functions.\n>\n> 1. https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n> 2. https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"435658","messageId":"YT54CNYgtGcqexwq@coredump.intra.peff.net","threadId":"56486","inReplyTo":"87o88z82pc.fsf@evledraar.gmail.com","subject":"Re: [PATCH] strvec: use size_t to store nr and alloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T21:58:32Z","receivedAt":"2021-09-12T21:58:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 06:13:18PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I don't really think any of that needs to go into the commit message,\n> > but if that's a hold-up, I can try to summarize it (though I think\n> > referring to the commit which _already_ did this and was accidentally\n> > reverted would be sufficient).\n> \n> Thanks, I have a WIP version of this outstanding starting with this\n> patch that I was planning to submit sometime, but I'm happy to have you\n> pursue it, especially with the ~100 outstanding patches I have in\n> master..seen.\n> \n> It does feel somewhere between iffy and a landmine waiting to be stepped\n> on to only convert the member itself, and not any of the corresponding\n> \"int\" variables that track it to \"size_t\".\n\nI don't think it's a landmine. If those other places are using \"int\" and\nhave trouble with a too-big strvec after the patch, then they were\ngenerally already buggy before, too. I have poked around before for\ncases where half-converting some code from size_t to int can possibly\nmake things worse, but it's pretty rare.\n\nMeanwhile, strvec using an int has obvious and easy-to-trigger overflow\nproblems. Try this:\n\n yes '000aagent' | GIT_PROTOCOL=version=2 ./git-upload-pack .\n\nwhere we will happily allocate an arbitrarily-large strvec. If you have\nthe patience and the RAM, this will overflow the alloc field. Luckily we\ncatch it before under-allocating the array. Because our ALLOC_GROW()\ngrowth pattern is fixed, we'll always end up with a negative 32-bit int,\nwhich gets cast to a very large size_t, and an st_mult() invocation\ncomplains.\n\nMuch scarier to me is that string_list seems to be using \"unsigned int\"\nfor its counter, so it never goes negative! Try this:\n\n yes | git pack-objects --stdin-packs foo\n\nwhich reads into a string_list. It takes even more RAM to overflow\nbecause string_list is so horribly inefficient. But here once again\nwe're saved by a quirk of ALLOC_GROW() dating back to 4234a76167 (Extend\n--pretty=oneline to cover the first paragraph,, 2007-06-11).\n\nalloc_nr() will overflow to a small number when it hits around 2^32 / 3.\nBefore that patch, we'd then realloc a too-small buffer and have a heap\noverflow. After that patch (I think in order to catch growth by more\nthan 1 element), we notice the case that alloc_nr() didn't give us a big\nenough size, and extend it exactly to what was asked for. So no heap\noverflow, though we do enter a quadratic growth loop. :-/\n\nSo I don't think we have any heap overflows here, which is good. But we\nare protected by a fair bit of luck, and none of this would get nearly\nas close to danger if we just used a sensible type for our allocation\nsizes.\n\nI do think we should separately fix the v2 protocol not to just allocate\narbitrary-sized strvecs on behalf of the user. I took a stab at that\nthis weekend and have a series I'm pretty happy with, but of course it\nconflicts with your ab/serve-cleanup (and in fact, I ended up doing some\nof those same cleanups, like not passing \"keys\" around). I'll see if I\ncan re-build it on top.\n\n> If you do the change I suggested in\n> https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll\n> find that there's at least one first-order reference to this that now\n> uses \"int\" that if converted to \"size_t\" will result in a wrap-around\n> error, we're lucky that one has a test failure.\n> \n> I can tell you what that bug is, but maybe it's better if you find it\n> yourself :) I.e. I found *that* one, but I'm not sure I found them\n> all. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler\n> errors & the code context (note: pointer, obviously broken, but makes\n> the compiler yell).\n\nYes, having converted s/int/size_t/ for other structures before, you\nhave to be very careful of signedness. A safer conversion is ssize_t\n(which similarly avoids overflow problems on LP64 systems).\n\nI'm sure there are other overflow bugs lurking around use of strvecs, if\nyou really push them hard. But I care a lot less about something like\n\"oops, I accidentally overwrite list[0] instead of list[2^32-1]\". Those\nare bugs that normal people will never see, because they aren't doing\nstupid or malicious things. I care a lot more about our allocating\nfunctions being vulnerable to heap overflows from malicious attackers.\n\nSo I was really hoping to do the size_t fix separately. It's hard for it\nto screw anything up, and it addresses the most vulnerable and important\nuse.\n\n-Peff\n"},{"id":"435659","messageId":"YT54mskEIir2gXc2@coredump.intra.peff.net","threadId":"56486","inReplyTo":"5e5e7fd9-83d7-87f7-b1ef-1292912b6c00@iee.email","subject":"Re: [PATCH] strvec: use size_t to store nr and alloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T22:00:58Z","receivedAt":"2021-09-12T22:01:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 11:48:38PM +0100, Philip Oakley wrote:\n\n> I'm particularly interested in the int -> size_t change problem as part\n> of the wider 4GB limitations for the LLP64 systems [0] such as the\n> RaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big\n> problem.\n\nNote that a lot of the Windows LLP64 problems are really a separate\nissue. They come from a misuse of \"unsigned long\" as \"gee, this should\nbe big enough for anything\". Most of that is due to its use for object\nsizes, which of course infected a whole bunch of other code.\n\nWhich isn't to say it's not important. But my main goal here was making\nsure we use size_t for growth allocations to avoid integer overflow\nleading to under-allocation (and thus heap overflow).\n\n-Peff\n"},{"id":"435662","messageId":"YT586/CO7QsTb3TK@coredump.intra.peff.net","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-12T22:19:23Z","receivedAt":"2021-09-12T22:19:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 12, 2021 at 02:15:48AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This is what I'd been sitting on locally since that recent thread, I\n> polished it up a bit since Jeff King posted his version.\n> \n> The potential overflow bug I mentioned is in rebase.c. See\n> 5/7. \"Potential\" because it's not a bug now, but that code\n> intentionally considers a strvec, and then iterates it from nr-1 to 0,\n> and if it reaches 0 intentionally counts down one more to -1 to\n> indicate that it's visited all elements.\n> \n> We then check that with i >= 0, except of course if it becomes\n> unsigned that doesn't become -1, but rather it wraps around.\n\nYou can also just use ssize_t, or you can compare against SIZE_MAX to\ncatch the wraparound (there's some prior art in sort_revindex()). That\nsaid, I don't mind rewriting loops to count up rather than down. It\nusually makes them easier to follow (and in your patch 5, I do not see\nany reason we would need to count down rather than up; we do not even\ncare where we find \"-q\", only that we found it.\n\n> The rest of this is all changes to have that s/int/size_t/ radiate\n> outwards, i.e. when we assign that value to a variable somewhere its\n> now a \"size_t\" instead of an \"int\" etc.\n\nI'm a little \"meh\" on some of these, for a few reasons:\n\n - anything calling into setup_revisions() eventually is just kicking\n the can anyway. And these are generally not buggy in the first place,\n since they're bounded argv creations.\n\n - passing a strvec instead of the broken-down pair is a less flexible\n interface. It's one thing if the callee benefits from seeing the\n strvec (say, because they may push more items onto it). But I think\n with strbufs, we have a general guideline that if a function _can_\n take the bare pointer, then it should. (Sorry, I don't have a\n succinct reference to CodingGuidelines or anything like that; I feel\n like this is wisdom we came up with on the list in the early days of\n strbufs).\n\n - if we are going to pass a strvec, it should almost certainly be\n const, to make it clear how we intend to use it.\n\nSo if we we wanted to try to reduce the int/size_t conversions here (and\nI don't mind doing it, but am not altogether sure it is a good use of\ntime, because the rabbit hole runs deep), I think we ought to be\nswitching to size_t everywhere-ish along whole call chains. Or possibly\nproviding a checked size_to_int() which will safely catch and abort.\nThese cases are largely stupid things that real people would never come\nacross. The real goal is making sure we don't get hit with a memory\nsafety bug (under-allocation, converting a big size_t to a negative int,\netc).\n\n-Peff\n"},{"id":"435688","messageId":"874kap5dqp.fsf@evledraar.gmail.com","threadId":"56486","inReplyTo":"CAPUEspizWWigu4yb_fK1kmzJDx7G0JUOFmRsAfaKeuvTPrk4bA@mail.gmail.com","subject":"Re: [PATCH v2 1/7] remote-curl: pass \"struct strvec *\" instead of int/char ** pair","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T03:56:45Z","receivedAt":"2021-09-13T03:57:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 11 2021, Carlo Arenas wrote:\n\n> On Sat, Sep 11, 2021 at 5:16 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> We'll be changing the \"int nr\" in the \"struct strvec\" to \"int size_t\"\n>\n> Did you mean \"size_t nr\"?\n\nYes, will fix.\n\n>> -static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n>> +static int push_git(struct discovery *heads, struct strvec *specs)\n>> {\n>> struct rpc_state rpc;\n>> - int i, err;\n>> + int i;\n>\n> you are already splitting them, why not then change 'i' also to be a size_t?\n\nThe conversion to size_t comes later in this series, this is setting\nthat diff up to be smaller. I.e. we'll then to s/int i/size_t /g.\n"},{"id":"435692","messageId":"xmqqlf41ghmk.fsf@gitster.g","threadId":"56486","inReplyTo":"YT586/CO7QsTb3TK@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T05:38:11Z","receivedAt":"2021-09-13T05:38:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm a little \"meh\" on some of these, for a few reasons:\n>\n> - anything calling into setup_revisions() eventually is just kicking\n> the can anyway. And these are generally not buggy in the first place,\n> since they're bounded argv creations.\n>\n> - passing a strvec instead of the broken-down pair is a less flexible\n> interface. It's one thing if the callee benefits from seeing the\n> strvec (say, because they may push more items onto it). But I think\n> with strbufs, we have a general guideline that if a function _can_\n> take the bare pointer, then it should. (Sorry, I don't have a\n> succinct reference to CodingGuidelines or anything like that; I feel\n> like this is wisdom we came up with on the list in the early days of\n> strbufs).\n>\n> - if we are going to pass a strvec, it should almost certainly be\n> const, to make it clear how we intend to use it.\n\nAll true.\n\n> These cases are largely stupid things that real people would never come\n> across. The real goal is making sure we don't get hit with a memory\n> safety bug (under-allocation, converting a big size_t to a negative int,\n> etc).\n\nYes. \n\nÆvar, I do not mean any disrespect to you, but I have to say that\ntopics like this one are starting to wear my concentration and\npatience down really thin and making me really slow down.\n\nPerhaps I am biased by not yet having seen what you eventually want\nto build on top, and because of that I do not understood why and how\nthese \"clean ups\" are so valuable to have right now (as opposed to\njust letting the sleeping dog lie), which you would of course have a\nmuch better chance to know than I do.\n\nBut with that \"bias\", many of the recent patches from you look more\nlike pointless churn, mixed with fixes to theoretical problems, than\nclean-ups with real benefits.\n\nWhat makes it worse is that there are occasional real gems among\nthese \"meh\" patches, which means I have to read all of them anyway\nto sift wheat from chaff X-<.\n"},{"id":"435702","messageId":"cdf72de4-4879-c4b9-e882-2e134c6ba45f@iee.email","threadId":"56486","inReplyTo":"cover-v2-0.7-00000000000-20210912T001420Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-09-13T10:47:12Z","receivedAt":"2021-09-13T10:47:18Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 12/09/2021 01:15, Ævar Arnfjörð Bjarmason wrote:\n> This is a proposed v2 of Jeff King's one-patch change to change\n> strvec's nr/alloc from \"int\" to \"size_t\". As noted below I think it's\n> worthwhile to not only change that in the struct, but also in code\n> that directly references the \"nr\" member.\n>\n> On Sat, Sep 11 2021, Philip Oakley wrote:\n>\n>> On 11/09/2021 17:13, Ævar Arnfjörð Bjarmason wrote:\n>>> On Sat, Sep 11 2021, Jeff King wrote:\n>>>\n>>>> We converted argv_array (which later became strvec) to use size_t in\n>>>> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in\n>>>> order to avoid the possibility of integer overflow. But later, commit\n>>>> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally\n>>>> converted these back to ints!\n>>>>\n>>>> Those two commits were part of the same patch series. I'm pretty sure\n>>>> what happened is that they were originally written in the opposite order\n>>>> and then cleaned up and re-ordered during an interactive rebase. And\n>>>> when resolving the inevitable conflict, I mistakenly took the \"rename\"\n>>>> patch completely, accidentally dropping the type change.\n>>>>\n>>>> We can correct it now; better late than never.\n>>>>\n>>>> Signed-off-by: Jeff King \n>>>> ---\n>>>> This was posted previously in the midst of another thread, but I don't\n>>>> think was picked up. There was some positive reaction, but one \"do we\n>>>> really need this?\" to which I responded in detail:\n>>>>\n>>>> https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/\n>>>>\n>>>> I don't really think any of that needs to go into the commit message,\n>>>> but if that's a hold-up, I can try to summarize it (though I think\n>>>> referring to the commit which _already_ did this and was accidentally\n>>>> reverted would be sufficient).\n>>> Thanks, I have a WIP version of this outstanding starting with this\n>>> patch that I was planning to submit sometime, but I'm happy to have you\n>>> pursue it, especially with the ~100 outstanding patches I have in\n>>> master..seen.\n>>>\n>>> It does feel somewhere between iffy and a landmine waiting to be stepped\n>>> on to only convert the member itself, and not any of the corresponding\n>>> \"int\" variables that track it to \"size_t\".\n>>>\n>>> If you do the change I suggested in\n>>> https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll\n>>> find that there's at least one first-order reference to this that now\n>>> uses \"int\" that if converted to \"size_t\" will result in a wrap-around\n>>> error, we're lucky that one has a test failure.\n>>>\n>>> I can tell you what that bug is, but maybe it's better if you find it\n>>> yourself :) I.e. I found *that* one, but I'm not sure I found them\n>>> all. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler\n>>> errors & the code context (note: pointer, obviously broken, but makes\n>>> the compiler yell).\n>>>\n>>> That particular bug will be caught by the compiler as it involves a >= 0\n>>> comparison against unsigned, but we may not not have that everywhere...\n>> I'm particularly interested in the int -> size_t change problem as part\n>> of the wider 4GB limitations for the LLP64 systems [0] such as the\n>> RaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big\n>> problem.\n> Okey, fine, no fun excercise for the reader then ;)\n\nThere's a lot of weeds in there ;-)  In some ways it feels like the\nSHA1->SHA256 transition, but without the consensus, as the 'shifting\nfoundations' problem only affect a subgroup of a subgroup (large Windows\nfiles and repositories).\n>\n> This is what I'd been sitting on locally since that recent thread, I\n> polished it up a bit since Jeff King posted his version.\n>\n> The potential overflow bug I mentioned is in rebase.c. See\n> 5/7. \"Potential\" because it's not a bug now, but that code\n> intentionally considers a strvec, and then iterates it from nr-1 to 0,\n> and if it reaches 0 intentionally counts down one more to -1 to\n> indicate that it's visited all elements.\n\nIt's these tidbits about how the problems surface, their detection and\nresolution that are really useful. Along with general awareness raising.\nAt least here the issue is reasonably tightly focussed, and even then,\ntesting is hard.\n>\n> We then check that with i >= 0, except of course if it becomes\n> unsigned that doesn't become -1, but rather it wraps around.\n>\n> The rest of this is all changes to have that s/int/size_t/ radiate\n> outwards, i.e. when we assign that value to a variable somewhere its\n> now a \"size_t\" instead of an \"int\" etc.\n\nIn the LLP64 case, I'm somewhat concerned about the possible pushback of\na wide spread s/int/size_t/ on the codebase's look & feel.\n (aside) I don't think there is even a `1S` to match the  the `1L` and\n`1U` shorthands used in various places.\n\nNone of that is part of the series, but the patches are beneficial to\nthe codes portability.\n\n>\n>> [0]\n>> http://nickdesaulniers.github.io/blog/2016/05/30/data-models-and-word-size/\n>> [1] https://github.com/git-lfs/git-lfs/issues/2434  Git on Windows\n>> client corrupts files > 4Gb\n>> [2] https://github.com/git-for-windows/git/pull/2179  [DRAFT] for\n>> testing : Fix 4Gb limit for large files on Git for Windows\n> Jeff King (1):\n> strvec: use size_t to store nr and alloc\n>\n> Ævar Arnfjörð Bjarmason (6):\n> remote-curl: pass \"struct strvec *\" instead of int/char ** pair\n> pack-objects: pass \"struct strvec *\" instead of int/char ** pair\n> sequencer.[ch]: pass \"struct strvec *\" instead of int/char ** pair\n> upload-pack.c: pass \"struct strvec *\" instead of int/char ** pair\n> rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"\n> strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"\n>\n> builtin/pack-objects.c | 6 +++---\n> builtin/rebase.c | 26 ++++++++++++--------------\n> connect.c | 8 ++++----\n> fetch-pack.c | 4 ++--\n> ls-refs.c | 2 +-\n> remote-curl.c | 23 +++++++++++------------\n> sequencer.c | 8 ++++----\n> sequencer.h | 4 ++--\n> serve.c | 2 +-\n> shallow.c | 5 +++--\n> shallow.h | 6 ++++--\n> strvec.h | 4 ++--\n> submodule.c | 2 +-\n> upload-pack.c | 7 +++----\n> 14 files changed, 53 insertions(+), 54 deletions(-)\n>\n> Range-diff against v1:\n> -: ----------- > 1: 2ef48d734e8 remote-curl: pass \"struct strvec *\" instead of int/char ** pair\n> -: ----------- > 2: 7f59a58ed97 pack-objects: pass \"struct strvec *\" instead of int/char ** pair\n> -: ----------- > 3: c35cfb9c9c5 sequencer.[ch]: pass \"struct strvec *\" instead of int/char ** pair\n> -: ----------- > 4: 2e0b82d4316 upload-pack.c: pass \"struct strvec *\" instead of int/char ** pair\n> -: ----------- > 5: be85a0565ef rebase: don't have loop over \"struct strvec\" depend on signed \"nr\"\n> 1: 498f5ed80dc ! 6: ba17290852c strvec: use size_t to store nr and alloc\n> @@ Commit message\n> We can correct it now; better late than never.\n> \n> Signed-off-by: Jeff King \n> + Signed-off-by: Ævar Arnfjörð Bjarmason \n> \n> ## strvec.h ##\n> @@ strvec.h: extern const char *empty_strvec[];\n> -: ----------- > 7: 2edd9708888 strvec API users: change some \"int\" tracking \"nr\" to \"size_t\"\n\n"},{"id":"435709","messageId":"c9f76d8f-7b49-d4da-eec2-320ffa523144@iee.email","threadId":"56486","inReplyTo":"YT54mskEIir2gXc2@coredump.intra.peff.net","subject":"Re: [PATCH] strvec: use size_t to store nr and alloc","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-09-13T11:42:50Z","receivedAt":"2021-09-13T11:42:54Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 12/09/2021 23:00, Jeff King wrote:\n> On Sat, Sep 11, 2021 at 11:48:38PM +0100, Philip Oakley wrote:\n>\n>> I'm particularly interested in the int -> size_t change problem as part\n>> of the wider 4GB limitations for the LLP64 systems [0] such as the\n>> RaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big\n>> problem.\n> Note that a lot of the Windows LLP64 problems are really a separate\n> issue. They come from a misuse of \"unsigned long\" as \"gee, this should\n> be big enough for anything\". Most of that is due to its use for object\n> sizes, which of course infected a whole bunch of other code.\n\nI don't see it (root cause) as independent though. This is a nicely\nseparated issue (effect), which helps.\n\n> Which isn't to say it's not important. But my main goal here was making\n> sure we use size_t for growth allocations to avoid integer overflow\n> leading to under-allocation (and thus heap overflow).\n\nIt's also been helpful in highlighting some of the wider issues and\napproaches.\n\nThank you\n\nPhilip\n\n"},{"id":"435717","messageId":"87mtog4pai.fsf@evledraar.gmail.com","threadId":"56486","inReplyTo":"xmqqlf41ghmk.fsf@gitster.g","subject":"Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T12:29:01Z","receivedAt":"2021-09-13T12:45:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 12 2021, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> I'm a little \"meh\" on some of these, for a few reasons:\n>>\n>> - anything calling into setup_revisions() eventually is just kicking\n>> the can anyway. And these are generally not buggy in the first place,\n>> since they're bounded argv creations.\n>>\n>> - passing a strvec instead of the broken-down pair is a less flexible\n>> interface. It's one thing if the callee benefits from seeing the\n>> strvec (say, because they may push more items onto it). But I think\n>> with strbufs, we have a general guideline that if a function _can_\n>> take the bare pointer, then it should. (Sorry, I don't have a\n>> succinct reference to CodingGuidelines or anything like that; I feel\n>> like this is wisdom we came up with on the list in the early days of\n>> strbufs).\n>>\n>> - if we are going to pass a strvec, it should almost certainly be\n>> const, to make it clear how we intend to use it.\n>\n> All true.\n>\n>> These cases are largely stupid things that real people would never come\n>> across. The real goal is making sure we don't get hit with a memory\n>> safety bug (under-allocation, converting a big size_t to a negative int,\n>> etc).\n>\n> Yes. \n>\n> Ævar, I do not mean any disrespect to you, but I have to say that\n> topics like this one are starting to wear my concentration and\n> patience down really thin and making me really slow down.\n\nI'm sorry. I'll try to lay off on the patch firehose until the delta\nI've got in master..seen is way down from what it is now.\n\n> Perhaps I am biased by not yet having seen what you eventually want\n> to build on top, and because of that I do not understood why and how\n> these \"clean ups\" are so valuable to have right now (as opposed to\n> just letting the sleeping dog lie), which you would of course have a\n> much better chance to know than I do.\n\nI could go into that, but it's probably best to leave it at: Yeah for\nthese seemingly going nowhere small changes I'm generally taking them\nsomewhere interesting.\n\nBut figuring out how to split that is still a hard problem. E.g. I've\nhad one series (the fsck error message improvements) that's been stalled\nfor ~3 months due to size/including the \"interesting\" part, but recently\nrelatively small increments of prep code changes seem to get a lot of\nreview traction.\n\nAs for this strvec.h s/int/size_t/ topic. I'm not taking that anywhere,\nJeff suggested it and came up with the patch, I figured more helpful\nthan \"if we change s/int/size_t/g for x, shouldn't we change that for y\nwhich whe assign x to?\" would be patches I had to do that, which I'd\ncome up with after Jeff suggested this direction in response to another\ntopic.\n"},{"id":"435748","messageId":"YT+IU9J3yn1vlOuc@coredump.intra.peff.net","threadId":"56486","inReplyTo":"87mtog4pai.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-13T17:20:19Z","receivedAt":"2021-09-13T17:20:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 02:29:01PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> As for this strvec.h s/int/size_t/ topic. I'm not taking that anywhere,\n> Jeff suggested it and came up with the patch, I figured more helpful\n> than \"if we change s/int/size_t/g for x, shouldn't we change that for y\n> which whe assign x to?\" would be patches I had to do that, which I'd\n> come up with after Jeff suggested this direction in response to another\n> topic.\n\nI'm not inherently opposed to further int/size_t cleanups. But the\ntrouble is that my single patch stands on its own as an improvement to a\nreal issue, and does not (as far as I know) have any functional\ndownsides (either known or even hypothetical, aside from the obvious\nmismatch that some callers will still use \"int\").\n\nBut doing wide-spread int/size_t conversion has less obvious immediate\nbenefit, is much easier to get wrong, and may introduce further\ncomplications (e.g., differences of opinion in whether we should be\npassing strvecs around more, or just using size_t in more places).\n\nSo I don't mind a series in that direction (though I don't necessarily\nthink it is the best use of time), but I'd prefer not to see my original\npatch tied up in it.\n\n-Peff\n"}]}