{"thread":{"id":"62569","subject":"[PATCH] strvec: `strvec_splice()` to a statically initialized vector","startedAt":"2024-11-29T17:23:49Z","lastAt":"2024-12-09T22:42:36Z","messageCount":21,"participants":["Rubén Justo","Junio C Hamano","Patrick Steinhardt","karthik nayak","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508340","messageId":"37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com","threadId":"62569","inReplyTo":null,"subject":"[PATCH] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-11-29T17:23:45Z","receivedAt":"2024-11-29T17:23:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Let's avoid an invalid pointer error in case a client of\n`strvec_splice()` ends up with something similar to:\n\n       struct strvec arr = STRVEC_INIT;\n       const char *rep[] = { \"foo\" };\n\n       strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nI've had some time to review the new iteration of the series where\n`strvec_splice()` was introduced and perhaps we want to consider cases\nwhere we end up using `strvec_splice()` with a statically initialized\n`struct strvec`, i.e:\n\n       struct strvec value = STRVEC_INIT;\n       int s = 0, e = 0;\n\n       ... nothing added to `value` and \"s == e == 0\" ...\n\n       const char *rep[] = { \"foo\" };\n       strvec_splice(&arr, s, e, rep, ARRAY_SIZE(rep));\n\n       ... realloc(): invalid pointer\n\nSorry for getting back to this so late.  This slipped through in my\nreview.\n\nI know the series is already in `next`.  To avoid adding noise to the\nseries I'm not responding to the conversation, but here is a link to\nit:\n\n  https://lore.kernel.org/git/20241120-b4-pks-leak-fixes-pt10-v3-0-d67f08f45c74@pks.im/\n\n strvec.c              | 10 ++++++----\n t/unit-tests/strvec.c | 10 ++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex d1cf4e2496..64750e35e3 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n {\n \tif (idx + len > array->nr)\n \t\tBUG(\"range outside of array boundary\");\n-\tif (replacement_len > len)\n+\tif (replacement_len > len) {\n+\t\tif (array->v == empty_strvec)\n+\t\t\tarray->v = NULL;\n \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n \t\t\t   array->alloc);\n+\t}\n \tfor (size_t i = 0; i < len; i++)\n \t\tfree((char *)array->v[idx + i]);\n-\tif (replacement_len != len) {\n+\tif ((replacement_len != len) && array->nr)\n \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n-\t\tarray->nr += (replacement_len - len);\n-\t}\n+\tarray->nr += (replacement_len - len);\n \tfor (size_t i = 0; i < replacement_len; i++)\n \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n }\ndiff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c\nindex 855b602337..e66b7bbfae 100644\n--- a/t/unit-tests/strvec.c\n+++ b/t/unit-tests/strvec.c\n@@ -88,6 +88,16 @@ void test_strvec__pushv(void)\n \tstrvec_clear(&vec);\n }\n \n+void test_strvec__splice_just_initialized_strvec(void)\n+{\n+\tstruct strvec vec = STRVEC_INIT;\n+\tconst char *replacement[] = { \"foo\" };\n+\n+\tstrvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));\n+\tcheck_strvec(&vec, \"foo\", NULL);\n+\tstrvec_clear(&vec);\n+}\n+\n void test_strvec__splice_with_same_size_replacement(void)\n {\n \tstruct strvec vec = STRVEC_INIT;\n\n-- \n2.47.0.280.geb6a512a19\n"},{"id":"508407","messageId":"xmqqiks2kg6o.fsf@gitster.g","threadId":"62569","inReplyTo":"37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com","subject":"Re: [PATCH] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-02T01:49:03Z","receivedAt":"2024-12-02T01:49:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Let's avoid an invalid pointer error in case a client of\n> `strvec_splice()` ends up with something similar to:\n>\n>        struct strvec arr = STRVEC_INIT;\n>        const char *rep[] = { \"foo\" };\n>\n>        strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));\n\nWell spotted, but the explanation can be a bit more helpful to\ncasual readers.  If there were a few paragraphs like below\n\n    An empty strvec does not represent the array part of the\n    structure with a NULL pointer, but with a singleton empty array,\n    to help read-only applications.  This is similar to how an empty\n    strbuf uses a singleton empty string.\n\n    This approach requires us to be careful when adding elements to\n    an empty instance.  The strvec_splice() API function we recently\n    introduced however forgot to special case an empty strvec, and\n    ended up applying realloc() to the singleton.\n    \nbefore your proposed commit log message, I wouldn't have needed to\ngo read the implementation of STRVEC_INIT to understand what the fix\nis about.  From the fix by itself, it is a bit hard to see why\nempty_strvec needs to be special cased, until you re-read the\nimplementation of STRVEC_INIT.\n\nThanks.\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>\n> I've had some time to review the new iteration of the series where\n> `strvec_splice()` was introduced and perhaps we want to consider cases\n> where we end up using `strvec_splice()` with a statically initialized\n> `struct strvec`, i.e:\n>\n>        struct strvec value = STRVEC_INIT;\n>        int s = 0, e = 0;\n>\n>        ... nothing added to `value` and \"s == e == 0\" ...\n>\n>        const char *rep[] = { \"foo\" };\n>        strvec_splice(&arr, s, e, rep, ARRAY_SIZE(rep));\n>\n>        ... realloc(): invalid pointer\n>\n> Sorry for getting back to this so late.  This slipped through in my\n> review.\n>\n> I know the series is already in `next`.  To avoid adding noise to the\n> series I'm not responding to the conversation, but here is a link to\n> it:\n>\n>   https://lore.kernel.org/git/20241120-b4-pks-leak-fixes-pt10-v3-0-d67f08f45c74@pks.im/\n>\n>  strvec.c              | 10 ++++++----\n>  t/unit-tests/strvec.c | 10 ++++++++++\n>  2 files changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/strvec.c b/strvec.c\n> index d1cf4e2496..64750e35e3 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>  {\n>  \tif (idx + len > array->nr)\n>  \t\tBUG(\"range outside of array boundary\");\n> -\tif (replacement_len > len)\n> +\tif (replacement_len > len) {\n> +\t\tif (array->v == empty_strvec)\n> +\t\t\tarray->v = NULL;\n>  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>  \t\t\t   array->alloc);\n> +\t}\n>  \tfor (size_t i = 0; i < len; i++)\n>  \t\tfree((char *)array->v[idx + i]);\n> -\tif (replacement_len != len) {\n> +\tif ((replacement_len != len) && array->nr)\n>  \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n>  \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n> -\t\tarray->nr += (replacement_len - len);\n> -\t}\n> +\tarray->nr += (replacement_len - len);\n>  \tfor (size_t i = 0; i < replacement_len; i++)\n>  \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n>  }\n> diff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c\n> index 855b602337..e66b7bbfae 100644\n> --- a/t/unit-tests/strvec.c\n> +++ b/t/unit-tests/strvec.c\n> @@ -88,6 +88,16 @@ void test_strvec__pushv(void)\n>  \tstrvec_clear(&vec);\n>  }\n>  \n> +void test_strvec__splice_just_initialized_strvec(void)\n> +{\n> +\tstruct strvec vec = STRVEC_INIT;\n> +\tconst char *replacement[] = { \"foo\" };\n> +\n> +\tstrvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));\n> +\tcheck_strvec(&vec, \"foo\", NULL);\n> +\tstrvec_clear(&vec);\n> +}\n> +\n>  void test_strvec__splice_with_same_size_replacement(void)\n>  {\n>  \tstruct strvec vec = STRVEC_INIT;\n"},{"id":"508447","messageId":"Z02t4zPTR6O2Px1n@pks.im","threadId":"62569","inReplyTo":"37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com","subject":"Re: [PATCH] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-02T12:54:00Z","receivedAt":"2024-12-02T12:54:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 29, 2024 at 06:23:45PM +0100, Rubén Justo wrote:\n> Let's avoid an invalid pointer error in case a client of\n> `strvec_splice()` ends up with something similar to:\n> \n>        struct strvec arr = STRVEC_INIT;\n>        const char *rep[] = { \"foo\" };\n> \n>        strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));\n> \n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n> \n> I've had some time to review the new iteration of the series where\n> `strvec_splice()` was introduced and perhaps we want to consider cases\n> where we end up using `strvec_splice()` with a statically initialized\n> `struct strvec`, i.e:\n> \n>        struct strvec value = STRVEC_INIT;\n>        int s = 0, e = 0;\n> \n>        ... nothing added to `value` and \"s == e == 0\" ...\n> \n>        const char *rep[] = { \"foo\" };\n>        strvec_splice(&arr, s, e, rep, ARRAY_SIZE(rep));\n> \n>        ... realloc(): invalid pointer\n> \n> Sorry for getting back to this so late.  This slipped through in my\n> review.\n> \n> I know the series is already in `next`.  To avoid adding noise to the\n> series I'm not responding to the conversation, but here is a link to\n> it:\n\nThanks a lot for fixing this!\n\n> diff --git a/strvec.c b/strvec.c\n> index d1cf4e2496..64750e35e3 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>  {\n>  \tif (idx + len > array->nr)\n>  \t\tBUG(\"range outside of array boundary\");\n> -\tif (replacement_len > len)\n> +\tif (replacement_len > len) {\n> +\t\tif (array->v == empty_strvec)\n> +\t\t\tarray->v = NULL;\n>  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>  \t\t\t   array->alloc);\n> +\t}\n\nMakes sense.\n\n>  \tfor (size_t i = 0; i < len; i++)\n>  \t\tfree((char *)array->v[idx + i]);\n> -\tif (replacement_len != len) {\n> +\tif ((replacement_len != len) && array->nr)\n>  \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n>  \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n> -\t\tarray->nr += (replacement_len - len);\n> -\t}\n\nOkay, here we only move existing entries around if the array actually\nhad entries in the first place. Otherwise there's nothing to move\naround. Makes sense.\n\n> +\tarray->nr += (replacement_len - len);\n\nThe braces aren't required.\n\nThanks!\n\nPatrick\n"},{"id":"508473","messageId":"b0bd6c5d-83eb-4545-9b38-ab4e69d3882c@gmail.com","threadId":"62569","inReplyTo":"xmqqiks2kg6o.fsf@gitster.g","subject":"Re: [PATCH] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-02T22:01:14Z","receivedAt":"2024-12-02T22:01:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Dec 02, 2024 at 10:49:03AM +0900, Junio C Hamano wrote:\n\n> > Let's avoid an invalid pointer error in case a client of\n> > `strvec_splice()` ends up with something similar to:\n> >\n> >        struct strvec arr = STRVEC_INIT;\n> >        const char *rep[] = { \"foo\" };\n> >\n> >        strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));\n> \n> Well spotted, but the explanation can be a bit more helpful to\n> casual readers.  If there were a few paragraphs like below\n> \n>     An empty strvec does not represent the array part of the\n>     structure with a NULL pointer, but with a singleton empty array,\n>     to help read-only applications.  This is similar to how an empty\n>     strbuf uses a singleton empty string.\n> \n>     This approach requires us to be careful when adding elements to\n>     an empty instance.  The strvec_splice() API function we recently\n>     introduced however forgot to special case an empty strvec, and\n>     ended up applying realloc() to the singleton.\n>     \n> before your proposed commit log message, I wouldn't have needed to\n> go read the implementation of STRVEC_INIT to understand what the fix\n> is about.  From the fix by itself, it is a bit hard to see why\n> empty_strvec needs to be special cased, until you re-read the\n> implementation of STRVEC_INIT.\n\nThe explanation can certainly be improved.  I'll send a v2\niteration soon.  Thanks.\n"},{"id":"508555","messageId":"5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com","threadId":"62569","inReplyTo":"37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com","subject":"[PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-03T19:47:43Z","receivedAt":"2024-12-03T19:47:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We use a singleton empty array to initialize a `struct strvec`,\nsimilar to the empty string singleton we use to initialize a `struct\nstrbuf`.\n\nNote that an empty strvec instance (with zero elements) does not\nnecessarily need to be an instance initialized with the singleton.\nLet's refer to strvec instances initialized with the singleton as\n\"empty-singleton\" instances.\n\n    As a side note, this is the current `strvec_pop()`:\n\n    void strvec_pop(struct strvec *array)\n    {\n    \tif (!array->nr)\n    \t\treturn;\n    \tfree((char *)array->v[array->nr - 1]);\n    \tarray->v[array->nr - 1] = NULL;\n    \tarray->nr--;\n    }\n\n    So, with `strvec_pop()` an instance can become empty but it does\n    not going to be the an \"empty-singleton\".\n\nThis \"empty-singleton\" circumstance requires us to be careful when\nadding elements to instances.  Specifically, when adding the first\nelement:  we detach the strvec instance from the singleton and set the\ninternal pointer in the instance to NULL.  After this point we apply\n`realloc()` on the pointer.  We do this in `strvec_push_nodup()`, for\nexample.\n\nThe recently introduced `strvec_splice()` API is expected to be\nnormally used with non-empty strvec's.  However, it can also end up\nbeing used with \"empty-singleton\" strvec's:\n\n       struct strvec arr = STRVEC_INIT;\n       int a = 0, b = 0;\n\n       ... no modification to arr, a or b ...\n\n       const char *rep[] = { \"foo\" };\n       strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));\n\nSo, we'll try to add elements to an \"empty-singleton\" strvec instance.\n\nAvoid misapplying `realloc()` to the singleton in `strvec_splice()` by\nadding a special case for \"empty-singleton\" strvec's.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThis iteration adds more detail to the message plus a minor change to\nremove some unnecessary parentheses.\n\nJunio: My message in the previous iteration was aimed at readers like\nPatrick, who is also the author of `strvec_splice()`.  I certainly\nassumed too much prior knowledge, which made the review unnecessarily\nlaborious.\n\nRereading what I wrote last night, perhaps the problem now is excess.\nI hope not. In any case, here it is :-)\n\nThanks.\n\n strvec.c              | 10 ++++++----\n t/unit-tests/strvec.c | 10 ++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex d1cf4e2496..087c020f5b 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n {\n \tif (idx + len > array->nr)\n \t\tBUG(\"range outside of array boundary\");\n-\tif (replacement_len > len)\n+\tif (replacement_len > len) {\n+\t\tif (array->v == empty_strvec)\n+\t\t\tarray->v = NULL;\n \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n \t\t\t   array->alloc);\n+\t}\n \tfor (size_t i = 0; i < len; i++)\n \t\tfree((char *)array->v[idx + i]);\n-\tif (replacement_len != len) {\n+\tif ((replacement_len != len) && array->nr)\n \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n-\t\tarray->nr += (replacement_len - len);\n-\t}\n+\tarray->nr += replacement_len - len;\n \tfor (size_t i = 0; i < replacement_len; i++)\n \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n }\ndiff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c\nindex 855b602337..e66b7bbfae 100644\n--- a/t/unit-tests/strvec.c\n+++ b/t/unit-tests/strvec.c\n@@ -88,6 +88,16 @@ void test_strvec__pushv(void)\n \tstrvec_clear(&vec);\n }\n \n+void test_strvec__splice_just_initialized_strvec(void)\n+{\n+\tstruct strvec vec = STRVEC_INIT;\n+\tconst char *replacement[] = { \"foo\" };\n+\n+\tstrvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));\n+\tcheck_strvec(&vec, \"foo\", NULL);\n+\tstrvec_clear(&vec);\n+}\n+\n void test_strvec__splice_with_same_size_replacement(void)\n {\n \tstruct strvec vec = STRVEC_INIT;\n\nRange-diff against v1:\n1:  0b60fcc51a ! 1:  c1991e6f3c strvec: `strvec_splice()` to a statically initialized vector\n    @@ Metadata\n      ## Commit message ##\n         strvec: `strvec_splice()` to a statically initialized vector\n     \n    -    Let's avoid an invalid pointer error in case a client of\n    -    `strvec_splice()` ends up with something similar to:\n    +    We use a singleton empty array to initialize a `struct strvec`;\n    +    similar to the empty string singleton we use to initialize a `struct\n    +    strbuf`.\n    +\n    +    Note that an empty strvec instance (with zero elements) does not\n    +    necessarily need to be an instance initialized with the singleton.\n    +    Let's refer to strvec instances initialized with the singleton as\n    +    \"empty-singleton\" instances.\n    +\n    +        As a side note, this is the current `strvec_pop()`:\n    +\n    +        void strvec_pop(struct strvec *array)\n    +        {\n    +            if (!array->nr)\n    +                    return;\n    +            free((char *)array->v[array->nr - 1]);\n    +            array->v[array->nr - 1] = NULL;\n    +            array->nr--;\n    +        }\n    +\n    +        So, with `strvec_pop()` an instance can become empty but it does\n    +        not going to be the an \"empty-singleton\".\n    +\n    +    This \"empty-singleton\" circumstance requires us to be careful when\n    +    adding elements to instances.  Specifically, when adding the first\n    +    element:  when we detach the strvec instance from the singleton and\n    +    set the internal pointer in the instance to NULL.  After this point we\n    +    apply `realloc()` on the pointer.  We do this in\n    +    `strvec_push_nodup()`, for example.\n    +\n    +    The recently introduced `strvec_splice()` API is expected to be\n    +    normally used with non-empty strvec's.  However, it can also end up\n    +    being used with \"empty-singleton\" strvec's:\n     \n                struct strvec arr = STRVEC_INIT;\n    +           int a = 0, b = 0;\n    +\n    +           ... no modification to arr, a or b ...\n    +\n                const char *rep[] = { \"foo\" };\n    +           strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));\n    +\n    +    So, we'll try to add elements to an \"empty-singleton\" strvec instance.\n     \n    -           strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));\n    +    Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by\n    +    adding a special case for strvec's initialized with the singleton.\n     \n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n    @@ strvec.c: void strvec_splice(struct strvec *array, size_t idx, size_t len,\n      \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n     -\t\tarray->nr += (replacement_len - len);\n     -\t}\n    -+\tarray->nr += (replacement_len - len);\n    ++\tarray->nr += replacement_len - len;\n      \tfor (size_t i = 0; i < replacement_len; i++)\n      \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n      }\n-- \n2.47.0.281.g7eb946317c\n"},{"id":"508586","messageId":"xmqqplm871hb.fsf@gitster.g","threadId":"62569","inReplyTo":"5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T00:09:36Z","receivedAt":"2024-12-04T00:09:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Note that an empty strvec instance (with zero elements) does not\n> necessarily need to be an instance initialized with the singleton.\n\nCorrect.\n\nWhen (vec.nr == 0), vec.v may be pointing at \n\n (1) an allocated piece of memory, if the strvec was previously used\n     to hold some strings; or\n (2) singleton array with NULL.\n\nand vec.v[0] is NULL.  This is to allow you to pass vec.v[] as a\nNULL terminated list of (char *) (aka argv[][]) to functions.\n\nThat can be said a bit differently and more concisely like so:\n\n    A strvec instance with no elements can have its member .v\n    pointing at empty_strvec[] or pointing at an allocated piece of\n    memory, and either way .v[0] has NULL in it, to allow you to\n    always treat vec.v[] as a NULL terminated array of strings, even\n    immediately after initialization.\n\nand then you can lose the strvec_pop() illustration below that talks\nabout an allocated piece of memory that was previously used.\n\n> The recently introduced `strvec_splice()` API is expected to be\n> normally used with non-empty strvec's.\n\nIt is perfectly sensible to expect that you can splice your stuff\ninto an empty strvec, so all this sentence is saying is that a\nstrvec is more often non-empty than empty. I'd recommend dropping\nthis sentence.\n\nSomething like\n\n    When growing a strvec, we'd use a realloc() call on its .v[]\n    member, but a care must be taken when it is pointing at\n    empty_strvec[] and is not pointing at an allocated piece of\n    memory.  strvec_push_nodup() and strvec_push() correctly do so.\n    The recently added strvec_splice() forgot to.\n\nshould be sufficient.  Notice that I didn't have to invent a new\nterm \"empty-singleton\" at all ;-).\n\nThanks.\n\n"},{"id":"508591","messageId":"d669e92a-5499-4a33-9abc-525542615677@gmail.com","threadId":"62569","inReplyTo":"xmqqplm871hb.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-04T01:08:25Z","receivedAt":"2024-12-04T01:08:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Dec 04, 2024 at 09:09:36AM +0900, Junio C Hamano wrote:\n\n> > The recently introduced `strvec_splice()` API is expected to be\n> > normally used with non-empty strvec's.\n> \n> It is perfectly sensible to expect that you can splice your stuff\n> into an empty strvec, so all this sentence is saying is that a\n> strvec is more often non-empty than empty.\n\nI also wanted to introduce a reason why we might have overlooked\nmaking `strvec_splice()` aware of the singleton object, without using\na verb like \"forget\".\n\n> Notice that I didn't have to invent a new\n> term \"empty-singleton\" at all ;-).\n\n:-D\n\nIn my defense, I wrote the message late last night and was already\ntired.  And when I read it today, it didn't seem so bad to me, in the\ncontext of the message.\n\nThanks.\n"},{"id":"508604","messageId":"xmqqwmgf3nf3.fsf@gitster.g","threadId":"62569","inReplyTo":"5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T07:41:36Z","receivedAt":"2024-12-04T07:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is queued as rj/strvec-splice-fix, and t/unit-tests/bin/unit-tests\ndies of leaks under leak-check.\n\n\n\n$ t/unit-tests/bin/unit-tests\nTAP version 13\n# start of suite 1: ctype\nok 1 - ctype::isspace\nok 2 - ctype::isdigit\nok 3 - ctype::isalpha\nok 4 - ctype::isalnum\nok 5 - ctype::is_glob_special\nok 6 - ctype::is_regex_special\nok 7 - ctype::is_pathspec_magic\nok 8 - ctype::isascii\nok 9 - ctype::islower\nok 10 - ctype::isupper\nok 11 - ctype::iscntrl\nok 12 - ctype::ispunct\nok 13 - ctype::isxdigit\nok 14 - ctype::isprint\n# start of suite 2: strvec\nok 15 - strvec::init\nok 16 - strvec::dynamic_init\nok 17 - strvec::clear\nok 18 - strvec::push\nok 19 - strvec::pushf\nok 20 - strvec::pushl\nok 21 - strvec::pushv\nnot ok 22 - strvec::splice_just_initialized_strvec\n    ---\n    reason: |\n      String mismatch: (&vec)->v[i] != expect[i]\n      'bar' != '(null)'\n    at:\n      file: 't/unit-tests/strvec.c'\n      line: 97\n      function: 'test_strvec__splice_just_initialized_strvec'\n    ---\nok 23 - strvec::splice_with_same_size_replacement\nok 24 - strvec::splice_with_smaller_replacement\nok 25 - strvec::splice_with_bigger_replacement\nok 26 - strvec::splice_with_empty_replacement\nok 27 - strvec::splice_with_empty_original\nok 28 - strvec::splice_at_tail\nok 29 - strvec::replace_at_head\nok 30 - strvec::replace_at_tail\nok 31 - strvec::replace_in_between\nok 32 - strvec::replace_with_substring\nok 33 - strvec::remove_at_head\nok 34 - strvec::remove_at_tail\nok 35 - strvec::remove_in_between\nok 36 - strvec::pop_empty_array\nok 37 - strvec::pop_non_empty_array\nok 38 - strvec::split_empty_string\nok 39 - strvec::split_single_item\nok 40 - strvec::split_multiple_items\nok 41 - strvec::split_whitespace_only\nok 42 - strvec::split_multiple_consecutive_whitespaces\nok 43 - strvec::detach\n\n=================================================================\n==5178==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n    #0 0x5600496ec825 in __interceptor_realloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67825) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n    #1 0x56004973b4cd in xrealloc /usr/local/google/home/jch/w/git.git/wrapper.c:140:8\n    #2 0x560049714c6f in strvec_splice /usr/local/google/home/jch/w/git.git/strvec.c:67:3\n    #3 0x5600496f0c1d in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:96:2\n    #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n    #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n    #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n    #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n    #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n    #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n    #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\nDirect leak of 48 byte(s) in 1 object(s) allocated from:\n    #0 0x5600496ec640 in __interceptor_calloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67640) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n    #1 0x5600496f4cee in clar__fail /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:687:15\n    #2 0x5600496f5f25 in clar__assert_equal /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:844:3\n    #3 0x5600496f0db6 in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:97:2\n    #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n    #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n    #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n    #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n    #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n    #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n    #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\nIndirect leak of 18 byte(s) in 1 object(s) allocated from:\n    #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n    #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n    #2 0x296c6c756e28271f  (<unknown module>)\n\nIndirect leak of 4 byte(s) in 1 object(s) allocated from:\n    #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n    #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n\nSUMMARY: LeakSanitizer: 262 byte(s) leaked in 4 allocation(s).\n"},{"id":"508606","messageId":"c949fea0-817b-45f9-b8b2-55e1cb55e915@gmail.com","threadId":"62569","inReplyTo":"xmqqwmgf3nf3.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-04T08:46:22Z","receivedAt":"2024-12-04T08:46:25Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Dec 04, 2024 at 04:41:36PM +0900, Junio C Hamano wrote:\n> This is queued as rj/strvec-splice-fix, and t/unit-tests/bin/unit-tests\n> dies of leaks under leak-check.\n\nRight! We need this:\n\ndiff --git a/strvec.c b/strvec.c\nindex 087c020f5b..b1e6c5d8cd 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -66,6 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n                        array->v = NULL;\n                ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n                           array->alloc);\n+               array->v[array->nr + 1] = NULL;\n        }\n        for (size_t i = 0; i < len; i++)\n                free((char *)array->v[idx + i]);\n\nSorry.  I'll re-roll later today.\n\n> \n> \n> \n> $ t/unit-tests/bin/unit-tests\n> TAP version 13\n> # start of suite 1: ctype\n> ok 1 - ctype::isspace\n> ok 2 - ctype::isdigit\n> ok 3 - ctype::isalpha\n> ok 4 - ctype::isalnum\n> ok 5 - ctype::is_glob_special\n> ok 6 - ctype::is_regex_special\n> ok 7 - ctype::is_pathspec_magic\n> ok 8 - ctype::isascii\n> ok 9 - ctype::islower\n> ok 10 - ctype::isupper\n> ok 11 - ctype::iscntrl\n> ok 12 - ctype::ispunct\n> ok 13 - ctype::isxdigit\n> ok 14 - ctype::isprint\n> # start of suite 2: strvec\n> ok 15 - strvec::init\n> ok 16 - strvec::dynamic_init\n> ok 17 - strvec::clear\n> ok 18 - strvec::push\n> ok 19 - strvec::pushf\n> ok 20 - strvec::pushl\n> ok 21 - strvec::pushv\n> not ok 22 - strvec::splice_just_initialized_strvec\n>     ---\n>     reason: |\n>       String mismatch: (&vec)->v[i] != expect[i]\n>       'bar' != '(null)'\n>     at:\n>       file: 't/unit-tests/strvec.c'\n>       line: 97\n>       function: 'test_strvec__splice_just_initialized_strvec'\n>     ---\n> ok 23 - strvec::splice_with_same_size_replacement\n> ok 24 - strvec::splice_with_smaller_replacement\n> ok 25 - strvec::splice_with_bigger_replacement\n> ok 26 - strvec::splice_with_empty_replacement\n> ok 27 - strvec::splice_with_empty_original\n> ok 28 - strvec::splice_at_tail\n> ok 29 - strvec::replace_at_head\n> ok 30 - strvec::replace_at_tail\n> ok 31 - strvec::replace_in_between\n> ok 32 - strvec::replace_with_substring\n> ok 33 - strvec::remove_at_head\n> ok 34 - strvec::remove_at_tail\n> ok 35 - strvec::remove_in_between\n> ok 36 - strvec::pop_empty_array\n> ok 37 - strvec::pop_non_empty_array\n> ok 38 - strvec::split_empty_string\n> ok 39 - strvec::split_single_item\n> ok 40 - strvec::split_multiple_items\n> ok 41 - strvec::split_whitespace_only\n> ok 42 - strvec::split_multiple_consecutive_whitespaces\n> ok 43 - strvec::detach\n> \n> =================================================================\n> ==5178==ERROR: LeakSanitizer: detected memory leaks\n> \n> Direct leak of 192 byte(s) in 1 object(s) allocated from:\n>     #0 0x5600496ec825 in __interceptor_realloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67825) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>     #1 0x56004973b4cd in xrealloc /usr/local/google/home/jch/w/git.git/wrapper.c:140:8\n>     #2 0x560049714c6f in strvec_splice /usr/local/google/home/jch/w/git.git/strvec.c:67:3\n>     #3 0x5600496f0c1d in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:96:2\n>     #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n>     #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n>     #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n>     #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n>     #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n>     #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n>     #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n> \n> Direct leak of 48 byte(s) in 1 object(s) allocated from:\n>     #0 0x5600496ec640 in __interceptor_calloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67640) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>     #1 0x5600496f4cee in clar__fail /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:687:15\n>     #2 0x5600496f5f25 in clar__assert_equal /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:844:3\n>     #3 0x5600496f0db6 in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:97:2\n>     #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n>     #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n>     #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n>     #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n>     #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n>     #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n>     #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n> \n> Indirect leak of 18 byte(s) in 1 object(s) allocated from:\n>     #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>     #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n>     #2 0x296c6c756e28271f  (<unknown module>)\n> \n> Indirect leak of 4 byte(s) in 1 object(s) allocated from:\n>     #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>     #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n> \n> SUMMARY: LeakSanitizer: 262 byte(s) leaked in 4 allocation(s).\n"},{"id":"508607","messageId":"4e60eedc-e4d9-423c-b2e7-f1c65bccc254@gmail.com","threadId":"62569","inReplyTo":"c949fea0-817b-45f9-b8b2-55e1cb55e915@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-04T08:50:18Z","receivedAt":"2024-12-04T08:50:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\n\nOn 12/4/24 9:46 AM, Rubén Justo wrote:\n> On Wed, Dec 04, 2024 at 04:41:36PM +0900, Junio C Hamano wrote:\n>> This is queued as rj/strvec-splice-fix, and t/unit-tests/bin/unit-tests\n>> dies of leaks under leak-check.\n> \n> Right! We need this:\n> \n> diff --git a/strvec.c b/strvec.c\n> index 087c020f5b..b1e6c5d8cd 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -66,6 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>                         array->v = NULL;\n>                 ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>                            array->alloc);\n> +               array->v[array->nr + 1] = NULL;\n\nI mean:\n\n+               array->v[array->nr + (replacement_len - len) + 1] = NULL;\n\n\n>         }\n>         for (size_t i = 0; i < len; i++)\n>                 free((char *)array->v[idx + i]);\n> \n> Sorry.  I'll re-roll later today.\n> \n>>\n>>\n>>\n>> $ t/unit-tests/bin/unit-tests\n>> TAP version 13\n>> # start of suite 1: ctype\n>> ok 1 - ctype::isspace\n>> ok 2 - ctype::isdigit\n>> ok 3 - ctype::isalpha\n>> ok 4 - ctype::isalnum\n>> ok 5 - ctype::is_glob_special\n>> ok 6 - ctype::is_regex_special\n>> ok 7 - ctype::is_pathspec_magic\n>> ok 8 - ctype::isascii\n>> ok 9 - ctype::islower\n>> ok 10 - ctype::isupper\n>> ok 11 - ctype::iscntrl\n>> ok 12 - ctype::ispunct\n>> ok 13 - ctype::isxdigit\n>> ok 14 - ctype::isprint\n>> # start of suite 2: strvec\n>> ok 15 - strvec::init\n>> ok 16 - strvec::dynamic_init\n>> ok 17 - strvec::clear\n>> ok 18 - strvec::push\n>> ok 19 - strvec::pushf\n>> ok 20 - strvec::pushl\n>> ok 21 - strvec::pushv\n>> not ok 22 - strvec::splice_just_initialized_strvec\n>>     ---\n>>     reason: |\n>>       String mismatch: (&vec)->v[i] != expect[i]\n>>       'bar' != '(null)'\n>>     at:\n>>       file: 't/unit-tests/strvec.c'\n>>       line: 97\n>>       function: 'test_strvec__splice_just_initialized_strvec'\n>>     ---\n>> ok 23 - strvec::splice_with_same_size_replacement\n>> ok 24 - strvec::splice_with_smaller_replacement\n>> ok 25 - strvec::splice_with_bigger_replacement\n>> ok 26 - strvec::splice_with_empty_replacement\n>> ok 27 - strvec::splice_with_empty_original\n>> ok 28 - strvec::splice_at_tail\n>> ok 29 - strvec::replace_at_head\n>> ok 30 - strvec::replace_at_tail\n>> ok 31 - strvec::replace_in_between\n>> ok 32 - strvec::replace_with_substring\n>> ok 33 - strvec::remove_at_head\n>> ok 34 - strvec::remove_at_tail\n>> ok 35 - strvec::remove_in_between\n>> ok 36 - strvec::pop_empty_array\n>> ok 37 - strvec::pop_non_empty_array\n>> ok 38 - strvec::split_empty_string\n>> ok 39 - strvec::split_single_item\n>> ok 40 - strvec::split_multiple_items\n>> ok 41 - strvec::split_whitespace_only\n>> ok 42 - strvec::split_multiple_consecutive_whitespaces\n>> ok 43 - strvec::detach\n>>\n>> =================================================================\n>> ==5178==ERROR: LeakSanitizer: detected memory leaks\n>>\n>> Direct leak of 192 byte(s) in 1 object(s) allocated from:\n>>     #0 0x5600496ec825 in __interceptor_realloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67825) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>>     #1 0x56004973b4cd in xrealloc /usr/local/google/home/jch/w/git.git/wrapper.c:140:8\n>>     #2 0x560049714c6f in strvec_splice /usr/local/google/home/jch/w/git.git/strvec.c:67:3\n>>     #3 0x5600496f0c1d in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:96:2\n>>     #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n>>     #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n>>     #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n>>     #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n>>     #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n>>     #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n>>     #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n>>\n>> Direct leak of 48 byte(s) in 1 object(s) allocated from:\n>>     #0 0x5600496ec640 in __interceptor_calloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x67640) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>>     #1 0x5600496f4cee in clar__fail /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:687:15\n>>     #2 0x5600496f5f25 in clar__assert_equal /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:844:3\n>>     #3 0x5600496f0db6 in test_strvec__splice_just_initialized_strvec /usr/local/google/home/jch/w/git.git/t/unit-tests/strvec.c:97:2\n>>     #4 0x5600496f627b in clar_run_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:315:3\n>>     #5 0x5600496f46fa in clar_run_suite /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:412:3\n>>     #6 0x5600496f43e1 in clar_test_run /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:608:4\n>>     #7 0x5600496f4bdf in clar_test /usr/local/google/home/jch/w/git.git/t/unit-tests/clar/clar.c:651:11\n>>     #8 0x5600496f787c in cmd_main /usr/local/google/home/jch/w/git.git/t/unit-tests/unit-test.c:42:8\n>>     #9 0x5600496f793a in main /usr/local/google/home/jch/w/git.git/common-main.c:9:11\n>>     #10 0x7f59ea91dc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n>>\n>> Indirect leak of 18 byte(s) in 1 object(s) allocated from:\n>>     #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>>     #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n>>     #2 0x296c6c756e28271f  (<unknown module>)\n>>\n>> Indirect leak of 4 byte(s) in 1 object(s) allocated from:\n>>     #0 0x5600496ec3c6 in __interceptor_malloc (/usr/local/google/home/jch/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 6efbef9c6f87bfa879e770b463031b396d4d5efe)\n>>     #1 0x7f59ea9964f9 in strdup string/strdup.c:42:15\n>>\n>> SUMMARY: LeakSanitizer: 262 byte(s) leaked in 4 allocation(s).\n\n"},{"id":"508608","messageId":"xmqqser33ga6.fsf@gitster.g","threadId":"62569","inReplyTo":"4e60eedc-e4d9-423c-b2e7-f1c65bccc254@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T10:15:45Z","receivedAt":"2024-12-04T10:15:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> @@ -66,6 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>>                         array->v = NULL;\n>>                 ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>>                            array->alloc);\n>> +               array->v[array->nr + 1] = NULL;\n>\n> I mean:\n>\n> +               array->v[array->nr + (replacement_len - len) + 1] = NULL;\n>\n>\n>>         }\n>>         for (size_t i = 0; i < len; i++)\n>>                 free((char *)array->v[idx + i]);\n>> \n>> Sorry.  I'll re-roll later today.\n\nNo need to say \"sorry\".  Thanks for quickly reacting and starting to\nwork on it.\n\n"},{"id":"508613","messageId":"CAOLa=ZRaXZWmuLsmq9AFkzUFCa__=3rzAYkhULA4duJnGxcoyg@mail.gmail.com","threadId":"62569","inReplyTo":"5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-04T11:26:27Z","receivedAt":"2024-12-04T11:26:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\nNit: Is the commit subject missing a verb?\n\n> We use a singleton empty array to initialize a `struct strvec`,\n> similar to the empty string singleton we use to initialize a `struct\n> strbuf`.\n>\n\nSo a singleton empty array is a statically allocated array element, so\nfor strvec, this would be `const char *empty_strvec[] = { NULL }`.\n\n> Note that an empty strvec instance (with zero elements) does not\n> necessarily need to be an instance initialized with the singleton.\n> Let's refer to strvec instances initialized with the singleton as\n> \"empty-singleton\" instances.\n>\n\nRight, so when we add elements ideally, we ideally check whether it is a\nsingleton or not. This is evident in `strvec_push_nodup()`:\n\n    void strvec_push_nodup(struct strvec *array, char *value)\n    {\n    \tif (array->v == empty_strvec)\n    \t\tarray->v = NULL;\n\n    \tALLOC_GROW(array->v, array->nr + 2, array->alloc);\n    \tarray->v[array->nr++] = value;\n    \tarray->v[array->nr] = NULL;\n    }\n\n>     As a side note, this is the current `strvec_pop()`:\n>\n>     void strvec_pop(struct strvec *array)\n>     {\n>     \tif (!array->nr)\n>     \t\treturn;\n>     \tfree((char *)array->v[array->nr - 1]);\n>     \tarray->v[array->nr - 1] = NULL;\n>     \tarray->nr--;\n>     }\n>\n>     So, with `strvec_pop()` an instance can become empty but it does\n>     not going to be the an \"empty-singleton\".\n\nCorrect, since we simply set the array element to NULL, but this is\nstill a dynamically allocated array.\n\nNit: The sentence reads a bit weirdly.\n\n> This \"empty-singleton\" circumstance requires us to be careful when\n> adding elements to instances.  Specifically, when adding the first\n> element:  we detach the strvec instance from the singleton and set the\n> internal pointer in the instance to NULL.  After this point we apply\n> `realloc()` on the pointer.  We do this in `strvec_push_nodup()`, for\n> example.\n>\n> The recently introduced `strvec_splice()` API is expected to be\n> normally used with non-empty strvec's.  However, it can also end up\n> being used with \"empty-singleton\" strvec's:\n>\n>        struct strvec arr = STRVEC_INIT;\n>        int a = 0, b = 0;\n>\n>        ... no modification to arr, a or b ...\n>\n>        const char *rep[] = { \"foo\" };\n>        strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));\n>\n> So, we'll try to add elements to an \"empty-singleton\" strvec instance.\n>\n> Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by\n> adding a special case for \"empty-singleton\" strvec's.\n>\n\nSo everything said here makes sense, that's a great explanation.\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>\n> This iteration adds more detail to the message plus a minor change to\n> remove some unnecessary parentheses.\n>\n> Junio: My message in the previous iteration was aimed at readers like\n> Patrick, who is also the author of `strvec_splice()`.  I certainly\n> assumed too much prior knowledge, which made the review unnecessarily\n> laborious.\n>\n> Rereading what I wrote last night, perhaps the problem now is excess.\n> I hope not. In any case, here it is :-)\n>\n\nI would say this is very useful over the first iteration, considering I\nam someone without prior knowledge here.\n\n> Thanks.\n>\n>  strvec.c              | 10 ++++++----\n>  t/unit-tests/strvec.c | 10 ++++++++++\n>  2 files changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/strvec.c b/strvec.c\n> index d1cf4e2496..087c020f5b 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>  {\n>  \tif (idx + len > array->nr)\n>  \t\tBUG(\"range outside of array boundary\");\n> -\tif (replacement_len > len)\n> +\tif (replacement_len > len) {\n> +\t\tif (array->v == empty_strvec)\n> +\t\t\tarray->v = NULL;\n>  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>  \t\t\t   array->alloc);\n> +\t}\n>  \tfor (size_t i = 0; i < len; i++)\n>  \t\tfree((char *)array->v[idx + i]);\n> -\tif (replacement_len != len) {\n> +\tif ((replacement_len != len) && array->nr)\n>  \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n>  \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n> -\t\tarray->nr += (replacement_len - len);\n> -\t}\n> +\tarray->nr += replacement_len - len;\n\nWhy is this second block of changes needed? Will array-nr ever be 0 when\nwe reach here?\n\n>  \tfor (size_t i = 0; i < replacement_len; i++)\n>  \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n>  }\n\n[snip]\n"},{"id":"508627","messageId":"8ea38d79-9e1e-4d69-b734-273ebddbe384@gmail.com","threadId":"62569","inReplyTo":"CAOLa=ZRaXZWmuLsmq9AFkzUFCa__=3rzAYkhULA4duJnGxcoyg@mail.gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-04T22:22:01Z","receivedAt":"2024-12-04T22:22:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Dec 04, 2024 at 11:26:27AM +0000, karthik nayak wrote:\n\n> Nit: Is the commit subject missing a verb?\n\nI guess something like \"To strvec_splice\" sounded good in my head \n:)\n\n> \n> > We use a singleton empty array to initialize a `struct strvec`,\n> > similar to the empty string singleton we use to initialize a `struct\n> > strbuf`.\n> >\n> \n> So a singleton empty array is a statically allocated array element, so\n> for strvec, this would be `const char *empty_strvec[] = { NULL }`.\n> \n> > Note that an empty strvec instance (with zero elements) does not\n> > necessarily need to be an instance initialized with the singleton.\n> > Let's refer to strvec instances initialized with the singleton as\n> > \"empty-singleton\" instances.\n> >\n> \n> Right, so when we add elements ideally, we ideally check whether it is a\n> singleton or not. This is evident in `strvec_push_nodup()`:\n> \n>     void strvec_push_nodup(struct strvec *array, char *value)\n>     {\n>     \tif (array->v == empty_strvec)\n>     \t\tarray->v = NULL;\n> \n>     \tALLOC_GROW(array->v, array->nr + 2, array->alloc);\n>     \tarray->v[array->nr++] = value;\n>     \tarray->v[array->nr] = NULL;\n>     }\n> \n> >     As a side note, this is the current `strvec_pop()`:\n> >\n> >     void strvec_pop(struct strvec *array)\n> >     {\n> >     \tif (!array->nr)\n> >     \t\treturn;\n> >     \tfree((char *)array->v[array->nr - 1]);\n> >     \tarray->v[array->nr - 1] = NULL;\n> >     \tarray->nr--;\n> >     }\n> >\n> >     So, with `strvec_pop()` an instance can become empty but it does\n> >     not going to be the an \"empty-singleton\".\n> \n> Correct, since we simply set the array element to NULL, but this is\n> still a dynamically allocated array.\n> \n> Nit: The sentence reads a bit weirdly.\n> \n> > This \"empty-singleton\" circumstance requires us to be careful when\n> > adding elements to instances.  Specifically, when adding the first\n> > element:  we detach the strvec instance from the singleton and set the\n> > internal pointer in the instance to NULL.  After this point we apply\n> > `realloc()` on the pointer.  We do this in `strvec_push_nodup()`, for\n> > example.\n> >\n> > The recently introduced `strvec_splice()` API is expected to be\n> > normally used with non-empty strvec's.  However, it can also end up\n> > being used with \"empty-singleton\" strvec's:\n> >\n> >        struct strvec arr = STRVEC_INIT;\n> >        int a = 0, b = 0;\n> >\n> >        ... no modification to arr, a or b ...\n> >\n> >        const char *rep[] = { \"foo\" };\n> >        strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));\n> >\n> > So, we'll try to add elements to an \"empty-singleton\" strvec instance.\n> >\n> > Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by\n> > adding a special case for \"empty-singleton\" strvec's.\n> >\n> \n> So everything said here makes sense, that's a great explanation.\n\nThanks.\n\n> \n> > Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> > ---\n> >\n> > This iteration adds more detail to the message plus a minor change to\n> > remove some unnecessary parentheses.\n> >\n> > Junio: My message in the previous iteration was aimed at readers like\n> > Patrick, who is also the author of `strvec_splice()`.  I certainly\n> > assumed too much prior knowledge, which made the review unnecessarily\n> > laborious.\n> >\n> > Rereading what I wrote last night, perhaps the problem now is excess.\n> > I hope not. In any case, here it is :-)\n> >\n> \n> I would say this is very useful over the first iteration, considering I\n> am someone without prior knowledge here.\n\nI'm glad to read that.  I guess Junio is to blame ;)  Thanks.\n\n> \n> > Thanks.\n> >\n> >  strvec.c              | 10 ++++++----\n> >  t/unit-tests/strvec.c | 10 ++++++++++\n> >  2 files changed, 16 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/strvec.c b/strvec.c\n> > index d1cf4e2496..087c020f5b 100644\n> > --- a/strvec.c\n> > +++ b/strvec.c\n> > @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n> >  {\n> >  \tif (idx + len > array->nr)\n> >  \t\tBUG(\"range outside of array boundary\");\n> > -\tif (replacement_len > len)\n> > +\tif (replacement_len > len) {\n> > +\t\tif (array->v == empty_strvec)\n> > +\t\t\tarray->v = NULL;\n> >  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n> >  \t\t\t   array->alloc);\n> > +\t}\n> >  \tfor (size_t i = 0; i < len; i++)\n> >  \t\tfree((char *)array->v[idx + i]);\n> > -\tif (replacement_len != len) {\n> > +\tif ((replacement_len != len) && array->nr)\n> >  \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n> >  \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n> > -\t\tarray->nr += (replacement_len - len);\n> > -\t}\n> > +\tarray->nr += replacement_len - len;\n> \n> Why is this second block of changes needed? Will array-nr ever be 0 when\n> we reach here?\n\nI'm not sure I understand your questions.\n\nAt that point, `array->nr` is the initial number of entries in the\nvector.  It can be 0 when `strvec_splice()` is applied to an empty\nvector.\n\nWe are moving the line where we update \"array->nr\" outside the `if`\nblock because we want to do it even when we are not moving existing\nentries.  Again, this happens when `strvec_splice()` is applied to an\nempty vector.\n\nFinally, we don't mind too much (or value more the simplicity) of the\nnow unconditional update of \"array->nr\" because a clever compiler will\ngive us the third arm of the if: \"else -> do nothing\".  When\n`replacement_len == len` => \"array->nr += 0\" => do nothing.\n\n> \n> >  \tfor (size_t i = 0; i < replacement_len; i++)\n> >  \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n> >  }\n> \n> [snip]\n\nThank you for your review.\n"},{"id":"508628","messageId":"3c7b3c26-7501-4797-8afa-c7f7e9c46558@gmail.com","threadId":"62569","inReplyTo":"5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com","subject":"[PATCH v3] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-04T22:44:25Z","receivedAt":"2024-12-04T22:44:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We use a singleton empty array to initialize a `struct strvec`;\nsimilar to the empty string singleton we use to initialize a `struct\nstrbuf`.\n\nNote that an empty strvec instance (with zero elements) does not\nnecessarily need to be an instance initialized with the singleton.\nLet's refer to strvec instances initialized with the singleton as\n\"empty-singleton\" instances.\n\n    As a side note, this is the current `strvec_pop()`:\n\n    void strvec_pop(struct strvec *array)\n    {\n    \tif (!array->nr)\n    \t\treturn;\n    \tfree((char *)array->v[array->nr - 1]);\n    \tarray->v[array->nr - 1] = NULL;\n    \tarray->nr--;\n    }\n\n    So, with `strvec_pop()` an instance can become empty but it does\n    not going to be the an \"empty-singleton\".\n\nThis \"empty-singleton\" circumstance requires us to be careful when\nadding elements to instances.  Specifically, when adding the first\nelement:  when we detach the strvec instance from the singleton and\nset the internal pointer in the instance to NULL.  After this point we\napply `realloc()` on the pointer.  We do this in\n`strvec_push_nodup()`, for example.\n\nThe recently introduced `strvec_splice()` API is expected to be\nnormally used with non-empty strvec's.  However, it can also end up\nbeing used with \"empty-singleton\" strvec's:\n\n       struct strvec arr = STRVEC_INIT;\n       int a = 0, b = 0;\n\n       ... no modification to arr, a or b ...\n\n       const char *rep[] = { \"foo\" };\n       strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));\n\nSo, we'll try to add elements to an \"empty-singleton\" strvec instance.\n\nAvoid misapplying `realloc()` to the singleton in `strvec_splice()` by\nadding a special case for strvec's initialized with the singleton.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThis iteration fixes a problem we saw when running with SANITIZE=leak.\nAlthough it wasn't a leak.\n\nWe need to end the array because `realloc(NULL)` is not going to give\nus that { NULL }.  I know it's something I considered at some point\nbecause I thought about a change like `CALLOC_GROW()`.  Perhaps\nanother time.\n\n strvec.c              | 11 +++++++----\n t/unit-tests/strvec.c | 10 ++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex d1cf4e2496..62283fcef2 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -61,16 +61,19 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n {\n \tif (idx + len > array->nr)\n \t\tBUG(\"range outside of array boundary\");\n-\tif (replacement_len > len)\n+\tif (replacement_len > len) {\n+\t\tif (array->v == empty_strvec)\n+\t\t\tarray->v = NULL;\n \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n \t\t\t   array->alloc);\n+\t\tarray->v[array->nr + (replacement_len - len) + 1] = NULL;\n+\t}\n \tfor (size_t i = 0; i < len; i++)\n \t\tfree((char *)array->v[idx + i]);\n-\tif (replacement_len != len) {\n+\tif ((replacement_len != len) && array->nr)\n \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n-\t\tarray->nr += (replacement_len - len);\n-\t}\n+\tarray->nr += replacement_len - len;\n \tfor (size_t i = 0; i < replacement_len; i++)\n \t\tarray->v[idx + i] = xstrdup(replacement[i]);\n }\ndiff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c\nindex 855b602337..e66b7bbfae 100644\n--- a/t/unit-tests/strvec.c\n+++ b/t/unit-tests/strvec.c\n@@ -88,6 +88,16 @@ void test_strvec__pushv(void)\n \tstrvec_clear(&vec);\n }\n \n+void test_strvec__splice_just_initialized_strvec(void)\n+{\n+\tstruct strvec vec = STRVEC_INIT;\n+\tconst char *replacement[] = { \"foo\" };\n+\n+\tstrvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));\n+\tcheck_strvec(&vec, \"foo\", NULL);\n+\tstrvec_clear(&vec);\n+}\n+\n void test_strvec__splice_with_same_size_replacement(void)\n {\n \tstruct strvec vec = STRVEC_INIT;\n\nInterdiff against v2:\n  diff --git a/strvec.c b/strvec.c\n  index 087c020f5b..62283fcef2 100644\n  --- a/strvec.c\n  +++ b/strvec.c\n  @@ -66,6 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n   \t\t\tarray->v = NULL;\n   \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n   \t\t\t   array->alloc);\n  +\t\tarray->v[array->nr + (replacement_len - len) + 1] = NULL;\n   \t}\n   \tfor (size_t i = 0; i < len; i++)\n   \t\tfree((char *)array->v[idx + i]);\n-- \n2.47.0.281.g735430a4cf\n"},{"id":"508744","messageId":"CAOLa=ZSCyvd+LF_8ebkV5+O+y-jL90jd6aVb3HAytiW-1CiCbQ@mail.gmail.com","threadId":"62569","inReplyTo":"8ea38d79-9e1e-4d69-b734-273ebddbe384@gmail.com","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-06T11:33:07Z","receivedAt":"2024-12-06T11:33:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n[snip]\n\n>> > Thanks.\n>> >\n>> >  strvec.c              | 10 ++++++----\n>> >  t/unit-tests/strvec.c | 10 ++++++++++\n>> >  2 files changed, 16 insertions(+), 4 deletions(-)\n>> >\n>> > diff --git a/strvec.c b/strvec.c\n>> > index d1cf4e2496..087c020f5b 100644\n>> > --- a/strvec.c\n>> > +++ b/strvec.c\n>> > @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>> >  {\n>> >  \tif (idx + len > array->nr)\n>> >  \t\tBUG(\"range outside of array boundary\");\n>> > -\tif (replacement_len > len)\n>> > +\tif (replacement_len > len) {\n>> > +\t\tif (array->v == empty_strvec)\n>> > +\t\t\tarray->v = NULL;\n>> >  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>> >  \t\t\t   array->alloc);\n>> > +\t}\n>> >  \tfor (size_t i = 0; i < len; i++)\n>> >  \t\tfree((char *)array->v[idx + i]);\n>> > -\tif (replacement_len != len) {\n>> > +\tif ((replacement_len != len) && array->nr)\n>> >  \t\tmemmove(array->v + idx + replacement_len, array->v + idx + len,\n>> >  \t\t\t(array->nr - idx - len + 1) * sizeof(char *));\n>> > -\t\tarray->nr += (replacement_len - len);\n>> > -\t}\n>> > +\tarray->nr += replacement_len - len;\n>>\n>> Why is this second block of changes needed? Will array-nr ever be 0 when\n>> we reach here?\n>\n> I'm not sure I understand your questions.\n>\n> At that point, `array->nr` is the initial number of entries in the\n> vector.  It can be 0 when `strvec_splice()` is applied to an empty\n> vector.\n>\n> We are moving the line where we update \"array->nr\" outside the `if`\n> block because we want to do it even when we are not moving existing\n> entries.  Again, this happens when `strvec_splice()` is applied to an\n> empty vector.\n>\n\nAh. I was considering that ALLOC_GROW would update `array->nr`, but it\ndoesn't. So you're right.\n\n> Finally, we don't mind too much (or value more the simplicity) of the\n> now unconditional update of \"array->nr\" because a clever compiler will\n> give us the third arm of the if: \"else -> do nothing\".  When\n> `replacement_len == len` => \"array->nr += 0\" => do nothing.\n>\n\nIndeed.\n\nThanks\n"},{"id":"508843","messageId":"xmqqy10pprnp.fsf@gitster.g","threadId":"62569","inReplyTo":"xmqqser33ga6.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T01:32:42Z","receivedAt":"2024-12-09T01:32:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Rubén Justo <rjusto@gmail.com> writes:\n>\n>>> ...\n>>> Sorry.  I'll re-roll later today.\n>\n> No need to say \"sorry\".  Thanks for quickly reacting and starting to\n> work on it.\n\nAny progress?\n\nThanks.\n"},{"id":"508844","messageId":"xmqqttbdprj0.fsf@gitster.g","threadId":"62569","inReplyTo":"xmqqy10pprnp.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T01:35:31Z","receivedAt":"2024-12-09T01:35:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Rubén Justo <rjusto@gmail.com> writes:\n>>\n>>>> ...\n>>>> Sorry.  I'll re-roll later today.\n>>\n>> No need to say \"sorry\".  Thanks for quickly reacting and starting to\n>> work on it.\n>\n> Any progress?\n>\n> Thanks.\n\nSorry, you did send and I did queue v3.\n\n"},{"id":"508845","messageId":"xmqqikrtpqkb.fsf@gitster.g","threadId":"62569","inReplyTo":"xmqqttbdprj0.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T01:56:20Z","receivedAt":"2024-12-09T01:56:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Rubén Justo <rjusto@gmail.com> writes:\n>>>\n>>>>> ...\n>>>>> Sorry.  I'll re-roll later today.\n>>>\n>>> No need to say \"sorry\".  Thanks for quickly reacting and starting to\n>>> work on it.\n>>\n>> Any progress?\n>>\n>> Thanks.\n>\n> Sorry, you did send and I did queue v3.\n\n... and it seems to be causing problems, I didn't look very deep,\nbut it looks similar to what I reported for the earlier round.\n\nTAP version 13\n# start of suite 1: ctype\nok 1 - ctype::isspace\nok 2 - ctype::isdigit\nok 3 - ctype::isalpha\nok 4 - ctype::isalnum\nok 5 - ctype::is_glob_special\nok 6 - ctype::is_regex_special\nok 7 - ctype::is_pathspec_magic\nok 8 - ctype::isascii\nok 9 - ctype::islower\nok 10 - ctype::isupper\nok 11 - ctype::iscntrl\nok 12 - ctype::ispunct\nok 13 - ctype::isxdigit\nok 14 - ctype::isprint\n# start of suite 2: strvec\nok 15 - strvec::init\nok 16 - strvec::dynamic_init\nok 17 - strvec::clear\nok 18 - strvec::push\nok 19 - strvec::pushf\nok 20 - strvec::pushl\nok 21 - strvec::pushv\nnot ok 22 - strvec::splice_just_initialized_strvec\n    ---\n    reason: |\n      String mismatch: (&vec)->v[i] != expect[i]\n      'bar' != '(null)'\n    at:\n      file: 't/unit-tests/strvec.c'\n      line: 97\n      function: 'test_strvec__splice_just_initialized_strvec'\n    ---\nok 23 - strvec::splice_with_same_size_replacement\nok 24 - strvec::splice_with_smaller_replacement\nok 25 - strvec::splice_with_bigger_replacement\nok 26 - strvec::splice_with_empty_replacement\nok 27 - strvec::splice_with_empty_original\nok 28 - strvec::splice_at_tail\nok 29 - strvec::replace_at_head\nok 30 - strvec::replace_at_tail\nok 31 - strvec::replace_in_between\nok 32 - strvec::replace_with_substring\nok 33 - strvec::remove_at_head\nok 34 - strvec::remove_at_tail\nok 35 - strvec::remove_in_between\nok 36 - strvec::pop_empty_array\nok 37 - strvec::pop_non_empty_array\nok 38 - strvec::split_empty_string\nok 39 - strvec::split_single_item\nok 40 - strvec::split_multiple_items\nok 41 - strvec::split_whitespace_only\nok 42 - strvec::split_multiple_consecutive_whitespaces\nok 43 - strvec::detach\n\n=================================================================\n==2199597==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n    #0 0x556696842825 in __interceptor_realloc (/home/gitster/w/git.git/t/unit-tests/bin/unit-tests+0x67825) (BuildId: 408260ac1cf86eb6b93dfb7851633403d20c9aef)\n    #1 0x55669691c87d in xrealloc /home/gitster/w/git.git/wrapper.c:137:8\n    #2 0x5566968ebd2f in strvec_splice /home/gitster/w/git.git/strvec.c:67:3\n    #3 0x556696846c1d in test_strvec__splice_just_initialized_strvec /home/gitster/w/git.git/t/unit-tests/strvec.c:96:2\n    #4 0x55669684c1bb in clar_run_test /home/gitster/w/git.git/t/unit-tests/clar/clar.c:307:3\n    #5 0x55669684a772 in clar_run_suite /home/gitster/w/git.git/t/unit-tests/clar/clar.c:403:3\n    #6 0x55669684a471 in clar_test_run /home/gitster/w/git.git/t/unit-tests/clar/clar.c:598:4\n    #7 0x55669684ac2f in clar_test /home/gitster/w/git.git/t/unit-tests/clar/clar.c:642:11\n    #8 0x55669684d78c in cmd_main /home/gitster/w/git.git/t/unit-tests/unit-test.c:42:8\n    #9 0x55669684d8fd in main /home/gitster/w/git.git/common-main.c:64:11\n    #10 0x7f85891ebc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\nDirect leak of 48 byte(s) in 1 object(s) allocated from:\n    #0 0x556696842640 in __interceptor_calloc (/home/gitster/w/git.git/t/unit-tests/bin/unit-tests+0x67640) (BuildId: 408260ac1cf86eb6b93dfb7851633403d20c9aef)\n    #1 0x55669684ad3e in clar__fail /home/gitster/w/git.git/t/unit-tests/clar/clar.c:676:29\n    #2 0x55669684bf45 in clar__assert_equal /home/gitster/w/git.git/t/unit-tests/clar/clar.c:829:3\n    #3 0x556696846db6 in test_strvec__splice_just_initialized_strvec /home/gitster/w/git.git/t/unit-tests/strvec.c:97:2\n    #4 0x55669684c1bb in clar_run_test /home/gitster/w/git.git/t/unit-tests/clar/clar.c:307:3\n    #5 0x55669684a772 in clar_run_suite /home/gitster/w/git.git/t/unit-tests/clar/clar.c:403:3\n    #6 0x55669684a471 in clar_test_run /home/gitster/w/git.git/t/unit-tests/clar/clar.c:598:4\n    #7 0x55669684ac2f in clar_test /home/gitster/w/git.git/t/unit-tests/clar/clar.c:642:11\n    #8 0x55669684d78c in cmd_main /home/gitster/w/git.git/t/unit-tests/unit-test.c:42:8\n    #9 0x55669684d8fd in main /home/gitster/w/git.git/common-main.c:64:11\n    #10 0x7f85891ebc89 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\nIndirect leak of 18 byte(s) in 1 object(s) allocated from:\n    #0 0x5566968423c6 in __interceptor_malloc (/home/gitster/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 408260ac1cf86eb6b93dfb7851633403d20c9aef)\n    #1 0x7f85892644f9 in strdup string/strdup.c:42:15\n    #2 0x296c6c756e28271f  (<unknown module>)\n\nIndirect leak of 4 byte(s) in 1 object(s) allocated from:\n    #0 0x5566968423c6 in __interceptor_malloc (/home/gitster/w/git.git/t/unit-tests/bin/unit-tests+0x673c6) (BuildId: 408260ac1cf86eb6b93dfb7851633403d20c9aef)\n    #1 0x7f85892644f9 in strdup string/strdup.c:42:15\n\nSUMMARY: LeakSanitizer: 262 byte(s) leaked in 4 allocation(s).\n"},{"id":"508846","messageId":"20241209021556.GA1293399@coredump.intra.peff.net","threadId":"62569","inReplyTo":"xmqqikrtpqkb.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-09T02:15:56Z","receivedAt":"2024-12-09T02:15:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 09, 2024 at 10:56:20AM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >>\n> >>> Rubén Justo <rjusto@gmail.com> writes:\n> >>>\n> >>>>> ...\n> >>>>> Sorry.  I'll re-roll later today.\n> >>>\n> >>> No need to say \"sorry\".  Thanks for quickly reacting and starting to\n> >>> work on it.\n> >>\n> >> Any progress?\n> >>\n> >> Thanks.\n> >\n> > Sorry, you did send and I did queue v3.\n> \n> ... and it seems to be causing problems, I didn't look very deep,\n> but it looks similar to what I reported for the earlier round.\n\nI think it is this off-by-one:\n\ndiff --git a/strvec.c b/strvec.c\nindex 62283fcef2..d67596e571 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -66,7 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n \t\t\tarray->v = NULL;\n \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n \t\t\t   array->alloc);\n-\t\tarray->v[array->nr + (replacement_len - len) + 1] = NULL;\n+\t\tarray->v[array->nr + (replacement_len - len)] = NULL;\n \t}\n \tfor (size_t i = 0; i < len; i++)\n \t\tfree((char *)array->v[idx + i]);\n\nWe allocate with \"+1\" to account for the NULL, but when we index to\nassign the slot, we count from 0.\n\nOr more concretely for the test case, we are adding 1 replacement item\nto a 0-element array, and the result will have 1 item. So we allocate\n2 slots, and slot 1 is the NULL.\n\n-Peff\n"},{"id":"508848","messageId":"xmqq5xntpaya.fsf@gitster.g","threadId":"62569","inReplyTo":"20241209021556.GA1293399@coredump.intra.peff.net","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T07:33:33Z","receivedAt":"2024-12-09T07:33:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think it is this off-by-one:\n>\n> diff --git a/strvec.c b/strvec.c\n> index 62283fcef2..d67596e571 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -66,7 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n>  \t\t\tarray->v = NULL;\n>  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n>  \t\t\t   array->alloc);\n> -\t\tarray->v[array->nr + (replacement_len - len) + 1] = NULL;\n> +\t\tarray->v[array->nr + (replacement_len - len)] = NULL;\n>  \t}\n>  \tfor (size_t i = 0; i < len; i++)\n>  \t\tfree((char *)array->v[idx + i]);\n>\n> We allocate with \"+1\" to account for the NULL, but when we index to\n> assign the slot, we count from 0.\n\nAh, of course.  Usually v[len] is what you never touch (because\n0..(len-1) are the valid index into an array of length len), unless\nthe array has a sentinel at the end, in which case you have the\nsentinel there.  v[len + 1] would obviously be out of bounds.\n\n> Or more concretely for the test case, we are adding 1 replacement item\n> to a 0-element array, and the result will have 1 item. So we allocate\n> 2 slots, and slot 1 is the NULL.\n\nThanks.\n"},{"id":"508888","messageId":"9b46736b-3303-4837-b2ae-72757a0bd60b@gmail.com","threadId":"62569","inReplyTo":"xmqq5xntpaya.fsf@gitster.g","subject":"Re: [PATCH v2] strvec: `strvec_splice()` to a statically initialized vector","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-09T22:42:33Z","receivedAt":"2024-12-09T22:42:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Dec 09, 2024 at 04:33:33PM +0900, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > I think it is this off-by-one:\n> >\n> > diff --git a/strvec.c b/strvec.c\n> > index 62283fcef2..d67596e571 100644\n> > --- a/strvec.c\n> > +++ b/strvec.c\n> > @@ -66,7 +66,7 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,\n> >  \t\t\tarray->v = NULL;\n> >  \t\tALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,\n> >  \t\t\t   array->alloc);\n> > -\t\tarray->v[array->nr + (replacement_len - len) + 1] = NULL;\n> > +\t\tarray->v[array->nr + (replacement_len - len)] = NULL;\n> >  \t}\n> >  \tfor (size_t i = 0; i < len; i++)\n> >  \t\tfree((char *)array->v[idx + i]);\n\nYes, of course that's the right fix.  I have just seen what has been\nqueued.  Thank you Peff for the quick response.\n\nJust in case it get lost in a junk folder, I just sent you a message\nwithout cc'ing the list.\n"}]}