From: Patrick Steinhardt Date: Tue, 06 Oct 2026 05:51:27 GMT Subject: Re: [PATCH v2 1/1] repo: add filtering options to "repo structure" Message-ID: In-Reply-To: <20261005174045.1900391-2-markchucarroll@fastmail.com> On Mon, Oct 05, 2026 at 01:40:44PM -0400, Mark C. Chu-Carroll wrote: > Implement filtering for repo structure, imitating the mechanism > used in "git log". The message should give an explanation of what this change does, and what the motivation behind it is. > diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc > index ed7d80c690..5cbdf8e727 100644 > --- a/Documentation/git-repo.adoc > +++ b/Documentation/git-repo.adoc > @@ -10,7 +10,7 @@ SYNOPSIS > [synopsis] > git repo info [--format=(lines|nul) | -z] [--all | ...] > git repo info --keys [--format=(lines|nul) | -z] > -git repo structure [--format=(table|lines|nul) | -z] > +git repo structure [--format=(table|lines|nul) | -z] [...] I think we should probably have this be `[...]`. > @@ -56,9 +56,10 @@ supported: > `nul`::: > Similar to `lines`, but using a _NUL_ character after each value. > > -`structure [--format=(table|lines|nul) | -z]`:: > - Retrieve statistics about the current repository structure. The > - following kinds of information are reported: > +`structure [--format=(table|lines|nul) | -z] [...]:: Same here. > @@ -66,6 +67,16 @@ supported: > * Total disk size of reachable objects by type > * Largest reachable objects in the repository by type > + > +The set of objects counted can be filtered by specifying a > +collection of query clauses to select which objects will be s/query clauses/revisions/, which is a well-defined term. So with this change I think we can drop most of the remaining paragraph, except for the last sentence. > +counted. These parameters follow the same syntax as the parameters > +to similar commands like `git log`. Semantically, these parameters > +are treated as a collection of include and exclude specifiers. Th > +set of objects counted will consist of all objects reachable from > +an object included by one of the include specifiers via a path that > +does not include an object in an exclude clause. If no includes > +are specified, then the include set is all reachable objects. > ++ > The output format can be chosen through the flag `--format`. Three formats are > supported: > + > diff --git a/builtin/repo.c b/builtin/repo.c > index 84e012f83f..b3aca71298 100644 > --- a/builtin/repo.c > +++ b/builtin/repo.c > @@ -946,12 +946,20 @@ static int cmd_repo_structure(int argc, const char **argv, const char *prefix, > OPT_BOOL(0, "progress", &show_progress, N_("show progress")), > OPT_END() > }; > + struct setup_revision_opt s_r_opt; > + memset(&s_r_opt, 0, sizeof(s_r_opt)); > + s_r_opt.def = "HEAD"; > + s_r_opt.revarg_opt = REVARG_COMMITTISH; This can be: struct setup_revision_opt s_r_opt = { .def = "HEAD", .revarg_opt = REVARG_COMMITTISH, }; But I wonder whether we want to pass it at all: - `.def` specifies the default, but do we even want to have one when the user has passed arguments? - `.revarg_opt` makes us treat it like a committish by default, but a user may for example want to figure out the size of all objects reachable from a specific tree, only. So maybe we shouldn't be setting this at all and just pass `NULL` to `setup_revisions()`? > - argc = parse_options(argc, argv, prefix, options, repo_structure_usage, 0); > - if (argc) > - usage(_("too many arguments")); > + argc = parse_options(argc, argv, prefix, options, repo_structure_usage, > + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT); Makes sense. Here we keep argv0 because of `setup_revisions()`' weird calling convention. And we also ignore any unknown options so that we can pass them along, too. > repo_init_revisions(repo, &revs, prefix); > + if (argc > 1) { > + argc = setup_revisions(argc, argv, &revs, &s_r_opt); > + if (argc > 1) > + usage(_("too many arguments")); > + } > > if (show_progress < 0) > show_progress = isatty(2); And then, if we have any additional parameters then we pass it on to `setup_revisions()`. > diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh > index 02cc2b594a..eb2c595955 100755 > --- a/t/t1901-repo-structure.sh > +++ b/t/t1901-repo-structure.sh > @@ -144,6 +144,90 @@ test_expect_success SHA1 'repository with references and objects' ' > ) > ' > > +test_expect_success SHA1 'repository with references and objects, filtered' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit_bulk 1005 && > + git tag -a foo -m bar && > + > + oid="$(git rev-parse HEAD)" && > + git update-ref refs/remotes/origin/foo "$oid" && > + git checkout -b grobble && > + test_commit_bulk --ref=refs/heads/grobble 20 && > + git checkout master && > + test_commit_bulk 20 && > + # Also creates a commit, tree, and blob. > + git notes add -m foo && > + > + # git-rev-list(1) --disk-usage=human option printing the full > + # "byte/bytes" unit string instead of just "B". > + cat >expect <<-EOF && > + | Repository structure | Value | > + | ------------------------- | ---------- | > + | * References | | > + | * Count | 5 | > + | * Branches | 2 | > + | * Tags | 1 | > + | * Remotes | 1 | > + | * Others | 1 | > + | | | > + | * Reachable objects | | > + | * Count | 3.06 k | > + | * Commits | 1.05 k | > + | * Trees | 1.01 k | > + | * Blobs | 1.01 k | > + | * Tags | 1 | > + | * Inflated size | 16.04 MiB | > + | * Commits | 226.54 KiB | > + | * Trees | 15.81 MiB | > + | * Blobs | 11.68 KiB | > + | * Tags | 132 B | > + | * Disk size | $(object_type_disk_usage all true) | > + | * Commits | $(object_type_disk_usage commit true) | > + | * Trees | $(object_type_disk_usage tree true) | > + | * Blobs | $(object_type_disk_usage blob true) | > + | * Tags | $(object_type_disk_usage tag) B | > + | | | > + | * Largest objects | | > + | * Commits | | > + | * Maximum size [1] | 223 B | > + | * Maximum parents [2] | 1 | > + | * Trees | | > + | * Maximum size [3] | 32.29 KiB | > + | * Maximum entries [4] | 1.01 k | > + | * Blobs | | > + | * Maximum size [5] | 13 B | > + | * Tags | | > + | * Maximum size [6] | 132 B | > + > + [1] 0dc91eb18580102a3a216c8bfecedeba2b9f9b9a > + [2] df6400c01440c329f1011669c4c26cc0c7852887 > + [3] 60665251ab71dbd8c18d9bf2174f4ee0d58aa06c > + [4] 60665251ab71dbd8c18d9bf2174f4ee0d58aa06c > + [5] 97d808e45116bf02103490294d3d46dad7a2ac62 > + [6] 4dae4f5954f5e6feb3577cfb1b181daa3fd3afd2 > + EOF > + > + git repo structure >actual 2>actual-err && > + cp actual /tmp/actual && > + cp expect /tmp/expect && > + test_cmp expect actual && > + test_line_count = 0 actual-err && > + > + git repo structure grobble ^master >actual 2>actual-err && > + cp actual /tmp && > + cp actual-err /tmp && > + test_grep "| \* Commits | 21 |" actual && > + test_grep "| \* Trees | 2 |" actual && > + test_grep "| \* Commits | 4.50 KiB |" actual && > + test_grep "| \* Trees | 32.35 KiB |" actual && > + test_grep "| \* Blobs | 11.68 KiB |" actual && > + test_line_count = 0 actual-err > + ) > +' I wonder whether we maybe want to have some additional tests that assert that you can also pass e.g.: - A tree or blob. - Revision options, like for example `--all --filter=object:type=blob`. To make the test a bit less repetitive we might also want to use `--format=lines` and then only check for "objects.*.{inflated,disk}_size" to exercise only the parts that matter to this test. Thanks! Patrick