From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 25 Sep 2025 05:39:14 GMT
Subject: Re: [PATCH v2 6/6] builtin/repo: add progress meter for stats
Message-ID: <aNTVgnEGE2qUwZwK@pks.im>
In-Reply-To: <20250924212426.2930029-7-jltobler@gmail.com>

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`.

> @@ -365,13 +372,24 @@ static void stats_count_references(struct ref_stats *stats, struct ref_array *re
>  		default:
>  			BUG("unexpected reference type");
>  		}
> +
> +		display_progress(progress, i + 1);
>  	}
> +
> +	stop_progress(&progress);
>  }
>  
> +struct count_objects_data {
> +	struct object_stats *stats;
> +	struct progress *progress;
> +};
> +
>  static int count_objects(const char *path UNUSED, struct oid_array *oids,
>  			 enum object_type type, void *cb_data)
>  {
> -	struct object_stats *stats = cb_data;
> +	struct count_objects_data *data = cb_data;
> +	struct object_stats *stats = data->stats;
> +	size_t object_count;
>  
>  	switch (type) {
>  	case OBJ_TAG:
> @@ -390,17 +408,24 @@ static int count_objects(const char *path UNUSED, struct oid_array *oids,
>  		BUG("invalid object type");
>  	}
>  
> +	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;
    }

> @@ -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/

> @@ -432,10 +461,12 @@ static int cmd_repo_stats(int argc, const char **argv, const char *prefix,
>  	struct repo_stats stats = { 0 };
>  	struct ref_array refs = { 0 };
>  	struct rev_info revs;
> +	int show_progress = -1;
>  	struct option options[] = {
>  		OPT_CALLBACK_F(0, "format", &format, N_("format"),
>  			       N_("output format"),
>  			       PARSE_OPT_NONEG, parse_format_cb),
> +		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
>  		OPT_END()
>  	};
>  
> @@ -444,8 +475,11 @@ static int cmd_repo_stats(int argc, const char **argv, const char *prefix,
>  	if (filter_refs(&refs, &filter, FILTER_REFS_REGULAR))
>  		die(_("unable to filter refs"));
>  
> -	stats_count_references(&stats.refs, &refs);
> -	stats_count_objects(&stats.objects, &refs, &revs);
> +	if (show_progress < 0)
> +		show_progress = isatty(2);

Makes sense.

> +	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?

Patrick