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

> Justin Tobler <jltobler@gmail.com> writes:
> 
> [snip]
> 
> > diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> > index 209afd1b61..7762329551 100644
> > --- a/Documentation/git-repo.adoc
> > +++ b/Documentation/git-repo.adoc
> > @@ -9,6 +9,7 @@ SYNOPSIS
> >  --------
> >  [synopsis]
> >  git repo info [--format=(keyvalue|nul)] [-z] [<key>...]
> > +git repo stats
> >
> >  DESCRIPTION
> >  -----------
> > @@ -43,6 +44,12 @@ supported:
> >  +
> >  `-z` is an alias for `--format=nul`.
> >
> > +stats::
> > +	Retrieve stats about the current repository. All references in the
> > +	repository are categorized and counted accordingly.
> > ++
> 
> Nit: Should we use bullet points for the information provided? As of
> this commit it is only references, but with time, this will grow, so
> would be nice to have a set of bullet points to understand the different
> information retrieved.

> > +The table output format may change and is not intended for machine parsing.
> > +
> >  INFO KEYS
> >  ---------
> >  In order to obtain a set of values from `git repo info`, you should provide
> > diff --git a/builtin/repo.c b/builtin/repo.c
> > index bbb0966f2d..15899dd74c 100644
> > --- a/builtin/repo.c
> > +++ b/builtin/repo.c
> > @@ -4,12 +4,15 @@
> >  #include "environment.h"
> >  #include "parse-options.h"
> >  #include "quote.h"
> > +#include "ref-filter.h"
> >  #include "refs.h"
> >  #include "strbuf.h"
> > +#include "string-list.h"
> >  #include "shallow.h"
> >
> >  static const char *const repo_usage[] = {
> >  	"git repo info [--format=(keyvalue|nul)] [-z] [<key>...]",
> > +	"git repo stats",
> >  	NULL
> >  };
> >
> > @@ -156,12 +159,160 @@ static int repo_info(int argc, const char **argv, const char *prefix,
> >  	return print_fields(argc, argv, repo, format);
> >  }
> >
> > +struct stats {
> > +	size_t branches;
> > +	size_t remotes;
> > +	size_t tags;
> > +	size_t others;
> 
> Maybe we can use a nested structure here, which would reflect the output
> table. That would be much nicer, as the code would grow, currently we
> know 'others' here refers to references, but with other fields coming
> in, this would no longer be obvious.

> > +};
> > +
> > +struct stats_table {
> > +	struct string_list rows;
> > +
> > +	int name_col_width;
> > +	int value_col_width;
> 
> Can these be negative?

> > +};
> > +
> 
> Nit: Could we add a comment about what this structure is and what it
> holds? I think it is sort of obvious, but would be nice to clarify.

> > +static int repo_stats(int argc UNUSED, const char **argv UNUSED,
> > +		      const char *prefix UNUSED, struct repository *repo UNUSED)
> > +{
> > +	struct ref_filter filter = REF_FILTER_INIT;
> > +	struct strvec ref_patterns = STRVEC_INIT;
> > +	struct stats_table table = { 0 };
> > +	struct ref_array refs = { 0 };
> > +	struct stats stats = { 0 };
> > +
> > +	filter.name_patterns = ref_patterns.v;
> > +	filter_refs(&refs, &filter, FILTER_REFS_REGULAR);
> > +
> 
> I was wondering why we need the filter mechanism here, but seems like
> this is to obtain the type of the reference. This is setup automatically
> by the filter mechanism and so it's okay.
> 
> We could replicate the same using the ref iterator code. This would be a
> little more involved, but would remove the need to store all refs in a
> 'ref_array' and also the need to loop over references twice. But it
> doesn't really matter in this usecase I assume.

> > +	stats_count_references(&stats, &refs);
> > +
> > +	stats_table_setup(&table, &stats);
> > +	stats_table_print(&table);
> > +
> > +	string_list_clear(&table.rows, 1);
> > +	strvec_clear(&ref_patterns);
> 
> Huh. So `ref_patterns` is simply a dummy variable, I was wondering why
> we simply can't set `filter.name_patterns = NULL`. I see that it is
> because in `filter_pattern_match()` we do `if
> (!*filter->name_patterns)`. This is not a great interface for
> ref-filter. Perhaps we could add a precursor commit like:
> 
> diff --git a/ref-filter.c b/ref-filter.c
> index 520d2539c9..20284b5918 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2664,7 +2664,7 @@ static int match_name_as_path(const char
> **pattern, const char *refname,
>  /* Return 1 if the refname matches one of the patterns, otherwise 0. */
>  static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>  {
> -	if (!*filter->name_patterns)
> +	if (!filter->name_patterns || !*filter->name_patterns)
>  		return 1; /* No pattern always matches */
>  	if (filter->match_as_path)
>  		return match_name_as_path(filter->name_patterns, refname,
> diff --git a/ref-filter.h b/ref-filter.h
> index f22ca94b49..44d9b481ad 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -109,6 +109,7 @@ struct ref_format {
>  #define REF_FILTER_INIT { \
>  	.points_at = OID_ARRAY_INIT, \
>  	.exclude = STRVEC_INIT, \
> +	.name_pattern = NULL, \
>  }
>  #define REF_FORMAT_INIT {             \
>  	.use_color = -1,              \