threads / patch / 62225

patchunit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack

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

## tl;dr

3 messages between Oct 1, 2024 and Oct 1, 2024. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Chandra Pratap· Oct 1, 2024, 17:05 UTC · lore

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(-)

Show changes to t/unit-tests/t-reftable-stack.c +2 −4

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

Junio C Hamano· Oct 1, 2024, 19:22 UTC · re: Chandra Pratap · lore

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

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?

Show 14 quoted lines

> 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);

Show 6 quoted lines

>  		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");

Eric Sunshine· Oct 1, 2024, 19:32 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 1, 2024 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> 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);