Re: [PATCH v4 4/7] builtin/repo: introduce stats subcommand

> Justin Tobler <jltobler@gmail.com> writes:
> 
> > The shape of a repository's history can have huge impacts on the
> > performance and health of the repository itself. Currently, Git lacks a
> > means to surface key stats/information regarding the shape of a
> > repository via a single command.
> 
> Talking about "shape of a repository's *history*" may negatively
> affect your goal here.  If a project is overly mergy with many
> octopus merges, it would have huge impacts on the performance to run
> "git bisect" over its history, so it may be interesting to know the
> ratio of the merge commits in the total commits, and also the
> average number of merge parents.  But after you obtain such numbers,
> you cannot do anything about it, as you cannot afford to rewrite its
> history only to improve the "performance and health".

> And that is what makes "key stats" relative to your goal.  If your
> goal is to give stats on the things you can control (e.g., how long
> a typical delta chain is, how many loose objects there are that can
> be moved to a packfile, how small would your object database would
> become if you prune all the unreachable objects), that would cut off
> some stats that may still be interesting but may not contribute to
> address "huge impacts on the performance and health".

> With Devil's advocate hat on, a single command that gives a set of
> stats that are "key" to a goal of a single use case may not be as
> useful as a collection of commands, each of which gives stats on one
> aspect of the repository, that can be combined to help you address
> various different goals.

> > To allow users to more readily identify potential issues for a
> > repository, introduce the "stats" subcommand in git-repo(1) to output
> > stats for the repository that may be of interest to users. The goal of
> > this subcommand is to eventually provide similar functionality to
> > git-sizer(1), but natively in Git.
> 
> So, it is needless to say that the kind of "stats" obtained by such
> a single tool needs to be chosen carefully, but more importantly,
> its output should give users actionable output, as whoever designed
> such a tool and chose what "key stats" are has a clear idea on
> various aspects of repository.  "stats" measure the health of the
> repository against certain yardstick, but it should come with a
> clear instruction to make use of that measurement.  The tool may say
> "the stats indicate that you have commits that touch too many paths
> at the same time".  The users need to be know what consequence of
> that finding is, and what they can do about it.

> For example, what would the user do with the new knowledge that the
> repository has 100x as many local branches as there are
> remote-tracking branches?  Without breaking down these numerous
> local branches into those that are still used in active development
> (hint: peek into their reflog), kept as historical landmarks, past
> development that has already been merged (hint: "git branch --list
> --merged origin/master"), or abandoned cruft that hasn't been
> touched with some changes that are not merged anywhere, the users
> would not know what to do.

> > +`stats`::
> > +	Retrieve statistics about the current repository. The following kinds
> > +	of information are reported:
> > ++
> > +* Reference counts categorized by type
> > +
> > ++
> > +The table output format may change and is not intended for machine parsing.
> 
> Do we eventually want to give another format that is intended for
> machine parsing?

> > +static void stats_table_print(const struct stats_table *table)
> > +{
> > +	const char *name_col_title = _("Repository stats");
> > +	const char *value_col_title = _("Value");
> > +	size_t name_title_len = utf8_strwidth(name_col_title);
> > +	size_t value_title_len = utf8_strwidth(value_col_title);
> > +	struct string_list_item *item;
> > +	int name_col_width;
> > +	int value_col_width;
> > +
> > +	name_col_width = cast_size_t_to_int(
> > +		max_size_t(table->name_col_width, name_title_len));
> > +	value_col_width = cast_size_t_to_int(
> > +		max_size_t(table->value_col_width, value_title_len));
> 
> If table->name_col_width and table->value_col_width were int to
> begin with, none of these casts would have been necessary.  Aren't
> we overusing size_t to count things that are not memory allocations?

> > +	printf("| %-*s | %-*s |\n", name_col_width, name_col_title,
> > +	       value_col_width, value_col_title);
> > +	printf("| ");
> > +	for (int i = 0; i < name_col_width; i++)
> > +		putchar('-');
> > +	printf(" | ");
> > +	for (int i = 0; i < value_col_width; i++)
> > +		putchar('-');
> > +	printf(" |\n");
> 
> I wonder if people want to use unicode "Box Drawing" block and other
> fancier things, as we assume utf8 for names and values, in which
> case these printf would need to be "translatable", but locale
> administrators should not have more say than others what kind of
> line drawing elements are to be used, so perhaps the above is good
> enough at least for now.