From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 24 Sep 2025 04:48:50 GMT
Subject: Re: [PATCH 4/4] builtin/repo: add nul format for stats
Message-ID: <aNN4Mk9Nh_mF1dJ6@pks.im>
In-Reply-To: <r2rt5aq64x232gmpj5wzzchut2w2axnkyfzg7wpthwuf2w3j4f@teydueqvon5d>

On Tue, Sep 23, 2025 at 10:33:45AM -0500, Justin Tobler wrote:
> On 25/09/23 12:53PM, Patrick Steinhardt wrote:
> > 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.
> 
> Ya, we could do that here instead. Something I just noticed is that the
> NUL format in `git repo info` also replaces the '=' delimiter with a
> newline. I'm not sure if it would be best to match the same behavior
> here?
> 
> If so, we would have to either pass both delimiters as arguments to the
> function, or just keep the boolean toggle for the mode.

I think being consistent would be nice, yes. I'd personally lean towards
passing both delimiters as arguments in that case. Makes the callsites
way easier to read.

Patrick