Re: [PATCH v2 5/6] builtin/repo: add keyvalue and nul format for stats

> On Wed, Sep 24, 2025 at 04:24:25PM -0500, Justin Tobler wrote:
> > diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> > index 0b8d74ed3e..db21b75522 100644
> > --- a/Documentation/git-repo.adoc
> > +++ b/Documentation/git-repo.adoc
> > @@ -52,7 +52,26 @@ supported:
> >  * Reachable object counts categorized by type
> >  
> >  +
> > -The table output format may change and is not intended for machine parsing.
> > +The output format can be chosen through the flag `--format`. Three formats are
> > +supported:
> > ++
> > +`table`:::
> > +	Outputs repository stats in a human-friendly table and is used by
> > +	default. This format may change and is not intended for machine
> > +	parsing.
> 
> Let's mention that this is the default format.

> > +`keyvalue`:::
> > +	Each line of output contains a key-value pair for a repository stat.
> > +	The '=' character is used to delimit between the key and the value.
> > +	Values containing "unusual" characters are quoted as explained for the
> > +	configuration variable `core.quotePath` (see linkgit:git-config[1]).
> 
> In the current state there is never any quoting, so this statement here
> is a bit misleading. Should we maybe drop that part?

> > +static void stats_keyvalue_print(struct repo_stats *stats, char key_delim,
> > +				 char value_delim)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +
> > +	strbuf_addf(&buf, "references.branches.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->refs.branches, value_delim);
> > +	strbuf_addf(&buf, "references.tags.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->refs.tags, value_delim);
> > +	strbuf_addf(&buf, "references.remotes.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->refs.remotes, value_delim);
> > +	strbuf_addf(&buf, "references.others.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->refs.others, value_delim);
> > +
> > +	strbuf_addf(&buf, "objects.commits.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->objects.commits, value_delim);
> > +	strbuf_addf(&buf, "objects.trees.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->objects.trees, value_delim);
> > +	strbuf_addf(&buf, "objects.blobs.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->objects.blobs, value_delim);
> > +	strbuf_addf(&buf, "objects.tags.count%c%" PRIuMAX "%c",
> > +		    key_delim, (uintmax_t)stats->objects.tags, value_delim);
> > +
> > +	fwrite(buf.buf, sizeof(char), buf.len, stdout);
> > +	strbuf_release(&buf);
> > +}
> 
> Same question here regarding the buffering. Can't we print to stdout
> directly, or is there a reason not to?

> > @@ -389,17 +421,25 @@ static void stats_count_objects(struct object_stats *stats,
> >  	path_walk_info_clear(&info);
> >  }
> >  
> > -static int cmd_repo_stats(int argc UNUSED, const char **argv UNUSED,
> > -			  const char *prefix, struct repository *repo)
> > +static int cmd_repo_stats(int argc, const char **argv, const char *prefix,
> > +			  struct repository *repo)
> >  {
> >  	struct ref_filter filter = REF_FILTER_INIT;
> >  	struct stats_table table = {
> >  		.rows = STRING_LIST_INIT_DUP,
> >  	};
> > +	enum output_format format = FORMAT_TABLE;
> >  	struct repo_stats stats = { 0 };
> >  	struct ref_array refs = { 0 };
> >  	struct rev_info revs;
> > +	struct option options[] = {
> > +		OPT_CALLBACK_F(0, "format", &format, N_("format"),
> > +			       N_("output format"),
> > +			       PARSE_OPT_NONEG, parse_format_cb),
> > +		OPT_END()
> > +	};
> >  
> > +	parse_options(argc, argv, prefix, options, repo_usage, 0);
> 
> I think it would be sensible to introduce this call to `parse_options()`
> right in the first commit that wires up the new subcommand. If we don't
> do that we otherwise accept arbitrary arguments without raising any
> error, and neither do we know to output help.
> 
> So we should move the addition to a previous commit and probably do the
> following:
> 
>         argc = parse_options(...);
>         if (argc)
>             usagef("too many arguments");

> > @@ -407,8 +447,20 @@ static int cmd_repo_stats(int argc UNUSED, const char **argv UNUSED,
> >  	stats_count_references(&stats.refs, &refs);
> >  	stats_count_objects(&stats.objects, &refs, &revs);
> >  
> > -	stats_table_setup(&table, &stats);
> > -	stats_table_print(&table);
> > +	switch (format) {
> > +	case FORMAT_TABLE:
> > +		stats_table_setup(&table, &stats);
> > +		stats_table_print(&table);
> > +		break;
> > +	case FORMAT_KEYVALUE:
> > +		stats_keyvalue_print(&stats, '=', '\n');
> > +		break;
> > +	case FORMAT_NUL_TERMINATED:
> > +		stats_keyvalue_print(&stats, '\n', '\0');
> > +		break;
> 
> This reads much nicer now. The newline as key-value delimiter is a
> curious choice, but you simply do what we already do in `git repo info`.

> > diff --git a/t/t1901-repo-stats.sh b/t/t1901-repo-stats.sh
> > index 315b9e1767..d2c1b6e307 100755
> > --- a/t/t1901-repo-stats.sh
> > +++ b/t/t1901-repo-stats.sh
> > @@ -73,4 +73,37 @@ test_expect_success 'repository with references and objects' '
> >  	)
> >  '
> >  
> > +test_expect_success 'repository stats with keyvalue and nul format' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		test_commit_bulk 42 &&
> > +		git tag -a foo -m bar &&
> > +
> > +		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
> > +
> > +		git repo stats --format=keyvalue >out 2>err &&
> > +
> > +		test_cmp expect out &&
> > +		test_line_count = 0 err &&
> > +
> > +		# Replace key and value delimiters for nul format.
> > +		tr "\n" "\0" <expect | tr "=" "\n" >expect_null &&
> 
> You can do this without the pipe:
> 
>     tr "\n=" "\0\n" <expect >expect_nul
> 
> Also, let's call the file `expect_nul` (with a single 'l') to match the
> format.