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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2024, 19:22 UTC
Message-ID: <xmqqo743vd9q.fsf@gitster.g>
In-Reply-To: <20241001170629.7768-1-chandrapratap3519@gmail.com>

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