From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 23 Sep 2025 10:53:05 GMT
Subject: Re: [PATCH 4/4] builtin/repo: add nul format for stats
Message-ID: <aNJ8EUT_QVCqfFo7@pks.im>
In-Reply-To: <20250923025700.3046260-5-jltobler@gmail.com>

On Mon, Sep 22, 2025 at 09:57:00PM -0500, Justin Tobler wrote:
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 4c16a68e4e..37034e6347 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -291,27 +291,31 @@ static void stats_table_print(struct stats_table *table)
>  	strbuf_release(&buf);
>  }
>  
> -static void stats_print(struct stats *stats)
> +static void stats_print(struct stats *stats, int nul_delim)

Instead of passing a boolean-style option, can't we pass the expected
delimiter directly? Makes the callsite a bit more obvious.

>  {
>  	struct strbuf buf = STRBUF_INIT;
> -
> -	strbuf_addf(&buf, "references.branches.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->refs.branches);
> -	strbuf_addf(&buf, "references.tags.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->refs.tags);
> -	strbuf_addf(&buf, "references.remotes.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->refs.remotes);
> -	strbuf_addf(&buf, "references.others.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->refs.others);
> -
> -	strbuf_addf(&buf, "objects.commits.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->objects.commits);
> -	strbuf_addf(&buf, "objects.trees.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->objects.trees);
> -	strbuf_addf(&buf, "objects.blobs.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->objects.blobs);
> -	strbuf_addf(&buf, "objects.tags.count=%" PRIuMAX "\n",
> -		    (uintmax_t)stats->objects.tags);
> +	char delim = '\n';
> +
> +	if (nul_delim)
> +		delim = '\0';
> +
> +	strbuf_addf(&buf, "references.branches.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->refs.branches, delim);
> +	strbuf_addf(&buf, "references.tags.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->refs.tags, delim);
> +	strbuf_addf(&buf, "references.remotes.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->refs.remotes, delim);
> +	strbuf_addf(&buf, "references.others.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->refs.others, delim);
> +
> +	strbuf_addf(&buf, "objects.commits.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->objects.commits, delim);
> +	strbuf_addf(&buf, "objects.trees.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->objects.trees, delim);
> +	strbuf_addf(&buf, "objects.blobs.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->objects.blobs, delim);
> +	strbuf_addf(&buf, "objects.tags.count=%" PRIuMAX "%c",
> +		    (uintmax_t)stats->objects.tags, delim);
>  
>  	fwrite(buf.buf, sizeof(char), buf.len, stdout);
>  	strbuf_release(&buf);

It's a bit unfortunate we have to rewrite most of the function. I'd
either have the `delim` parameter right from the start or just squash
these two patches together.

> diff --git a/t/t1901-repo-stats.sh b/t/t1901-repo-stats.sh
> index 5bc6d9d5c4..061b2fbbc1 100755
> --- a/t/t1901-repo-stats.sh
> +++ b/t/t1901-repo-stats.sh
> @@ -127,4 +127,31 @@ test_expect_success 'repository stats with keyvalue format' '
>  	)
>  '
>  
> +test_expect_success 'repository stats with nul format' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit_bulk 42 &&
> +		git tag -a foo -m bar &&
> +		git repo stats --format=nul >out 2>err &&
> +
> +		cat >expect <<-EOF &&
> +		references.branches.count=1
> +		references.tags.count=1
> +		references.remotes.count=0
> +		references.others.count=0
> +		objects.commits.count=42
> +		objects.trees.count=42
> +		objects.blobs.count=42
> +		objects.tags.count=1
> +		EOF
> +
> +		tr "\n" "\0" <expect >expect_null &&
> +
> +		test_cmp expect_null out &&
> +		test_line_count = 0 err
> +	)
> +'

We already have a test for the keyvalue format that looks mostly the
same, so we may just as well test both formats in a single test.

Patrick