From: Justin Tobler <jltobler@gmail.com>
Date: Tue, 23 Sep 2025 15:26:46 GMT
Subject: Re: [PATCH 3/4] builtin/repo: add keyvalue format for stats
Message-ID: <5kwuxyttcnhe57hej3xfmaunbjmpu7og52ft2bb4uawjqq4lvb@s3zub5742v4e>
In-Reply-To: <aNJ8DIPI0v6Jhn5C@pks.im>

On 25/09/23 12:53PM, Patrick Steinhardt wrote:
> 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.

That's fair. Will update.

> > @@ -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()`.

I adapt this same function in the next version to also print the NUL
format which is why I kept it generic. The NUL format is really still
just a key-format just with different delimiters. I'll change this in
the next version.

[snip]
> > @@ -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.

At this point, the only other valid option was handled above. This would
leave only invalid options. I think in the next version I'm just going
to combine patches 3 and 4 though to avoid this issue altogether though.

-Justin