From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 25 Sep 2025 13:20:48 GMT
Subject: Re: [PATCH v2 6/6] builtin/repo: add progress meter for stats
Message-ID: <ehjxy2qvwhrtmyvmjv6hu5ej2pslomjn476mlkkodmbxwutel5@yx6an6nj75p4>
In-Reply-To: <aNTVgnEGE2qUwZwK@pks.im>

On 25/09/25 07:39AM, Patrick Steinhardt wrote:
> On Wed, Sep 24, 2025 at 04:24:26PM -0500, Justin Tobler wrote:
> > diff --git a/builtin/repo.c b/builtin/repo.c
> > index fe7d43f78e..fdc8af92dc 100644
> > --- a/builtin/repo.c
> > +++ b/builtin/repo.c
> > @@ -344,8 +345,14 @@ static void stats_keyvalue_print(struct repo_stats *stats, char key_delim,
> >  	strbuf_release(&buf);
> >  }
> >  
> > -static void stats_count_references(struct ref_stats *stats, struct ref_array *refs)
> > +static void stats_count_references(struct ref_stats *stats, struct ref_array *refs,
> > +				   struct repository *repo, int show_progress)
> >  {
> > +	struct progress *progress = NULL;
> > +
> > +	if (show_progress)
> > +		progress = start_progress(repo, _("Counting references"), refs->nr);
> 
> We tend to use `start_delayed_progress()` so that the progress meter is
> not displayed when the action takes less than a second. The delay can be
> disabled in our tests by using `GIT_PROGRESS_DELAY=0`.

Good to know. I'll update in the next version.

[snip]
> > +	object_count = stats->tags + stats->commits + stats->trees + stats->blobs;
> 
> We have this computation in two locations now. Maybe we should
> deduplicate it via something like:
> 
>     static inline size_t stats_get_total_object_count()
>     {
>         return stats->tags + stats->commits + stats->trees + stats->blobs;
>     }

Make sense. Will add.

> 
> > @@ -417,8 +442,12 @@ static void stats_count_objects(struct object_stats *stats,
> >  		}
> >  	}
> >  
> > +	if (show_progress)
> > +		data.progress = start_progress(repo, _("Counting Objects"), 0);
> 
> s/Objects/objects/

Will fix in the next version.

[snip]
> > +	stats_count_references(&stats.refs, &refs, repo, show_progress);
> > +	stats_count_objects(&stats.objects, &refs, &revs, repo, show_progress);
> >  
> >  	switch (format) {
> >  	case FORMAT_TABLE:
> 
> Should our tests be updated to verify that we know to print progress
> depending on whether or not `--progress` is passed?

That seems reasonable to me. I'll add a test for this in the next
version.

-Justin