[PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack

[PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack

[Chandra Pratap]

A char buffer is used to hold refname values as formatted strings
in the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c.
This can be replaced with a single call to xstrfmt() making the test
conciser.

Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-stack.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 31d563d992..2d7cfbf8aa 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -523,14 +523,12 @@ static void t_reftable_stack_add(void)
 	check(!err);
 
 	for (i = 0; i < N; i++) {
-		char buf[256];
-		snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i);
-		refs[i].refname = xstrdup(buf);
+		refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
 		refs[i].update_index = i + 1;
 		refs[i].value_type = REFTABLE_REF_VAL1;
 		t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);
 
-		logs[i].refname = xstrdup(buf);
+		logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
 		logs[i].update_index = N + i + 1;
 		logs[i].value_type = REFTABLE_LOG_UPDATE;
 		logs[i].value.update.email = xstrdup("identity@invalid");
-- 
2.45.GIT

Re: [PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack

[Junio C Hamano]

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> A char buffer is used to hold refname values as formatted strings
> in the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c.
> This can be replaced with a single call to xstrfmt() making the test
> conciser.

It may make the test more concise, but would we now need to worry
about leaking .refname?

It turns out that we do not have to, as we were already storing the
result of xstrdup() to .refname, so we must have been freeing them
already (or we are not making existing leak worse, if .refname were
leaking).

I've heard some noises about using our helper functions in tests, in
that a buggy helper function of ours would interfere with testing
the thing(s) we truly want to test, but we have been using xstrdup()
and replacing it with xstrfmt(), so it is not like we are making
things worse in that regard.  Not that I entirely buy the "don't use
git to test git" argument.

One thing that this worsens is that we now have two copies of the
literal "branch%02"PRIuMAX string.  If we ever want to change one of
them, we must remember to change the other to match.

Perhaps with another constant, this patch would become perfect, like
this?

> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  t/unit-tests/t-reftable-stack.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
> index 31d563d992..2d7cfbf8aa 100644
> --- a/t/unit-tests/t-reftable-stack.c
> +++ b/t/unit-tests/t-reftable-stack.c
> @@ -523,14 +523,12 @@ static void t_reftable_stack_add(void)
>  	check(!err);
>  
>  	for (i = 0; i < N; i++) {
> -		char buf[256];

+		static const char fmt[] = "branch%02"PRIuMAX;

> -		snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i);
> -		refs[i].refname = xstrdup(buf);
> +		refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);

+		refs[i].refname = xstrfmt(fmt, (uintmax_t)i);

>  		refs[i].update_index = i + 1;
>  		refs[i].value_type = REFTABLE_REF_VAL1;
>  		t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);
>  
> -		logs[i].refname = xstrdup(buf);
> +		logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);

+		logs[i].refname = xstrfmt(fmt, (uintmax_t)i);

>  		logs[i].update_index = N + i + 1;
>  		logs[i].value_type = REFTABLE_LOG_UPDATE;
>  		logs[i].value.update.email = xstrdup("identity@invalid");

Re: [PATCH] unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack

[Eric Sunshine]

On Tue, Oct 1, 2024 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> One thing that this worsens is that we now have two copies of the
> literal "branch%02"PRIuMAX string.  If we ever want to change one of
> them, we must remember to change the other to match.
>
> Perhaps with another constant, this patch would become perfect, like
> this?
>
> +               static const char fmt[] = "branch%02"PRIuMAX;
>
> > -             snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i);
> > -             refs[i].refname = xstrdup(buf);
> > +             refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
>
> +               refs[i].refname = xstrfmt(fmt, (uintmax_t)i);
>
> >               refs[i].update_index = i + 1;
> >               refs[i].value_type = REFTABLE_REF_VAL1;
> >               t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID);
> >
> > -             logs[i].refname = xstrdup(buf);
> > +             logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
>
> +               logs[i].refname = xstrfmt(fmt, (uintmax_t)i);

Even simpler would be merely to xstrdup() the string which was already
formatted by xstrfmt(), I would think.

    refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i);
    ...
    logs[i].refname = xstrdup(refs[i].refname);