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

On Mon, Sep 22, 2025 at 09:56:59PM -0500, Justin Tobler wrote:
> @@ -157,6 +160,8 @@ static int repo_info(int argc, const char **argv, const char *prefix,
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
> +	if (format == FORMAT_TABLE)
> +		die(_("table format not supported"));

We can deduplicate this error message by saying "format '%s' not
supported".

Other than that, let's fail closed and say `if (format !=
FORMAT_KEYVALUE && format != FORMAT_NUL)`. Like this, we won't have to
update the condition every time a new format is added, which is easy to
forget.

> @@ -286,6 +291,32 @@ static void stats_table_print(struct stats_table *table)
>  	strbuf_release(&buf);
>  }
>  
> +static void stats_print(struct stats *stats)

I think it would make sense to call this `stats_keyvalue_print()` to
clearly distinguish it from `stats_table_print()`.

> @@ -359,9 +390,16 @@ static void stats_count_objects(struct object_stats *stats,
>  	path_walk_info_clear(&info);
>  }
>  
> -static int repo_stats(int argc UNUSED, const char **argv UNUSED,
> -		      const char *prefix, struct repository *repo)
> +static int repo_stats(int argc, const char **argv, const char *prefix,
> +		      struct repository *repo)
>  {
> +	enum output_format format = FORMAT_TABLE;
> +	struct option options[] = {
> +		OPT_CALLBACK_F(0, "format", &format, N_("format"),
> +			       N_("output format"),
> +			       PARSE_OPT_NONEG, parse_format_cb),
> +		OPT_END()
> +	};
>  	struct ref_filter filter = REF_FILTER_INIT;
>  	struct strvec ref_patterns = STRVEC_INIT;
>  	struct stats_table table = { 0 };

Nice that we can reuse the callback.

> @@ -369,6 +407,10 @@ static int repo_stats(int argc UNUSED, const char **argv UNUSED,
>  	struct stats stats = { 0 };
>  	struct rev_info revs;
>  
> +	parse_options(argc, argv, prefix, options, repo_usage, 0);
> +	if (format == FORMAT_NUL_TERMINATED)
> +		die(_("nul format not yet supported"));
> +
>  	repo_init_revisions(repo, &revs, prefix);
>  	filter.name_patterns = ref_patterns.v;
>  	filter_refs(&refs, &filter, FILTER_REFS_REGULAR);

Same comment here regarding failing in a closed way.

> @@ -376,8 +418,17 @@ static int 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_print(&stats);
> +		break;
> +	default:
> +		BUG("not a valid output format: %d", format);

Nit: it may be valid, but definitely not supported.

Patrick