{"thread":{"id":"62225","subject":"[PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack","startedAt":"2024-10-01T17:07:16Z","lastAt":"2024-10-01T19:32:54Z","messageCount":3,"participants":["Chandra Pratap","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503846","messageId":"20241001170629.7768-1-chandrapratap3519@gmail.com","threadId":"62225","inReplyTo":null,"subject":"[PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2024-10-01T17:05:55Z","receivedAt":"2024-10-01T17:07:16Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"A char buffer is used to hold refname values as formatted strings\nin the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c.\nThis can be replaced with a single call to xstrfmt() making the test\nconciser.\n\nSigned-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n---\n t/unit-tests/t-reftable-stack.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 31d563d992..2d7cfbf8aa 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -523,14 +523,12 @@ static void t_reftable_stack_add(void)\n \tcheck(!err);\n \n \tfor (i = 0; i < N; i++) {\n-\t\tchar buf[256];\n-\t\tsnprintf(buf, sizeof(buf), \"branch%02\"PRIuMAX, (uintmax_t)i);\n-\t\trefs[i].refname = xstrdup(buf);\n+\t\trefs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\trefs[i].update_index = i + 1;\n \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n \t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n \n-\t\tlogs[i].refname = xstrdup(buf);\n+\t\tlogs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n \t\tlogs[i].update_index = N + i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n-- \n2.45.GIT\n\n"},{"id":"503864","messageId":"xmqqo743vd9q.fsf@gitster.g","threadId":"62225","inReplyTo":"20241001170629.7768-1-chandrapratap3519@gmail.com","subject":"Re: [PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T19:22:57Z","receivedAt":"2024-10-01T19:23:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chandra Pratap <chandrapratap3519@gmail.com> writes:\n\n> A char buffer is used to hold refname values as formatted strings\n> in the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c.\n> This can be replaced with a single call to xstrfmt() making the test\n> conciser.\n\nIt may make the test more concise, but would we now need to worry\nabout leaking .refname?\n\nIt turns out that we do not have to, as we were already storing the\nresult of xstrdup() to .refname, so we must have been freeing them\nalready (or we are not making existing leak worse, if .refname were\nleaking).\n\nI've heard some noises about using our helper functions in tests, in\nthat a buggy helper function of ours would interfere with testing\nthe thing(s) we truly want to test, but we have been using xstrdup()\nand replacing it with xstrfmt(), so it is not like we are making\nthings worse in that regard.  Not that I entirely buy the \"don't use\ngit to test git\" argument.\n\nOne thing that this worsens is that we now have two copies of the\nliteral \"branch%02\"PRIuMAX string.  If we ever want to change one of\nthem, we must remember to change the other to match.\n\nPerhaps with another constant, this patch would become perfect, like\nthis?\n\n> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>\n> ---\n>  t/unit-tests/t-reftable-stack.c | 6 ++----\n>  1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index 31d563d992..2d7cfbf8aa 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n> @@ -523,14 +523,12 @@ static void t_reftable_stack_add(void)\n>  \tcheck(!err);\n>  \n>  \tfor (i = 0; i < N; i++) {\n> -\t\tchar buf[256];\n\n+\t\tstatic const char fmt[] = \"branch%02\"PRIuMAX;\n\n> -\t\tsnprintf(buf, sizeof(buf), \"branch%02\"PRIuMAX, (uintmax_t)i);\n> -\t\trefs[i].refname = xstrdup(buf);\n> +\t\trefs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n\n+\t\trefs[i].refname = xstrfmt(fmt, (uintmax_t)i);\n\n>  \t\trefs[i].update_index = i + 1;\n>  \t\trefs[i].value_type = REFTABLE_REF_VAL1;\n>  \t\tt_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n>  \n> -\t\tlogs[i].refname = xstrdup(buf);\n> +\t\tlogs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n\n+\t\tlogs[i].refname = xstrfmt(fmt, (uintmax_t)i);\n\n>  \t\tlogs[i].update_index = N + i + 1;\n>  \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n>  \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n"},{"id":"503868","messageId":"CAPig+cQGz3TFGfmLwdpYHNMOqKbqXZpCRMj=bT3pvaZD=oyVSQ@mail.gmail.com","threadId":"62225","inReplyTo":"xmqqo743vd9q.fsf@gitster.g","subject":"Re: [PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-01T19:32:42Z","receivedAt":"2024-10-01T19:32:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 1, 2024 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n> One thing that this worsens is that we now have two copies of the\n> literal \"branch%02\"PRIuMAX string.  If we ever want to change one of\n> them, we must remember to change the other to match.\n>\n> Perhaps with another constant, this patch would become perfect, like\n> this?\n>\n> +               static const char fmt[] = \"branch%02\"PRIuMAX;\n>\n> > -             snprintf(buf, sizeof(buf), \"branch%02\"PRIuMAX, (uintmax_t)i);\n> > -             refs[i].refname = xstrdup(buf);\n> > +             refs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n>\n> +               refs[i].refname = xstrfmt(fmt, (uintmax_t)i);\n>\n> >               refs[i].update_index = i + 1;\n> >               refs[i].value_type = REFTABLE_REF_VAL1;\n> >               t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);\n> >\n> > -             logs[i].refname = xstrdup(buf);\n> > +             logs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n>\n> +               logs[i].refname = xstrfmt(fmt, (uintmax_t)i);\n\nEven simpler would be merely to xstrdup() the string which was already\nformatted by xstrfmt(), I would think.\n\n    refs[i].refname = xstrfmt(\"branch%02\"PRIuMAX, (uintmax_t)i);\n    ...\n    logs[i].refname = xstrdup(refs[i].refname);\n"}]}