Re: [PATCH v4 11/15] commit-graph: add --changed-paths option to write subcommand
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Jun 7, 2020, 22:21 UTC
- Message-ID
- <20200607222106.GB2898@szeder.dev>
- In-Reply-To
- <c8b86c383abdbbd31ba307eb7e79942ddde1b711.1586192395.git.gitgitgadget@gmail.com>
On Mon, Apr 06, 2020 at 04:59:51PM +0000, Garima Singh via GitGitGadget wrote:
Show 28 quoted lines
> From: Garima Singh <garima.singh@microsoft.com> > > Add --changed-paths option to git commit-graph write. This option will > allow users to compute information about the paths that have changed > between a commit and its first parent, and write it into the commit graph > file. If the option is passed to the write subcommand we set the > COMMIT_GRAPH_WRITE_BLOOM_FILTERS flag and pass it down to the > commit-graph logic. > > Helped-by: Derrick Stolee <dstolee@microsoft.com> > Signed-off-by: Garima Singh <garima.singh@microsoft.com> > --- > Documentation/git-commit-graph.txt | 5 +++++ > builtin/commit-graph.c | 9 +++++++-- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > index 28d1fee5053..f4b13c005b8 100644 > --- a/Documentation/git-commit-graph.txt > +++ b/Documentation/git-commit-graph.txt > @@ -57,6 +57,11 @@ or `--stdin-packs`.) > With the `--append` option, include all commits that are present in the > existing commit-graph file. > + > +With the `--changed-paths` option, compute and write information about the > +paths changed between a commit and it's first parent. This operation can > +take a while on large repositories. It provides significant performance gains > +for getting history of a directory or a file with `git log -- <path>`.
So 'git commit-graph write' only computes and writes changed path Bloom filters if this option is specified. Though not mentioned in the documentation or in the commit message, the negated '--no-changed-paths' is supported as well, and it removes Bloom filters from the commit-graph file. All this is quite reasonable.
However, the most important question is what happens when the commit-graph file already contains Bloom filters and neither of these options are specified on the command line. This isn't mentioned in the docs or in the commit message, either, but as it is implemented in this patch (i.e. COMMIT_GRAPH_WRITE_BLOOM_FILTERS is not passed from the builtin to the commit-graph logic) all those existing Bloom filters are removed from the commit-graph. Considering how expensive it was to compute those Bloom filters this might not be the most desirable behaviour.
This is important, because 'git commit-graph write' is not the only command that writes the commit-graph file. 'git gc' does that by default, too, and will wipe out any modified path Bloom filters while doing so. Worse, the user doesn't even have to invoke 'git gc' manually, because a lot of git commands invoke 'git gc --auto'.
$ git commit-graph write --reachable --changed-paths $ ~/src/git/t/helper/test-tool read-graph |grep ^chunks chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data $ git gc --quiet $ ~/src/git/t/helper/test-tool read-graph |grep ^chunks chunks: oid_fanout oid_lookup commit_metadata
Consequently, if users want to use modified path Bloom filters, then they should avoid gc, both manual and auto, or they'll have to re-generate the Bloom filters every once in a while. That is definitely not the desired behaviour.
Now compare this e.g. to the behaviour of 'git update-index --split-index' and '--untracked-cache': both of these options turn on features that improve performance and write extra stuff to the index, and after they did so all subsequent git commands updating the index will keep writing that extra stuff, including 'git update-index' itself even without those options, until it's finally invoked with the corresponding '--no-...' option. I particularly like how '--[no-]untracked-cache' and 'core.untrackedCache' work together and warn when the given command line option goes against the configured value, and I think the command line options and configuration variables controlling modified path Bloom filters should behave similarly.
Show 54 quoted lines
> With the `--split` option, write the commit-graph as a chain of multiple
> commit-graph files stored in `<dir>/info/commit-graphs`. The new commits
> not already in the commit-graph are added in a new "tip" file. This file
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index d1ab6625f63..cacb5d04a80 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -9,7 +9,7 @@
>
> static char const * const builtin_commit_graph_usage[] = {
> N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
>
> @@ -19,7 +19,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {
> };
>
> static const char * const builtin_commit_graph_write_usage[] = {
> - N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
> + N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--changed-paths] [--[no-]progress] <split options>"),
> NULL
> };
>
> @@ -32,6 +32,7 @@ static struct opts_commit_graph {
> int split;
> int shallow;
> int progress;
> + int enable_changed_paths;
> } opts;
>
> static struct object_directory *find_odb(struct repository *r,
> @@ -135,6 +136,8 @@ static int graph_write(int argc, const char **argv)
> N_("start walk at commits listed by stdin")),
> OPT_BOOL(0, "append", &opts.append,
> N_("include all commits already in the commit-graph file")),
> + OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
> + N_("enable computation for changed paths")),
> OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
> OPT_BOOL(0, "split", &opts.split,
> N_("allow writing an incremental commit-graph file")),
> @@ -168,6 +171,8 @@ static int graph_write(int argc, const char **argv)
> flags |= COMMIT_GRAPH_WRITE_SPLIT;
> if (opts.progress)
> flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> + if (opts.enable_changed_paths)
> + flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
>
> read_replace_refs = 0;
> odb = find_odb(the_repository, opts.obj_dir);
> --
> gitgitgadget
>